React: React shouldn't bind wheel or touch events to the document.

[joshduck]

React binds touchmove, touchstart, touchend, touchcancel and wheel handlers to the document. When the user tries to scroll the browser needs to execute these event handlers to ensure event.preventDefault() was not called. This means scrolling will stall while JavaScript is executing.

Chrome has a fast-path that checks whether the coordinate where the touch event happens has touch listeners (https://plus.google.com/+RickByers/posts/cmzrtyBYPQc). If there are no listeners Chrome can allow the scroll to happen even if the main thread is blocked on JavaScript.

We should bind our listeners for these events directly to the node which requires it. That event handler can then dispatch the event to the standard React top level event system. Then it will bubble/capture just like everything else and be visible to all event plugins.

[sophiebits]

This would also definitely fix our initializeTouchEvents problem. I don't know the perf impacts of not bubbling to document though.

[zpao]

[SanderSpies]

Anyone picking this one up? Otherwise, I would like to give it a go :-).

[possibilities]

It would also fix the problem I'm having here:

http://jsfiddle.net/BSJPd/1/

Arguably not a bug but unexpected and undesirable for me.

[petehunt]

@SanderSpies you're exactly who I had in mind for this! :)

[SanderSpies]

petehunt: great :)

Potential scenario's I'm seeing:

• elements with listeners at the same level.

So for these event types there will be more then one event listener

• child element listens to same event as parent element.

I guess we always want to use the parent element

• during lifetime the dom changes, and therefore also the element which needs to listen.

Which would imply we would also need to remove/add event listeners on the fly, not sure if we already do that

I'm thinking of adding something like 'bindLocally' at the event plugins level like:

onTouchMove: {
bindLocally: true,
phasedRegistrationNames: {
bubbled: keyOf({onDragOver: true}),
captured: keyOf({onDragOverCapture: true})
}
}

Good idea? Bad idea? Missing stuff here?

[sophiebits]

It's really a function of the topLevelType topTouchMove, not the event that you wrote there. For naming, delegate: false might be clearer.

Right now I don't believe we ever remove listeners.

[SanderSpies]

@spicyj I wrote that code a bit too hastily I guess. Should have been touchMove, also notice the onDragOver + onDragOverCapture that I forgot to correct...

[SanderSpies]

Currently the event listeners are attached before React inserts the nodes into the DOM. So I was wondering how I should handle this.

I was thinking of letting ReactEventEmitter.listenTo create a 'bag' for local events, that is later used by a new function (addLocalEventListeners?) which is called by ReactComponent._mountComponentIntoNode.

Thoughts on this?

[sophiebits]

Are you sure it's before the DOM nodes are created? Regardless, probably you want to tie it to the transaction -- you can see how ReactReconcileTransaction stores a queue of componentDidMount callbacks (ON_DOM_READY_QUEUEING/ReactMountReady) and a queue of listeners to put (PUT_LISTENER_QUEUEING/ReactPutListenerQueue). If the put listener queue can't easily be modified to do what you want (though I'm guessing it can), you can make a new transaction wrapper and add it to the list there.

[u9520107]

Is this still being investigated?
In my project, I have a component that uses scrollable div's with roughly 300 direct child, which nests maybe 6 to 7 children. While putting translateZ(0) to force composition layer seems to be able to boost scroll performance to 60 fps. As soon as the document wheel event is attached, the performance is crippled. Even after the component that relies on wheel events is dismounted, the wheel listener on document would live on, reducing the scroll performance to around 30-40 fps.

[nolanlawson]

This problem was also noticed by former Edge/IE engineer Justin Rogers, who now works with y'all at Facebook. :smiley: React's technique of adding a global wheel listener on the document defeats optimizations that certain browsers (notably Safari and Edge) have implemented to avoid jank that only needs to occur for inner elements, not for the entire document.

To demonstrate, I put together a little sample application based on create-react-app. There's a setInterval() in there that janks the main thread for 1000 milliseconds every 2000 milliseconds. This is to simulate a very active page with lots of JS. _(Update: bumped it to 3000/4000 to make it more obvious.)_

There are three versions of the app: 1) no wheel events, 2) a React onWheel event on the inner scrollable div (which adds a global wheel event to the document), and 3) a regular DOM wheel event on the inner scrollable div. In all cases, the listener itself is just lodash.noop.

In Firefox and Chrome, there is no difference between (2) and (3). However, in Edge and Safari, React's technique of adding the global wheel listener has the unfortunate side-effect of causing the entire page to jank while scrolling, rather than just when you're scrolling the inner div.

