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

Removed optimization for events without target in ReactNativeEventEmitter #9219

Merged
merged 2 commits into from
Mar 21, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
94 changes: 94 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,97 @@ 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 = [];

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

render(true);

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

// Unmounting component 'one'.
render(false);

EventEmitter.receiveTouches(
'topTouchEnd',
[{target: getViewById('one'), 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([
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for feedback, Ben!
I changed the test to match the original conditions.

'one responder start',
'two responder start',
'two responder end',
]);
});