Skip to content

Commit

Permalink
Removed optimization for events without target in ReactNativeEventEmi…
Browse files Browse the repository at this point in the history
…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.
  • Loading branch information
shergin committed Mar 20, 2017
1 parent f606e0e commit e24b381
Show file tree
Hide file tree
Showing 3 changed files with 86 additions and 5 deletions.
1 change: 1 addition & 0 deletions scripts/fiber/tests-passing.txt
Original file line number Diff line number Diff line change
Expand Up @@ -1438,6 +1438,7 @@ src/renderers/native/__tests__/ReactNativeEvents-test.js
* handles events
* handles events on text nodes
* handles when a responder is unmounted while a touch sequence is in progress
* handles events without target

src/renderers/native/__tests__/ReactNativeMount-test.js
* should be able to create and render a native component
Expand Down
5 changes: 0 additions & 5 deletions src/renderers/native/ReactNativeEventEmitter.js
Original file line number Diff line number Diff line change
Expand Up @@ -102,11 +102,6 @@ var ReactNativeEventEmitter = {
) {
var nativeEvent = nativeEventParam || EMPTY_NATIVE_EVENT;
var inst = ReactNativeComponentTree.getInstanceFromNode(rootNodeID);
if (!inst) {
// If the original instance is already gone, we don't have to dispatch
// any events.
return;
}
ReactGenericBatching.batchedUpdates(function() {
ReactNativeEventEmitter.handleTopLevel(
topLevelType,
Expand Down
85 changes: 85 additions & 0 deletions src/renderers/native/__tests__/ReactNativeEvents-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -267,3 +267,88 @@ it('handles when a responder is unmounted while a touch sequence is in progress'
expect(getResponderId()).toBe('two');
expect(log).toEqual(['two responder start']);
});

it('handles events without target', () => {
var EventEmitter = RCTEventEmitter.register.mock.calls[0][0];
var View = createReactNativeComponentClass({
validAttributes: {id: true},
uiViewClassName: 'View',
});

function getViewById(id) {
return UIManager.createView.mock.calls.find(
args => args[3] && args[3].id === id,
)[0];
}

function getResponderId() {
const responder = ResponderEventPlugin._getResponder();
if (responder === null) {
return null;
}
const props = typeof responder.tag === 'number'
? responder.memoizedProps
: responder._currentElement.props;
return props ? props.id : null;
}

var log = [];
ReactNative.render(
<View id="parent">
<View key={1}>
<View
id="one"
onResponderEnd={() => log.push('one responder end')}
onResponderStart={() => log.push('one responder start')}
onStartShouldSetResponder={() => true}
/>
</View>
<View key={2}>
<View
id="two"
onResponderEnd={() => log.push('two responder end')}
onResponderStart={() => log.push('two responder start')}
onStartShouldSetResponder={() => true}
/>
</View>
</View>,
1,
);

EventEmitter.receiveTouches(
'topTouchStart',
[{target: getViewById('one'), identifier: 17}],
[0],
);

EventEmitter.receiveTouches(
'topTouchEnd',
[{target: null, identifier: 17}],
[0],
);

expect(getResponderId()).toBe(null);

EventEmitter.receiveTouches(
'topTouchStart',
[{target: getViewById('two'), identifier: 18}],
[0],
);

expect(getResponderId()).toBe('two');

EventEmitter.receiveTouches(
'topTouchEnd',
[{target: getViewById('two'), identifier: 18}],
[0],
);

expect(getResponderId()).toBe(null);

expect(log).toEqual([
'one responder start',
'one responder end',
'two responder start',
'two responder end',
]);
});

0 comments on commit e24b381

Please sign in to comment.