Note that this optimization only applies to certain kinds of scroll events like mousewheel scrolling, not necessarily two-finger touchpad scrolling, clicking-and-dragging the scrollbar, or keyboard-scrolling. So you may need to use a real mouse to reproduce. It also may apply to scroll, touchstart, touchmove, etc., but I only tested wheel, since the others have their own performance characteristics and may only be impacted on touchscreen devices. It's worth investigating, but for this demo I focused on wheel.

It's kind of a subtle issue, so I also made a quick video demonstration: https://youtu.be/6Ckepx3wPPE

[nolanlawson]

Update: added onTouchStart/touchstart to the demo as well, and confirmed that this causes unnecessarily janky touch-scrolling in all mobile browsers (iOS Safari, Android Chrome, Android Firefox, Edge Mobile).

I compiled the results into a table for those who want to see a browser-by-browser breakdown.

[gaearon]

We just published a code dive into React event system (thanks @spicyj and @kentcdodds!)
This would be helpful to anyone who’d like to dig into this.

[nhunzaker]

Just read this article and found my way here. I've got some bandwidth now that the number input issue is more or less wrapped up.

Is anyone working on this? Anything I should keep in mind before I get started?

[nhunzaker]

Working on a fix here:
https://github.com/facebook/react/compare/master...nhunzaker:nh-scroll-fix

This change allows ReactBrowserEventEmitter, which is responsible for attaching event listeners, to make the decision on a case-by-case basis as to what events should be attached to the document vs the element itself.

I still haven't gotten into touch events or serious wheel testing, but I can confirm that it gets rid of the window scroll jank in @nolanlawson v2 example

Time to find a mouse with a wheel!

[sebmarkbage]

@nhunzaker We should chat about this strategy. There are several considerations here.

1) We want to minimize the amount of work we do during initial render which includes minimizing the checks we do on events. Currently we do way too much work just to figure out that we've already attached an event.

2) We noticed in https://github.com/facebook/react/pull/8117 that moving event listeners locally had many unexpected consequences on other parts of the system so it was harder than it initially seemed. E.g. it can break how the select event plugin works. We need to make sure that all that stuff plays nicely together.

[nhunzaker]

Absolutely. Is reactiflux, the discord room, best? Really any medium is fine.

Re 1: Are there any known bottlenecks that you could point me to?

Re 2: Totally, and hard to test. It would be good to get some DOM fixtures in place as we uncover issues. That was going to be my next step here.

[sebmarkbage]

Probably best to start here. I just wanted to highlight those issue so you're aware.

1) ensureListeningTo and these steps in ReactBrowserEventEmitter are currently executed for every event listener in the entire page. That's several slowish DOM operations, map lookups, hasOwnProperties and property look ups PER every listener before we can show the first screen. In theory, if we attached listeners to every event at the document eagerly, we wouldn't have to do any of that for any event listener.

2) I think wheel specifically is probably pretty safe but I believe at least one platform it might be possible to change the value in a <select /> with the wheel which might or might not be related.

[nhunzaker]

@sebmarkbage I recently had a few PRs merged that remove some of the work in BrowserEventEmitter doing support checks we don't need. Next I'm going to see what kind of savings we actually get by just assigning an event listener for every supported event.

Without changing things too much, I want to figure out what's slow, eliminate that work, then evaluate reworking ReactBrowserEventEmitter with less moving parts (and I better understand what's happening).

I'm also curious if we can reduce the number of calls to ensureListeningTo for special cases in ReactDomComponent

Is that a reasonable approach?

[nolanlawson]

Restricting the set of "prefer local listeners" to wheel, touchstart, and touchmove is probably a good start. All of those events can block scrolling in Edge/Safari/etc. but only if done globally.

FWIW, in case you don't want to go to the trouble of buying a mouse with a wheel, I've found that two-fingered scrolling on a touchpad usually exhibits identical behavior.

[nhunzaker]

@nolanlawson Thank you for the guidance! I have a mouse with a wheel (thank goodness for conference swag), but I wasn't sure about two-fingered scrolling, that's really good to know.

@sebmarkbage The only hiccup I'm hitting now is that to-string rendering (at least running tests against the feature flag: useCreateElement: false throws an error because the node to attach the listener for doesn't exist yet.

ensureListeningTo, which runs to make sure a top-level event listener is attached to the document, looks something like this:

function ensureListeningTo(inst, registrationName, transaction) {
  if (transaction instanceof ReactServerRenderingTransaction) {
    return;
  }

  listenTo(registrationName, getDocument(inst), getNode(inst));
}

The problem is that getNode(inst) fails some times when the useCreateElement feature flag is set to false. You see this warning:

