Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

use AddHandler to register pointer handlers so we also get 'handled' events #1646

Merged
merged 1 commit into from
Feb 19, 2018

Conversation

mrousal
Copy link

@mrousal mrousal commented Feb 15, 2018

Originally only unhandled events were propagated up and pointer events from native components with their own handling where ignored. For example clicks on selectable text were ignored etc so this should fix #564.

@mrousal mrousal changed the title use AddHandler to register pointer handlers so we also also 'handled' events use AddHandler to register pointer handlers so we also get 'handled' events Feb 15, 2018
@rozele
Copy link
Collaborator

rozele commented Feb 15, 2018

Thanks @mrousal! I wonder if we need something a bit more robust to detect when a "handled" event should be re-handled or not. For example, Android and iOS have the concept of the "JS responder" (see https://github.com/facebook/react-native/blob/4f886a29a1234c967deae2354bbc5092e0e6595e/ReactAndroid/src/main/java/com/facebook/react/uimanager/UIManagerModule.java#L572). Currently, we're silently ignoring this part of the UIManager, but I wonder if we could leverage this to make this part more robust.

I'm concerned about the impact on, e.g., ScrollView, which relies on the initial press event being canceled to function properly.

@mrousal
Copy link
Author

mrousal commented Feb 15, 2018

I agree it would be better, but with current implementation relying on pointer events being routed to root view this seems to be one way how to receive all of them.
I have also tested the app some after the change and didn't notice any performance degradation or other issues.

@rozele
Copy link
Collaborator

rozele commented Feb 15, 2018

@mrousal - I'm wondering if this could help remove the hack we have in place to support "swipeable rows" (79b8ae4), or if it will also help fix #1591?

@mrousal
Copy link
Author

mrousal commented Feb 15, 2018

It might help with first one as it makes sure that all pointer events are processed and can be passed to JS (regardless if some native UIElement marked them as handled), based on description I don't think it is related to #1591 as problem there is Parent == null and not events themselves.

@rozele
Copy link
Collaborator

rozele commented Feb 19, 2018

Just double checked, doesn't look like this has any impact on our need for ManipulationMode to implement touch handling inside ScrollViewer.

@rozele
Copy link
Collaborator

rozele commented Feb 19, 2018

I also confirmed nothing breaks in RNTester.

@rozele rozele merged commit 9fe74a0 into microsoft:master Feb 19, 2018
ladipro pushed a commit to ladipro/react-native-windows that referenced this pull request May 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

When selection is enabled, onPress handlers on Text components do not work
3 participants