Attach scroll/wheel to root: alternative scroll browser locking fix #13460
Conversation
Scroll events need to be attached locally in order to receive rendering optimizations in Firefox, Edge, and Safari. Unfortunately, scroll events can be nested. When attached locally, events bubble up through both the DOM and React event systems. This commit adds a module that tracks what events have dispatched. When possible, it uses a WeakSet to test membership. When the event is fully dispatched, it should be released with GC. For browsers that do not support WeakSets, this module modifies the event with a flag. This needs to be tested in IE.
| trapCapturedEvent(dependency, root); | ||
| rootListeners[dependency] = true; | ||
| } | ||
| return; | ||
| case TOP_FOCUS: | ||
| case TOP_BLUR: |
There was a problem hiding this comment.
@gaearon you mentioned that touch events were essential (#9333 (comment)), would you like me to add them to the list here?
|
I was thinking that if we do this, we should do it for all events. And behind a feature flag (it will be a pain to deploy at FB and would need to be breaking change). |
| const dependencies = registrationNameDependencies[registrationName]; | ||
| for (let i = 0; i < dependencies.length; i++) { | ||
| const dependency = dependencies[i]; | ||
| if (!(isListening.hasOwnProperty(dependency) && isListening[dependency])) { |
There was a problem hiding this comment.
I'm going to revert this change. It isn't related to this work and should be done in a separate PR.
|
ReactDOM: size: 🔺+0.1%, gzip: 🔺+0.2% Details of bundled changes.Comparing: e106b8c...1361a81 react-dom
Generated by 🚫 dangerJS |
|
Cool. Doing that exposes a few failures. Particularly around mouse enter and leave because it needs to pick up movements outside of React. That might be fixable by attaching to the owner document only for those events. |
|
Note past attempt in #8117 and the uncovered difficulties. |
|
Yeah. This is going to take some planning. Maybe we can talk about it at the DOM team meeting. |
|
@nhunzaker I've tested Safari and Chrome and it seems that both browsers are still blocking at some point. Firefox seems good but I have not tried Edge. 🙂 Edit: Firefox also seems good when using 16.4.2. |
This is an alternative solution to #9333 that attaches scroll/wheel listeners to the root container element.
So far, this prevents browser scroll locking in Chrome, Edge and Firefox. It does not appear to fix locking on Safari. However I think I might be getting inaccurate readings in BrowserStack and would really appreciate some help testing.
Test Plan
Expected: No scroll locking
Browsers