Invariant Violation: React DOM tree root should always have a node reference.

So I had a few questions/ideas:

1. Is setting useCreateElement: false still the best way to test server-side rendering?
2. What about doing enqueuing event listener attachment post mount, like:

transaction.getReactMountReady().enqueue(attachListeners, this);

If we did this, what do you think about moving all of the local listener logic out of ReactDOMComponent and into ReactBrowserEventEmitter? For example, this section here:

https://github.com/facebook/react/blob/master/src/renderers/dom/stack/client/ReactDOMComponent.js#L254-L310

Then, possibly, we could eliminate the unmount work here:

https://github.com/facebook/react/blob/master/src/renderers/dom/stack/client/ReactDOMComponent.js#L1107-L1159

[sebmarkbage]

useCreateElement: false is what happens when you render into a server rendered tree on the client. We're in need of imminently rewriting that whole algorithm and likely need to switch strategies for Fiber anyway.

[nhunzaker]

I took another pass:
https://github.com/facebook/react/compare/master...nhunzaker:nh-scroll-fix

This diff further moves event listener location handling to BrowserEventEmitter. Here's how it works:

1. createElement:false runs a post-mount hook to attach all listeners
2. _updateDOMProperties runs BrowserEventEmitter.listenTo(eventName, document, element), more or less like normal.
3. BrowserEventEmitter.listenTo is responsible for figuring out if a listener should be attached locally or globally.

No fiber tests fail! But we probably have a few more tests to write.

What _does_ fail is createElement:false in the stack renderer: Because attaching event listeners to invalid nested markup means ReactDOMComponentTree can't find them.

For example:

<div>
  <tr></tr>
  <tr></tr>
</div>

When ReactDOMComponentTree goes to precache the nodes, the <tr> tags can't be found in the div, they seem to get stripped out. This causes the following validation error:

https://github.com/facebook/react/blob/master/src/renderers/dom/shared/ReactDOMComponentTree.js#L115

ReactDOMComponent › nesting validation › warns nicely for table rows
Invariant Violation: Unable to find element with ID 2.

So I'm looking for a good check to run to see if the node associated with a ReactDOMComponent instance is actually mountable. This is further complicated by the <source> tag inside a <video> element. Most of my attempts either fail this test:

https://github.com/facebook/react/blob/master/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js#L759-L779

Or this family of tests:
https://github.com/facebook/react/blob/master/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js#L1263-L1277

Or

Do we even need to do listener attachment if createElement is set to false?

[gaearon]

Do we even need to do listener attachment if createElement is set to false?

No, createElement: false is only used for validating server markup on the client. Its result is thrown away.

[nhunzaker]

👍 . That solves my problem then

[nhunzaker]

Okay. PR soon. here's a test page:
http://nh-react-scroll-fix.surge.sh

Associated code:
https://github.com/nhunzaker/react-wheel-jank-demo

(Thanks @nolanlawson!)

I believe this fixes the issues with wheel, touchstart, and touchmove events. But I need to test touchstart and touchmove on Edge. That's outside of my capabilities. Is this something someone else could take up?

Beyond that, generally, would others be able to help verify?

[sophiebits]

No, createElement: false is only used for validating server markup on the client. Its result is thrown away.

Wait, I don't think that's true. We use it only when rendering on top of server markup, but when called on the client it does need to attach event handlers! Like Seb said though, server rendering attaching code needs to get rewritten soon for Fiber so we're not sure what will happen.

[nhunzaker]

@spicyj Well shoot... okay. Any ideas? What is the best way to handle invalidly nested tags that never get created, but React is still tracking?

Would this discussion be better in https://github.com/facebook/react/pull/9333? Or do you want me to close that out and keep talking here?

[sophiebits]

Sorry, I haven't been following this whole thread – I just saw the one comment and it seemed wrong. In the div > tr example, why are we doing anything with event listeners? I must be missing something because I would have assumed we skip all the events code for that test.

We do have a public Messenger room at https://m.me/g/Abas8IcWxIvjVxHN which everyone is welcome to participate in. I'm usually responsive in close to real time and happy to answer questions.

[nhunzaker]

Perfect, I'll hop in.

[gaearon]

We use it only when rendering on top of server markup, but when called on the client it does need to attach event handlers!

Oops, you’re right, thanks for correcting me! Still learning 😄 Sorry @nhunzaker for confusing you.

[philipp-spiess]

Mentioning #2043 here as "probably related". 🙂

[Nandez89]

This might be related to https://github.com/facebook/react/issues/14856 or become more important since that chrome change.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contribution.

[gaearon]

This has been implemented and will go out in React 17.