From c5dc9a91da120db7d32c419468b3c46a6be1a8cf Mon Sep 17 00:00:00 2001 From: Ben Alpert Date: Mon, 31 Aug 2015 15:54:46 -0700 Subject: [PATCH] Don't crash in event handling when mixing React copies Should fix #1939. Test Plan: With two copies of React, render a div using React1 and use that as a container to render a div with React2. Add onMouseEnter/onMouseLeave to both divs that log. Mouse around and see correct logs (as if each React was isolated), no errors. --- .../dom/client/ReactEventListener.js | 4 +- src/renderers/dom/client/ReactMount.js | 37 ++++++++++++++++--- .../ReactBrowserEventEmitter-test.js | 11 +++++- .../eventPlugins/EnterLeaveEventPlugin.js | 20 ++++++---- .../dom/client/wrappers/ReactDOMInput.js | 4 ++ .../__tests__/ReactInstanceHandles-test.js | 4 +- 6 files changed, 62 insertions(+), 18 deletions(-) diff --git a/src/renderers/dom/client/ReactEventListener.js b/src/renderers/dom/client/ReactEventListener.js index ea22e29e8a997..7fb23302e8b34 100644 --- a/src/renderers/dom/client/ReactEventListener.js +++ b/src/renderers/dom/client/ReactEventListener.js @@ -112,11 +112,11 @@ function handleTopLevelWithPath(bookKeeping) { var eventsFired = 0; for (var i = 0; i < path.length; i++) { var currentPathElement = path[i]; - var currentPathElementID = ReactMount.getID(currentPathElement); if (currentPathElement.nodeType === DOCUMENT_FRAGMENT_NODE_TYPE) { currentNativeTarget = path[i + 1]; } - if (ReactMount.isRenderedByReact(currentPathElement)) { + if (ReactMount.isRenderedByThisReact(currentPathElement)) { + var currentPathElementID = ReactMount.getID(currentPathElement); var newRootID = ReactInstanceHandles.getReactRootIDFromNodeID( currentPathElementID ); diff --git a/src/renderers/dom/client/ReactMount.js b/src/renderers/dom/client/ReactMount.js index 42e013e0ce418..3a789f71ecce1 100644 --- a/src/renderers/dom/client/ReactMount.js +++ b/src/renderers/dom/client/ReactMount.js @@ -586,13 +586,14 @@ var ReactMount = { var reactRootElement = getReactRootElementInContainer(container); var containerHasReactMarkup = - reactRootElement && ReactMount.isRenderedByReact(reactRootElement); + reactRootElement && + !!internalGetID(reactRootElement); if (__DEV__) { if (!containerHasReactMarkup || reactRootElement.nextSibling) { var rootElementSibling = reactRootElement; while (rootElementSibling) { - if (ReactMount.isRenderedByReact(rootElementSibling)) { + if (internalGetID(rootElementSibling)) { warning( false, 'render(): Target node has markup rendered by React, but there ' + @@ -772,13 +773,37 @@ var ReactMount = { * @return {boolean} True if the DOM Element appears to be rendered by React. * @internal */ - isRenderedByReact: function(node) { + isRenderedByThisReact: function(node) { if (node.nodeType !== 1) { // Not a DOMElement, therefore not a React component return false; } - var id = ReactMount.getID(node); - return id ? id.charAt(0) === SEPARATOR : false; + // This node might be from another React instance, so we make sure not to + // examine the node cache here + var nodeID = internalGetID(node); + if (!nodeID) { + return false; + } + var reactRootID = ReactInstanceHandles.getReactRootIDFromNodeID(nodeID); + + // If containersByReactRootID contains the container we find by crawling up + // the tree, we know that this instance of React rendered the node. + // nb. isValid's strategy (with containsNode) does not work because render + // trees may be nested and we don't want a false positive in that case. + var current = node; + var lastID; + do { + lastID = internalGetID(current); + current = current.parentNode; + invariant( + current != null, + 'isRenderedByThisReact(...): Unexpected detached subtree found when ' + + 'traversing DOM from node `%s`.', + nodeID + ); + } while (lastID !== reactRootID); + + return current === containersByReactRootID[reactRootID]; }, /** @@ -792,7 +817,7 @@ var ReactMount = { getFirstReactDOM: function(node) { var current = node; while (current && current.parentNode !== current) { - if (ReactMount.isRenderedByReact(current)) { + if (ReactMount.isRenderedByThisReact(current)) { return current; } current = current.parentNode; diff --git a/src/renderers/dom/client/__tests__/ReactBrowserEventEmitter-test.js b/src/renderers/dom/client/__tests__/ReactBrowserEventEmitter-test.js index b36c8d5468cd3..4a59b54a85ef0 100644 --- a/src/renderers/dom/client/__tests__/ReactBrowserEventEmitter-test.js +++ b/src/renderers/dom/client/__tests__/ReactBrowserEventEmitter-test.js @@ -21,7 +21,8 @@ var setID = function(el, id) { ReactMount.setID(el, id); idToNode[id] = el; }; -var oldGetNode = ReactMount.getNode; +var oldGetNode; +var oldIsRenderedByThisReact; var EventPluginHub; var ReactBrowserEventEmitter; @@ -83,9 +84,16 @@ describe('ReactBrowserEventEmitter', function() { EventListener = require('EventListener'); ReactBrowserEventEmitter = require('ReactBrowserEventEmitter'); ReactTestUtils = require('ReactTestUtils'); + + oldGetNode = ReactMount.getNode; + oldIsRenderedByThisReact = ReactMount.oldIsRenderedByThisReact; ReactMount.getNode = function(id) { return idToNode[id]; }; + ReactMount.isRenderedByThisReact = function() { + return true; + }; + idCallOrder = []; tapMoveThreshold = TapEventPlugin.tapMoveThreshold; EventPluginHub.injection.injectEventPluginsByName({ @@ -95,6 +103,7 @@ describe('ReactBrowserEventEmitter', function() { afterEach(function() { ReactMount.getNode = oldGetNode; + ReactMount.isRenderedByThisReact = oldIsRenderedByThisReact; }); it('should store a listener correctly', function() { diff --git a/src/renderers/dom/client/eventPlugins/EnterLeaveEventPlugin.js b/src/renderers/dom/client/eventPlugins/EnterLeaveEventPlugin.js index 142b00eaf183d..6eccf1779e0f9 100644 --- a/src/renderers/dom/client/eventPlugins/EnterLeaveEventPlugin.js +++ b/src/renderers/dom/client/eventPlugins/EnterLeaveEventPlugin.js @@ -89,15 +89,24 @@ var EnterLeaveEventPlugin = { } } - var from, to; + var from; + var to; + var fromID = ''; + var toID = ''; if (topLevelType === topLevelTypes.topMouseOut) { from = topLevelTarget; - to = - getFirstReactDOM(nativeEvent.relatedTarget || nativeEvent.toElement) || - win; + fromID = topLevelTargetID; + to = getFirstReactDOM(nativeEvent.relatedTarget || nativeEvent.toElement); + if (to) { + toID = ReactMount.getID(to); + } else { + to = win; + } + to = to || win; } else { from = win; to = topLevelTarget; + toID = topLevelTargetID; } if (from === to) { @@ -105,9 +114,6 @@ var EnterLeaveEventPlugin = { return null; } - var fromID = from ? ReactMount.getID(from) : ''; - var toID = to ? ReactMount.getID(to) : ''; - var leave = SyntheticMouseEvent.getPooled( eventTypes.mouseLeave, fromID, diff --git a/src/renderers/dom/client/wrappers/ReactDOMInput.js b/src/renderers/dom/client/wrappers/ReactDOMInput.js index f547128cf8013..276336b70f2a7 100644 --- a/src/renderers/dom/client/wrappers/ReactDOMInput.js +++ b/src/renderers/dom/client/wrappers/ReactDOMInput.js @@ -141,6 +141,10 @@ function _handleChange(event) { otherNode.form !== rootNode.form) { continue; } + // This will throw if radio buttons rendered by different copies of React + // and the same name are rendered into the same form (same as #1939). + // That's probably okay; we don't support it just as we don't support + // mixing React with non-React. var otherID = ReactMount.getID(otherNode); invariant( otherID, diff --git a/src/renderers/shared/reconciler/__tests__/ReactInstanceHandles-test.js b/src/renderers/shared/reconciler/__tests__/ReactInstanceHandles-test.js index 4b5481149f22a..5bae6f594b014 100644 --- a/src/renderers/shared/reconciler/__tests__/ReactInstanceHandles-test.js +++ b/src/renderers/shared/reconciler/__tests__/ReactInstanceHandles-test.js @@ -74,10 +74,10 @@ describe('ReactInstanceHandles', function() { aggregatedArgs = []; }); - describe('isRenderedByReact', function() { + describe('isRenderedByThisReact', function() { it('should not crash on text nodes', function() { expect(function() { - ReactMount.isRenderedByReact(document.createTextNode('yolo')); + ReactMount.isRenderedByThisReact(document.createTextNode('yolo')); }).not.toThrow(); }); });