Removed optimization for events without target in ReactNativeEventEmitter #9219
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sophiebits
reviewed
Mar 20, 2017
There was a problem hiding this comment.
This looks good though I'm unsure about the test.
(I would have liked to match this change in the corresponding DOM code at src/renderers/dom/shared/ReactEventListener.js handleTopLevelImpl but it seems like maybe the logic there is different enough due to the nested roots logic that it is probably fine to leave them separate.)
|
|
||
| expect(getResponderId()).toBe(null); | ||
|
|
||
| expect(log).toEqual([ |
There was a problem hiding this comment.
Not sure if I'm missing something obvious, but how does this test the codepath in question? It seems to me that the simulated touches you have both hit mounted views.
There was a problem hiding this comment.
Thank you for feedback, Ben!
I changed the test to match the original conditions.
…tter This PR fixes the problem originally introduced in facebook#6590 The problem is that `ResponderEventPlugin` (and `ResponderTouchHistoryStore`) relies on that fact that touch events are balanced. So if one `startish` event happened, should be coming `endish` event. Otherwise there is no way to maintain internal `trackedTouchCount` counter. So, if we drop some events, we break this logic. Moreover, that optimization clearly contradict with this statement from `ResponderEventPlugin`: ``` We must be resilient to `targetInst` being `null` on `touchMove` or `touchEnd`. On certain platforms, this means that a native scroll has assumed control and the original touch targets are destroyed. ``` This issue causes several major problems in React Native, and one of them (finally!) is easy to reproduce: facebook/react-native#12976 . The test also illustrates this problem.
|
Thanks! |
bvaughn
pushed a commit
to bvaughn/react
that referenced
this pull request
Mar 28, 2017
bvaughn
added a commit
that referenced
this pull request
Mar 28, 2017
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR fixes the problem originally introduced in #6590
The problem is that
ResponderEventPlugin(andResponderTouchHistoryStore) relies on that fact that touch events are balanced.So if one
startishevent happened, should be comingendishevent. Otherwise there is no way to maintain internaltrackedTouchCountcounter. So, if we drop some events, we break this logic.Moreover, that optimization clearly contradict with this statement from
ResponderEventPlugin:This issue causes several major problems in React Native, and one of them (finally!) is easy to reproduce: facebook/react-native#12976 .
The test also illustrates this problem.
@spicyj, please review. 😄