Attach event listeners at the root of the tree instead of document

[vjeux]

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 :)

[vjeux]

Thanks @spicyj for telling me how to implement this! cc @sebmarkbage and @nathansobo

[sophiebits]

Can you rename things in ReactBrowserEventEmitter.listenTo to be called "container" not "document"?

I also noticed scroll always attaches to window when capturing isn't supported: https://github.com/facebook/react/blob/master/src/renderers/dom/shared/ReactBrowserEventEmitter.js#L283. Not sure if relevant.

[vjeux]

Can you rename things in ReactBrowserEventEmitter.listenTo to be called "container" not "document"?

Will do

[vjeux]

   * 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.

This comment is super scary. Firefox 8 isn't even tracked by our analytics. The last (#7th) version of Firefox we have is 38 at 0.10% of global market share, so there's likely no longer any meaningful 8.01 versions out there.

[sebmarkbage]

So I think mouseenter/mouseleave is busted in nested renders anyway.

class Foo extends React.Component {
  enter(){ console.log('enter foo'); }
  leave(){ console.log('leave foo'); }
  render() {
    return <div onMouseEnter={this.enter.bind(this)} onMouseLeave={this.leave.bind(this)}>Foo</div>;
  }
}

class Bar extends React.Component {
  enter(){ console.log('enter bar'); }
  leave(){ console.log('leave bar'); }
  componentDidMount() {
    ReactDOM.render(<Foo />, this.refs.c);
  }
  render() {
    return <div onMouseEnter={this.enter.bind(this)} onMouseLeave={this.leave.bind(this)}>Bar <div ref="c"/></div>;
  }
}

ReactDOM.render(<Bar />, mountNode);

This should not leave Bar when you enter Foo but it currently does and I don't think this changes that but we should confirm. Especially if it might fire double events in some case.

There is also a case where we assume that we moved from outside the window if we get a mouseover event.

https://github.com/facebook/react/blob/master/src/renderers/dom/shared/eventPlugins/EnterLeaveEventPlugin.js#L84

This is no longer true since we have moved from between the two roots. So this would be a breaking change.

You should also look out for duplicate events being fired in general in the nested case, when you don't stopPropagation.

I think what will happen is that we'll catch it on the inner one. That will then collect the target which is the inner one and walk back up to find all React components - which includes the outer root! Then we'll bubble up and catch it in the outer root. That will then to the same thing which will fire all events again for both roots.

[sophiebits]

Enter/leave not working in nested roots is a KP. @conartist6 wrote conartist6@c926378 but I haven't had a chance to review it and I think we can do something simpler than that to fix.

[vjeux]

Okay, so I ran my test plan with the same version of React and e.stopPropagation() doesn't prevent the outer one from triggering. It also doesn't without my changes anyway.

I don't yet understand why but this is going to be an issue if React gets deduped and both use the same React instance.

[sebmarkbage]

@vjeux This is the relevant code for why that is a problem:

react/src/renderers/dom/shared/ReactEventListener.js

Lines 60 to 72 in e3131c1

	var targetInst = ReactDOMComponentTree.getClosestInstanceFromNode(
	nativeEventTarget
	);
	// Loop through the hierarchy, in case there's any nested components.
	// It's important that we build the array of ancestors before calling any
	// event handlers, because event handlers can modify the DOM, leading to
	// inconsistencies with ReactMount's node cache. See #1105.
	var ancestor = targetInst;
	do {
	bookKeeping.ancestors.push(ancestor);
	ancestor = ancestor && findParent(ancestor);
	} while (ancestor);

EDIT: Maybe we are talking about two different problems.

[sebmarkbage]

It might be enough to simply not do that second traversal since nested React trees will be caught by the normal DOM event bubbling. EDIT: Actually, no that's not enough because getClosestInstanceFromNode will get the inner one twice. But something like it.

[vjeux]

@sebmarkbage removing that traversal up indeed fixes the issue I'm experiencing. I'm going to try and reproduce the original issue and see if it fixes it as well.

/*  var ancestor = targetInst;
  do {
    bookKeeping.ancestors.push(ancestor);
    ancestor = ancestor && findParent(ancestor);
  } while (ancestor);
*/
  bookKeeping.ancestors.push(targetInst);

[sebmarkbage]

@vjeux Fixing that issue seems like a good thing but would be breaking change so we might need to warn/track if someone relies on that. Should be doable.

The problem with just removing the second traversal, is that you won't get outer events when you do want them to bubble (no stopPropagation). Instead, you'll get duplicate inner events. When using the same instance of React.

[sebmarkbage]

Flagging as needing revision but I think this is a viable direction. We just need to fix the other callsites.

[conartist6]

@vjeux I don't quite understand what you're saying: "The reason is that both event listeners are added to the root of the tree and therefore do not cancel each others."

They issue with the handlers canceling on each other is not that it's impossible, right? The issue would be be that the root instance will have registered its handler first, and thus it will execute first before the child has a chance to cancel it.

Really it seems to me though like the code has been written with the basic intent that there will only ever be one top level handler for a particular event type regardless of the number of roots. That is, for example, no more than one document.addEventListener('click') call for any number of nested react roots which handle click events. On receipt of the event we iterate up through the element hierarchy to find roots - which get stored in bookkeeping.ancestors - then we trigger handling logic on each root from that list.

