Skip to content

Commit

Permalink
Attach event listeners at the root of the tree instead of document
Browse files Browse the repository at this point in the history
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 :)
  • Loading branch information
vjeux committed Oct 26, 2016
1 parent 2ef1208 commit 80dc96b
Show file tree
Hide file tree
Showing 3 changed files with 18,310 additions and 25 deletions.
27 changes: 5 additions & 22 deletions src/renderers/dom/shared/ReactBrowserEventEmitter.js
Original file line number Diff line number Diff line change
Expand Up @@ -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)) {
Expand Down Expand Up @@ -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];

Expand Down
4 changes: 1 addition & 3 deletions src/renderers/dom/stack/client/ReactDOMComponent.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
Loading

0 comments on commit 80dc96b

Please sign in to comment.