From e7ce46ff28554d98a653b26c0e9a5fdfb63eea89 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 | 6 +- src/renderers/dom/client/ReactMount.js | 75 ++++++++++++------- .../ReactBrowserEventEmitter-test.js | 11 ++- .../eventPlugins/EnterLeaveEventPlugin.js | 20 +++-- .../dom/client/wrappers/ReactDOMInput.js | 4 + .../__tests__/ReactInstanceHandles-test.js | 8 -- 6 files changed, 77 insertions(+), 47 deletions(-) diff --git a/src/renderers/dom/client/ReactEventListener.js b/src/renderers/dom/client/ReactEventListener.js index ea22e29e8a9..422b0ee650e 100644 --- a/src/renderers/dom/client/ReactEventListener.js +++ b/src/renderers/dom/client/ReactEventListener.js @@ -112,11 +112,13 @@ 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)) { + // TODO: slow + var reactParent = ReactMount.getFirstReactDOM(currentPathElement); + if (reactParent === 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 e2ba7ebd4f9..f626bd43e7c 100644 --- a/src/renderers/dom/client/ReactMount.js +++ b/src/renderers/dom/client/ReactMount.js @@ -35,8 +35,6 @@ var shouldUpdateReactComponent = require('shouldUpdateReactComponent'); var validateDOMNesting = require('validateDOMNesting'); var warning = require('warning'); -var SEPARATOR = ReactInstanceHandles.SEPARATOR; - var ATTR_NAME = DOMProperty.ID_ATTRIBUTE_NAME; var nodeCache = {}; @@ -367,6 +365,48 @@ function hasNonRootReactChild(node) { ReactInstanceHandles.getReactRootIDFromNodeID(reactRootID) : false; } +/** + * Returns the first (deepest) ancestor of a node which is rendered by this copy + * of React. + */ +function findFirstReactDOMImpl(node) { + // This node might be from another React instance, so we make sure not to + // examine the node cache here + for (; node && node.parentNode !== node; node = node.parentNode) { + if (node.nodeType !== 1) { + // Not a DOMElement, therefore not a React component + continue; + } + var nodeID = internalGetID(node); + if (!nodeID) { + continue; + } + 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, + 'findFirstReactDOMImpl(...): Unexpected detached subtree found when ' + + 'traversing DOM from node `%s`.', + nodeID + ); + } while (lastID !== reactRootID); + + if (current === containersByReactRootID[reactRootID]) { + return node; + } + } + return null; +} + /** * Temporary (?) hack so that we can store all top-level pending updates on * composites instead of having to worry about different types of components @@ -602,7 +642,7 @@ var ReactMount = { var reactRootElement = getReactRootElementInContainer(container); var containerHasReactMarkup = - reactRootElement && ReactMount.isRenderedByReact(reactRootElement); + reactRootElement && !!internalGetID(reactRootElement); var containerHasNonRootReactChild = hasNonRootReactChild(container); if (__DEV__) { @@ -617,7 +657,7 @@ var ReactMount = { 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 ' + @@ -818,39 +858,16 @@ var ReactMount = { return ReactMount.findComponentRoot(reactRoot, id); }, - /** - * True if the supplied `node` is rendered by React. - * - * @param {*} node DOM Element to check. - * @return {boolean} True if the DOM Element appears to be rendered by React. - * @internal - */ - isRenderedByReact: 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; - }, - /** * Traverses up the ancestors of the supplied node to find a node that is a - * DOM representation of a React component. + * DOM representation of a React component rendered by this copy of React. * * @param {*} node * @return {?DOMEventTarget} * @internal */ getFirstReactDOM: function(node) { - var current = node; - while (current && current.parentNode !== current) { - if (ReactMount.isRenderedByReact(current)) { - return current; - } - current = current.parentNode; - } - return null; + return findFirstReactDOMImpl(node); }, /** diff --git a/src/renderers/dom/client/__tests__/ReactBrowserEventEmitter-test.js b/src/renderers/dom/client/__tests__/ReactBrowserEventEmitter-test.js index b36c8d5468c..3063754c202 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 oldGetFirstReactDOM; var EventPluginHub; var ReactBrowserEventEmitter; @@ -83,9 +84,16 @@ describe('ReactBrowserEventEmitter', function() { EventListener = require('EventListener'); ReactBrowserEventEmitter = require('ReactBrowserEventEmitter'); ReactTestUtils = require('ReactTestUtils'); + + oldGetNode = ReactMount.getNode; + oldGetFirstReactDOM = ReactMount.oldGetFirstReactDOM; ReactMount.getNode = function(id) { return idToNode[id]; }; + ReactMount.getFirstReactDOM = function(node) { + return node; + }; + idCallOrder = []; tapMoveThreshold = TapEventPlugin.tapMoveThreshold; EventPluginHub.injection.injectEventPluginsByName({ @@ -95,6 +103,7 @@ describe('ReactBrowserEventEmitter', function() { afterEach(function() { ReactMount.getNode = oldGetNode; + ReactMount.getFirstReactDOM = oldGetFirstReactDOM; }); 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 142b00eaf18..6eccf1779e0 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 f547128cf80..276336b70f2 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 4b5481149f2..f5dbc051ffc 100644 --- a/src/renderers/shared/reconciler/__tests__/ReactInstanceHandles-test.js +++ b/src/renderers/shared/reconciler/__tests__/ReactInstanceHandles-test.js @@ -74,14 +74,6 @@ describe('ReactInstanceHandles', function() { aggregatedArgs = []; }); - describe('isRenderedByReact', function() { - it('should not crash on text nodes', function() { - expect(function() { - ReactMount.isRenderedByReact(document.createTextNode('yolo')); - }).not.toThrow(); - }); - }); - describe('findComponentRoot', function() { it('should find the correct node with prefix sibling IDs', function() { var parentNode = document.createElement('div');