From 910044a41dbbf69aebc67598e0f052e13e7a767f Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Fri, 2 Dec 2016 19:33:24 -0800 Subject: [PATCH 1/3] Add failing event handler test When we perform an update to the event handler we properly update the immediate Fiber pointer of a child to be the current. However, when we bubble events we use the return pointer which is not guaranteed to point to the current Fiber even if we start from the current. This manifests itself when we bailout in a parent. So I made the tests use a PureComponent to illustrate this scenario. There is already a failing case but I'm adding another one too. --- scripts/fiber/tests-failing.txt | 4 ++ scripts/fiber/tests-passing.txt | 1 - .../ReactBrowserEventEmitter-test.js | 52 ++++++++++++++++++- 3 files changed, 55 insertions(+), 2 deletions(-) diff --git a/scripts/fiber/tests-failing.txt b/scripts/fiber/tests-failing.txt index 16dd1717af37..9b46b63ed5ea 100644 --- a/scripts/fiber/tests-failing.txt +++ b/scripts/fiber/tests-failing.txt @@ -27,6 +27,10 @@ src/renderers/art/__tests__/ReactART-test.js src/renderers/dom/__tests__/ReactDOMProduction-test.js * should throw with an error code in production +src/renderers/dom/shared/__tests__/ReactBrowserEventEmitter-test.js +* should bubble to the right handler after an update +* should invoke handlers that were removed while bubbling + src/renderers/dom/shared/__tests__/ReactDOM-test.js * throws in render() if the mount callback is not a function * throws in render() if the update callback is not a function diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index a1401beb4089..7fdf74256482 100644 --- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -561,7 +561,6 @@ src/renderers/dom/shared/__tests__/ReactBrowserEventEmitter-test.js * should support stopPropagation() * should stop after first dispatch if stopPropagation * should not stopPropagation if false is returned -* should invoke handlers that were removed while bubbling * should not invoke newly inserted handlers while bubbling * should have mouse enter simulated by test utils * should infer onTouchTap from a touchStart/End diff --git a/src/renderers/dom/shared/__tests__/ReactBrowserEventEmitter-test.js b/src/renderers/dom/shared/__tests__/ReactBrowserEventEmitter-test.js index 6462df35f6cd..0a6244e6f340 100644 --- a/src/renderers/dom/shared/__tests__/ReactBrowserEventEmitter-test.js +++ b/src/renderers/dom/shared/__tests__/ReactBrowserEventEmitter-test.js @@ -75,11 +75,21 @@ describe('ReactBrowserEventEmitter', () => { var PARENT_PROPS = {}; var CHILD_PROPS = {}; + function Child(props) { + return
CHILD = c} {...props} />; + } + + class ChildWrapper extends React.PureComponent { + render() { + return ; + } + } + function renderTree() { ReactDOM.render(
GRANDPARENT = c} {...GRANDPARENT_PROPS}>
PARENT = c} {...PARENT_PROPS}> -
CHILD = c} {...CHILD_PROPS} /> +
, container @@ -193,6 +203,46 @@ describe('ReactBrowserEventEmitter', () => { expect(idCallOrder[2]).toBe(GRANDPARENT); }); + it('should bubble to the right handler after an update', () => { + putListener( + GRANDPARENT, + ON_CLICK_KEY, + recordID.bind(null, 'GRANDPARENT') + ); + putListener( + PARENT, + ON_CLICK_KEY, + recordID.bind(null, 'PARENT') + ); + putListener( + CHILD, + ON_CLICK_KEY, + recordID.bind(null, 'CHILD') + ); + ReactTestUtils.Simulate.click(CHILD); + expect(idCallOrder).toEqual([ + 'CHILD', + 'PARENT', + 'GRANDPARENT', + ]); + + idCallOrder = []; + + // Update just the grand parent without updating the child. + putListener( + GRANDPARENT, + ON_CLICK_KEY, + recordID.bind(null, 'UPDATED_GRANDPARENT') + ); + + ReactTestUtils.Simulate.click(CHILD); + expect(idCallOrder).toEqual([ + 'CHILD', + 'PARENT', + 'UPDATED_GRANDPARENT', + ]); + }); + it('should continue bubbling if an error is thrown', () => { putListener( CHILD, From 2336b8183d7b2ef6edba34bb2472cf5e3aa90a21 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Fri, 2 Dec 2016 19:52:56 -0800 Subject: [PATCH 2/3] Remove forwarding from tree traversal in EventPluginUtils This is unnecessary forwarding since this is no longer injectable. However, I'd like to use the injectable getInstanceFromNode from TreeTraversal so this avoids a cyclic dependency. --- .../shared/shared/event/EventPluginUtils.js | 16 ---------------- .../shared/shared/event/EventPropagators.js | 10 +++++----- .../event/eventPlugins/ResponderEventPlugin.js | 5 +++-- 3 files changed, 8 insertions(+), 23 deletions(-) diff --git a/src/renderers/shared/shared/event/EventPluginUtils.js b/src/renderers/shared/shared/event/EventPluginUtils.js index c2861ddbf5ba..a616562807e0 100644 --- a/src/renderers/shared/shared/event/EventPluginUtils.js +++ b/src/renderers/shared/shared/event/EventPluginUtils.js @@ -11,7 +11,6 @@ 'use strict'; -var ReactTreeTraversal = require('ReactTreeTraversal'); var ReactErrorUtils = require('ReactErrorUtils'); var invariant = require('invariant'); @@ -226,21 +225,6 @@ var EventPluginUtils = { getNodeFromInstance: function(node) { return ComponentTree.getNodeFromInstance(node); }, - isAncestor: function(a, b) { - return ReactTreeTraversal.isAncestor(a, b); - }, - getLowestCommonAncestor: function(a, b) { - return ReactTreeTraversal.getLowestCommonAncestor(a, b); - }, - getParentInstance: function(inst) { - return ReactTreeTraversal.getParentInstance(inst); - }, - traverseTwoPhase: function(target, fn, arg) { - return ReactTreeTraversal.traverseTwoPhase(target, fn, arg); - }, - traverseEnterLeave: function(from, to, fn, argFrom, argTo) { - return ReactTreeTraversal.traverseEnterLeave(from, to, fn, argFrom, argTo); - }, injection: injection, }; diff --git a/src/renderers/shared/shared/event/EventPropagators.js b/src/renderers/shared/shared/event/EventPropagators.js index da1b23841665..75cd7d4e45db 100644 --- a/src/renderers/shared/shared/event/EventPropagators.js +++ b/src/renderers/shared/shared/event/EventPropagators.js @@ -12,7 +12,7 @@ 'use strict'; var EventPluginHub = require('EventPluginHub'); -var EventPluginUtils = require('EventPluginUtils'); +var ReactTreeTraversal = require('ReactTreeTraversal'); var accumulateInto = require('accumulateInto'); var forEachAccumulated = require('forEachAccumulated'); @@ -62,7 +62,7 @@ function accumulateDirectionalDispatches(inst, phase, event) { */ function accumulateTwoPhaseDispatchesSingle(event) { if (event && event.dispatchConfig.phasedRegistrationNames) { - EventPluginUtils.traverseTwoPhase( + ReactTreeTraversal.traverseTwoPhase( event._targetInst, accumulateDirectionalDispatches, event @@ -77,8 +77,8 @@ function accumulateTwoPhaseDispatchesSingleSkipTarget(event) { if (event && event.dispatchConfig.phasedRegistrationNames) { var targetInst = event._targetInst; var parentInst = - targetInst ? EventPluginUtils.getParentInstance(targetInst) : null; - EventPluginUtils.traverseTwoPhase( + targetInst ? ReactTreeTraversal.getParentInstance(targetInst) : null; + ReactTreeTraversal.traverseTwoPhase( parentInst, accumulateDirectionalDispatches, event @@ -124,7 +124,7 @@ function accumulateTwoPhaseDispatchesSkipTarget(events) { } function accumulateEnterLeaveDispatches(leave, enter, from, to) { - EventPluginUtils.traverseEnterLeave( + ReactTreeTraversal.traverseEnterLeave( from, to, accumulateDispatches, diff --git a/src/renderers/shared/shared/event/eventPlugins/ResponderEventPlugin.js b/src/renderers/shared/shared/event/eventPlugins/ResponderEventPlugin.js index 1973592d8a56..d4057833c26a 100644 --- a/src/renderers/shared/shared/event/eventPlugins/ResponderEventPlugin.js +++ b/src/renderers/shared/shared/event/eventPlugins/ResponderEventPlugin.js @@ -13,6 +13,7 @@ var EventPluginUtils = require('EventPluginUtils'); var EventPropagators = require('EventPropagators'); +var ReactTreeTraversal = require('ReactTreeTraversal'); var ResponderSyntheticEvent = require('ResponderSyntheticEvent'); var ResponderTouchHistoryStore = require('ResponderTouchHistoryStore'); @@ -331,7 +332,7 @@ function setResponderAndExtractTransfer( // TODO: stop one short of the current responder. var bubbleShouldSetFrom = !responderInst ? targetInst : - EventPluginUtils.getLowestCommonAncestor(responderInst, targetInst); + ReactTreeTraversal.getLowestCommonAncestor(responderInst, targetInst); // When capturing/bubbling the "shouldSet" event, we want to skip the target // (deepest ID) if it happens to be the current responder. The reasoning: @@ -454,7 +455,7 @@ function noResponderTouches(nativeEvent) { if (target !== null && target !== undefined && target !== 0) { // Is the original touch location inside of the current responder? var targetInst = EventPluginUtils.getInstanceFromNode(target); - if (EventPluginUtils.isAncestor(responderInst, targetInst)) { + if (ReactTreeTraversal.isAncestor(responderInst, targetInst)) { return false; } } From ee4984980e7f7b030e2a4b371bb45db294924f3b Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Fri, 2 Dec 2016 20:03:09 -0800 Subject: [PATCH 3/3] Get the "current" Fiber from each node along the path This fixes the issue where using the .return pointer isn't guaranteed to return the current Fiber so we might read the wrong props when we try to get the current event. --- scripts/fiber/tests-failing.txt | 4 ---- scripts/fiber/tests-passing.txt | 2 ++ src/renderers/shared/shared/ReactTreeTraversal.js | 14 ++++++++++++-- .../shared/shared/event/EventPluginHub.js | 5 ----- 4 files changed, 14 insertions(+), 11 deletions(-) diff --git a/scripts/fiber/tests-failing.txt b/scripts/fiber/tests-failing.txt index 9b46b63ed5ea..16dd1717af37 100644 --- a/scripts/fiber/tests-failing.txt +++ b/scripts/fiber/tests-failing.txt @@ -27,10 +27,6 @@ src/renderers/art/__tests__/ReactART-test.js src/renderers/dom/__tests__/ReactDOMProduction-test.js * should throw with an error code in production -src/renderers/dom/shared/__tests__/ReactBrowserEventEmitter-test.js -* should bubble to the right handler after an update -* should invoke handlers that were removed while bubbling - src/renderers/dom/shared/__tests__/ReactDOM-test.js * throws in render() if the mount callback is not a function * throws in render() if the update callback is not a function diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index 7fdf74256482..21ebd152e7fb 100644 --- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -556,11 +556,13 @@ src/renderers/dom/shared/__tests__/ReactBrowserEventEmitter-test.js * should invoke a simple handler registered on a node * should not invoke handlers if ReactBrowserEventEmitter is disabled * should bubble simply +* should bubble to the right handler after an update * should continue bubbling if an error is thrown * should set currentTarget * should support stopPropagation() * should stop after first dispatch if stopPropagation * should not stopPropagation if false is returned +* should invoke handlers that were removed while bubbling * should not invoke newly inserted handlers while bubbling * should have mouse enter simulated by test utils * should infer onTouchTap from a touchStart/End diff --git a/src/renderers/shared/shared/ReactTreeTraversal.js b/src/renderers/shared/shared/ReactTreeTraversal.js index 22a72b0a672f..a53eaf7b4c3f 100644 --- a/src/renderers/shared/shared/ReactTreeTraversal.js +++ b/src/renderers/shared/shared/ReactTreeTraversal.js @@ -12,6 +12,7 @@ 'use strict'; var { HostComponent } = require('ReactTypeOfWork'); +var { getNodeFromInstance, getInstanceFromNode } = require('EventPluginUtils'); function getParent(inst) { if (inst._hostParent !== undefined) { @@ -22,9 +23,18 @@ function getParent(inst) { inst = inst.return; // TODO: If this is a HostRoot we might want to bail out. // That is depending on if we want nested subtrees (layers) to bubble - // events to their parent. + // events to their parent. We could also go through parentNode on the + // host node but that wouldn't work for React Native and doesn't let us + // do the portal feature. } while (inst && inst.tag !== HostComponent); - return inst; + // Going through the Host Node will guarantee that we get the "current" + // Fiber, instead of the alternate because that pointer is updated when + // props update. + // TODO: This is a bit hacky and possibly slow. We should ideally have + // something in the reconciler that allow us to do this safely. + if (inst) { + return getInstanceFromNode(getNodeFromInstance(inst)); + } } return null; } diff --git a/src/renderers/shared/shared/event/EventPluginHub.js b/src/renderers/shared/shared/event/EventPluginHub.js index 402335427822..57d80423340c 100644 --- a/src/renderers/shared/shared/event/EventPluginHub.js +++ b/src/renderers/shared/shared/event/EventPluginHub.js @@ -126,11 +126,6 @@ var EventPluginHub = { // TODO: shouldPreventMouseEvent is DOM-specific and definitely should not // live here; needs to be moved to a better place soon if (typeof inst.tag === 'number') { - // TODO: This is not safe because we might want the *other* Fiber's - // props depending on which is the current one. This will usually be the - // current Fiber but if we're walking up the tree using TreeTraversal for - // bubbling, we will not be guaranteed to walk up the current tree when - // a Fiber has been reused. const props = inst.memoizedProps; listener = props[registrationName]; if (shouldPreventMouseEvent(registrationName, inst.type, props)) {