From 80dc96b1d6892f5ddfed8b2bdc534f4ba17a5d53 Mon Sep 17 00:00:00 2001 From: Christopher Chedeau Date: Wed, 26 Oct 2016 14:44:25 -0700 Subject: [PATCH] Attach event listeners at the root of the tree instead of document Context: we are investigating using React in order to build both Atom core components and Atom plugins. Work have been done last year to make sure that multiple React instances in the same app and this is working well. The only remaining issue is the fact that e.stopPropagation doesn't work as intended when there's two React trees that are nested. The reason is that both event listeners are added to the root of the tree and therefore do not cancel each others. The suggested solution is to attach event listeners at the root of the React tree. To understand how this solves the problem, let's assume that we have OuterComponent which is running React version A and InnerComponent that is running version 2. The inner component attaches the event listener at the top of the inner tree using bubble phase and the outer component at the root of the outer component. When there's a click event on the innerComponent, the inner version of React will be notified first because it's deeper in the tree, which will dispatch the event through the innerComponent hierarchy and eventually something will call React e.stopPropagation(), which will call the DOM e.stopPropagation(), so that the outer version of React will never be notified. Test Plan: - Have the changes of this revision - `yarn build` to generate react.js and react-dom.js - Copy react.js and react-dom.js into react2.js and react-dom2.js - Change the global assignation of React and ReactDOM inside of *2.js to React2 and ReactDOM2 - Build a test case with two nested components and the inner one calling e.stopPropagation - http://fooo.fr/~vjeux/fb/vjeux-test/test-event-boundary.html - Make sure it doesn't trigger the outer one. - Revert the changes and make sure that clicking on the inner one triggers both events to fire Important Note: I don't fully understand the implication of this changes around performance and potential side-effects that could happen. I'm going to spend time right now investigating this. Would love ideas on what to check :) --- .../dom/shared/ReactBrowserEventEmitter.js | 27 +- .../dom/stack/client/ReactDOMComponent.js | 4 +- vjeux-test/react-dom.js | 18304 ++++++++++++++++ 3 files changed, 18310 insertions(+), 25 deletions(-) create mode 100644 vjeux-test/react-dom.js diff --git a/src/renderers/dom/shared/ReactBrowserEventEmitter.js b/src/renderers/dom/shared/ReactBrowserEventEmitter.js index 0d6a3b7a46db7..ee984662cb82d 100644 --- a/src/renderers/dom/shared/ReactBrowserEventEmitter.js +++ b/src/renderers/dom/shared/ReactBrowserEventEmitter.js @@ -153,7 +153,7 @@ var topEventMapping = { */ var topListenersIDKey = '_reactListenersID' + String(Math.random()).slice(2); -function getListeningForDocument(mountAt) { +function getListeningForContainer(mountAt) { // In IE8, `mountAt` is a host object and doesn't have `hasOwnProperty` // directly. if (!Object.prototype.hasOwnProperty.call(mountAt, topListenersIDKey)) { @@ -214,29 +214,12 @@ var ReactBrowserEventEmitter = Object.assign({}, ReactEventEmitterMixin, { }, /** - * We listen for bubbled touch events on the document object. - * - * Firefox v8.01 (and possibly others) exhibited strange behavior when - * mounting `onmousemove` events at some node that was not the document - * element. The symptoms were that if your mouse is not moving over something - * contained within that mount point (for example on the background) the - * top-level listeners for `onmousemove` won't be called. However, if you - * register the `mousemove` on the document object, then it will of course - * catch all `mousemove`s. This along with iOS quirks, justifies restricting - * top-level listeners to the document object only, at least for these - * movement types of events and possibly all events. - * - * @see http://www.quirksmode.org/blog/archives/2010/09/click_event_del.html - * - * Also, `keyup`/`keypress`/`keydown` do not bubble to the window on IE, but - * they bubble to document. - * * @param {string} registrationName Name of listener (e.g. `onClick`). - * @param {object} contentDocumentHandle Document which owns the container + * @param {DOMElement} container element to attach the listener onto */ - listenTo: function(registrationName, contentDocumentHandle) { - var mountAt = contentDocumentHandle; - var isListening = getListeningForDocument(mountAt); + listenTo: function(registrationName, container) { + var mountAt = container; + var isListening = getListeningForContainer(mountAt); var dependencies = EventPluginRegistry.registrationNameDependencies[registrationName]; diff --git a/src/renderers/dom/stack/client/ReactDOMComponent.js b/src/renderers/dom/stack/client/ReactDOMComponent.js index 0da6954928477..f190801063ec0 100644 --- a/src/renderers/dom/stack/client/ReactDOMComponent.js +++ b/src/renderers/dom/stack/client/ReactDOMComponent.js @@ -217,9 +217,7 @@ function enqueuePutListener(inst, registrationName, listener, transaction) { 'This browser doesn\'t support the `onScroll` event' ); } - var containerInfo = inst._hostContainerInfo; - var isDocumentFragment = containerInfo._node && containerInfo._node.nodeType === DOC_FRAGMENT_TYPE; - var doc = isDocumentFragment ? containerInfo._node : containerInfo._ownerDocument; + var doc = inst._hostContainerInfo._node; listenTo(registrationName, doc); transaction.getReactMountReady().enqueue(putListener, { inst: inst, diff --git a/vjeux-test/react-dom.js b/vjeux-test/react-dom.js new file mode 100644 index 0000000000000..7af866eae739b --- /dev/null +++ b/vjeux-test/react-dom.js @@ -0,0 +1,18304 @@ + /** + * ReactDOM v16.0.0-alpha + */ + +;(function(f) { + // CommonJS + if (typeof exports === "object" && typeof module !== "undefined") { + f(require('react')); + + // RequireJS + } else if (typeof define === "function" && define.amd) { + require(['react'], f); + + //