Can we not just write logic to ensure that this is actually what happens? The logic would be:

• A nested root, when registering a click handler, will cause the top level root to register a click handler with the document if it has not yet.
• A top level root will not unregister its DOM click handler unless neither it nor its nested roots are using it any longer.

I'd also add I'll say that as far as I can tell the code in my PR, while complicated, is the only completely correct way to handle enter/leave events to specification.

[vjeux]

@conartist6 thanks for chiming in! The constraint here is that I want to make it work for nested roots that can be implemented with different versions of React. So we cannot "register a click handler with the document if it has not yet" since we don't want the two React versions to communicate.

[fcsonline]

We are trying to solve the same issues in this PR:
#7088

[maxired]

As I user, I would be very happy for this feature to be available (maybe optionnaly).

I am using in an React application a non react component (https://github.com/ceolter/ag-grid), when then create some react component in a new React tree. The current implementation make events handling really difficult.

[sebmarkbage]

The way we handle bubbling of focus in Firefox is to use the capture phase.

react/src/renderers/dom/shared/ReactBrowserEventEmitter.js

Lines 289 to 313 in e3131c1

	if (isEventSupported('focus', true)) {
	ReactBrowserEventEmitter.ReactEventListener.trapCapturedEvent(
	'topFocus',
	'focus',
	mountAt
	);
	ReactBrowserEventEmitter.ReactEventListener.trapCapturedEvent(
	'topBlur',
	'blur',
	mountAt
	);
	} else if (isEventSupported('focusin')) {
	// IE has `focusin` and `focusout` events which bubble.
	// @see http://www.quirksmode.org/blog/archives/2008/04/delegating_the.html
	ReactBrowserEventEmitter.ReactEventListener.trapBubbledEvent(
	'topFocus',
	'focusin',
	mountAt
	);
	ReactBrowserEventEmitter.ReactEventListener.trapBubbledEvent(
	'topBlur',
	'focusout',
	mountAt
	);
	}

Technically, this means that if someone outside of React is trying to call stopPropagation you can get different browser behaviors. However, that's pretty rare.

With this change, the events of nested trees will fire in the reverse order in Firefox, because it uses the capture phase. Not sure how bad that will be.

Actually, I think we prefer the capture phase if available. So it will always be in reverse order, except in old-IE.

[conartist6]

@sebmarkbage According to the quirksmode article linked in that block we have it because IE8 doesn't support event capture. We don't support IE8 anymore, so why not just move all browsers over to capture focus and blur?

[sebmarkbage]

@conartist6 We still leave them in as a courtesy but it doesn't change the problem here. That the events happen in reverse order when capture is used.

[conartist6]

Oh I see, because the first block catches browsers that support focus. So the order will be: bubble in parent tree, then bubble in child child tree. Correct?

[sebmarkbage]

Correct.

[nhunzaker]

I think this means that the iOS fix for click handler bubbling on non-interactive elements can go away:

https://github.com/facebook/react/blob/master/src/renderers/dom/shared/eventPlugins/SimpleEventPlugin.js#L312-L341

Here's my test case anyway:
https://gist.github.com/nhunzaker/358f3cf11d96133b5cb5db764e3f89b5

This is drawn from PPK' original test:
http://www.quirksmode.org/blog/archives/2010/09/click_event_del.html

And the change:
https://github.com/vjeux/react/compare/attach_events_to_container_root...nhunzaker:nh-remove-simple-event-ios-fix?expand=1

[conartist6]

A thought on focus and blur. What if instead of relying on the native browser focus and blur captured events in order to convey the information safely between react roots we trigger a surrogate custom event which bubbles and which individual React roots will then use as a cue to trigger a focus/blur event.

Also what happens when you have react version A nested in version B and A and B are on opposite sides of this changeset?

[sophiebits]

@nhunzaker It is still necessary or else the gray highlight would be over the entire container rather than just the clickable elements. In fact, we should make sure that doesn't happen here.

[nhunzaker]

@spicyj Drats. Taught me something new today, anyway.

[sophiebits]

SelectEventPlugin will also probably break with this change; we need to fix it before landing.

[acusti]

As long as we’re diving into the React dom event plugin world, any chance of getting some eyes on #7936? Also, do you anticipate possible conflicts between these tangentially related efforts?

[gaearon]

What is the consensus on this?

[sebmarkbage]

@gaearon The consensus is that we want to do it but it requires some work. We've identified a few related techniques and plugins that would be broken by a direct change. Probably needs to be a bigger change than first expected.

Specifically one issue is that the SelectEventPlugin assumes single document level listeners.

[nathansobo]

We're still really looking forward to this on the Atom team so we can feel more confident recommending React for packages. As it stands, the potential for unexpected event bubbling order when multiple instances of React are in play makes us pretty nervous.

[sebmarkbage]

Changes requested above.

[gaearon]

Closing as stale. Tracking in #2043.

[gaearon]

React 17 does this.
https://reactjs.org/blog/2020/08/10/react-v17-rc.html

[Knove]

Tracking