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 389ca15
Showing 1 changed file with 1 addition and 3 deletions.
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

0 comments on commit 389ca15

Please sign in to comment.