Fix wheel/touch browser locking in IE and Safari #9333

nhunzaker · 2017-04-05T00:05:33Z

In order for Firefox, Edge, and Safari to optimize scroll rendering, scroll events must be attached locally.

The React synthetic event system isn't really built to handle this, attaching events locally causes a dispatch to the React Synthetic event system, which is problematic for nested scroll event handlers. To work around that, this PR adds a deduping mechanism that track what native events have already dispatched, bailing early if they have already been processed

Testing

I have forked @nolanlawson's wheel jank example and built it with this branch's version of React. You can view the difference by doing the following:

Visit http://nh-react-scroll-fix.surge.sh in IE Edge or Safari
Using a wheel, or two-finger scroll, scroll outer container of the page for ~10 seconds.
Your scrolling should never be interrupted (though in Safari you will see the scroll bar lock up, the page still scrolls)

Related issues: #1254 (comment)

nhunzaker · 2017-04-05T00:09:30Z

src/renderers/dom/shared/ReactBrowserEventEmitter.js

@@ -151,6 +152,39 @@ var topEventMapping = {
  topWheel: 'wheel',


This object is the exact same as EventConstants, sans the values. Has there been any thought on consolidating them into EventConstants?

Yes, please send a follow-up to consolidate these! Would be nice to have 'Browser' or 'DOM' in the name too.

nhunzaker · 2017-04-05T00:11:48Z

src/renderers/dom/stack/client/ReactDOMComponent.js

@@ -948,6 +833,7 @@ ReactDOMComponent.Mixin = {
        DOMPropertyOperations.deleteValueForProperty(getNode(this), propKey);
      }
    }
+


I'm going to kill some of this whitespace noise.

nhunzaker · 2017-04-06T02:32:46Z

Talked through how to deal with invalid nesting with @spicyj. I'm sure there's some extra work here, but I think it's in a good spot to review.

sophiebits

I think this is good overall. A couple inlines.

Are we able to add touch events too? They can block scrolling on touch devices. We should be careful around the behavior for touch events on a node that gets removed though. Currently I think you lose move/end events if a node is removed while touching. We might even want to change this but should be conscious of it. Maybe best in a separate PR.

It used to be that we needed to remove listeners when a node is unmounted (a fix ages ago was in c62c2c5). I believe this is no longer necessary since we no longer track nodes by hierarchical IDs but I'm mentioning it for completeness and posterity.

sophiebits · 2017-04-06T23:35:45Z

src/renderers/dom/shared/ReactBrowserEventEmitter.js

@@ -151,6 +152,39 @@ var topEventMapping = {
  topWheel: 'wheel',


Yes, please send a follow-up to consolidate these! Would be nice to have 'Browser' or 'DOM' in the name too.

sophiebits · 2017-04-07T00:08:06Z

src/renderers/dom/shared/ReactBrowserEventEmitter.js

@@ -224,7 +258,7 @@ var ReactBrowserEventEmitter = Object.assign({}, ReactEventEmitterMixin, {
   * @param {string} registrationName Name of listener (e.g. `onClick`).
   * @param {object} contentDocumentHandle Document which owns the container
   */
-  listenTo: function(registrationName, contentDocumentHandle) {
+  listenTo: function(registrationName, contentDocumentHandle, node) {
    var mountAt = contentDocumentHandle;
    var isListening = getListeningForDocument(mountAt);


Can we add a 'Doc' somewhere in this name for clarity?

nhunzaker · 2017-04-07T12:28:15Z

src/renderers/dom/shared/ReactBrowserEventEmitter.js

+  topTouchMove: true,
+  topTouchStart: true,
+  topTouchCancel: true,
+  topTouchEnd: true,


@spicyj I've added touch cancel and touch end to the list here ☝️. They'll get attached locally now.

nhunzaker · 2017-04-07T12:33:43Z

Having some trouble npm linking the new flat bundled builds to the test case, but I'll get an update on http://nh-react-scroll-fix.surge.sh when possible.

gaearon · 2017-04-18T14:53:57Z

I tried to fix the conflict with master, let's see if CI passes.
What's the next actionable item here?

nhunzaker · 2017-04-19T14:02:09Z

I'll work on getting CI to pass later today. @spicyj I've pushed up some stuff to address your feedback, and I have a PR nearly done which consolidates the event names. It just needs to be rebased with master.

Beyond that, I wonder if it is a good idea to add some DOM fixtures for media events.

gaearon · 2017-04-21T13:17:28Z

@spicyj Could you take over rebasing this PR so that @nhunzaker can continue work on #7311 and not worry about this one?

This is a follow up to facebook#9333. This commit removes some duplication of event names and renames the EventConstants module to BrowserEventConstants.

nhunzaker · 2017-04-25T18:12:39Z

Upstreamed this with my other PR that consolidated the event list.

nhunzaker · 2018-09-05T19:20:09Z

@philipp-spiess Ah good deal. I can just make them attach with bubbling. We only need capture for scroll/wheel.

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.

Prior work included an event listener on the root container that would release events from tracking. This is not necessary. Instead, we can track the last currentTarget to see if the dispatch should be counted. This appears to work for both bubbling and capture. Assuming the following markup: ```html <div id="top" onScroll={...}> <div id="middle" onScroll={...}> <div id="bottom" onScroll={...} /> </div> </div> ``` Assume all levels have a local scroll listener. If we dispatch from the bottom: 1. The first target is the "top", because we assign it as captured. Capture works in reverse. 2. "middle" is skipped because the event is tracked to "top" 3. "bottom" is skipped because the event is trakced to "top" This means that only one synthetic event dispatch occurs once.

nhunzaker · 2018-09-05T20:12:16Z

@philipp-spiess moved touch events to bubbling and I added a test suite that verifies the bubble order on the react end (it's the same between capture and bubble, but using capture would definitely break existing expectations with events outside of react)

e7884fc

Are these valuable tests? It was useful for me to work through them, but I'm not sure if they give us any extra coverage.

philipp-spiess · 2018-09-05T22:03:29Z

Are these valuable tests? It was useful for me to work through them, but I'm not sure if they give us any extra coverage.

I think it makes sense. If we plan on handling touch events separately, we should at least make sure they still behave similarly.

nhunzaker · 2018-09-05T23:57:13Z

Supporting bubbling on touch added an edge case where re-dispatching the same event below the last dispatch ignores the second dispatch incorrectly. I've pushed up that failing case.

I'm about out of ideas, but inspiration might strike.

stale · 2020-01-10T08:52:36Z

This pull request 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.

stale · 2020-01-17T08:59:01Z

Closing this pull request after a prolonged period of inactivity. If this issue is still present in the latest release, please ask for this pull request to be reopened. Thank you!

facebook-github-bot added the CLA Signed label Apr 5, 2017

nhunzaker mentioned this pull request Apr 5, 2017

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

Closed

nhunzaker commented Apr 5, 2017

View reviewed changes

nhunzaker force-pushed the nh-scroll-fix branch from 0aafd7a to cfd21eb Compare April 5, 2017 00:13

aweary self-assigned this Apr 5, 2017

aweary requested a review from sebmarkbage April 5, 2017 01:18

nhunzaker force-pushed the nh-scroll-fix branch 3 times, most recently from d26fc06 to e61ddae Compare April 6, 2017 02:15

sophiebits reviewed Apr 7, 2017

View reviewed changes

nhunzaker force-pushed the nh-scroll-fix branch from 525bc46 to c61c9a5 Compare April 7, 2017 12:27

nhunzaker commented Apr 7, 2017

View reviewed changes

nhunzaker force-pushed the nh-scroll-fix branch 2 times, most recently from 8911065 to a655b5b Compare April 7, 2017 12:31

nhunzaker mentioned this pull request Apr 21, 2017

Implement Setting Unknown Attributes #9477

Closed

nhunzaker mentioned this pull request Apr 24, 2017

Consolidate events between EventConstants and BrowserEventEmitter #9512

Merged

nhunzaker force-pushed the nh-scroll-fix branch from d338a43 to 1ad7e85 Compare April 25, 2017 17:06

nhunzaker force-pushed the nh-scroll-fix branch from 1ad7e85 to 795ce86 Compare May 5, 2017 21:15

nhunzaker added 15 commits September 5, 2018 12:25

Locally attach wheel, scroll and touch events

267da68

Add test coverage for doubly nested event handlers

9b034e7

Attach scroll and wheel events to owner

f81f8db

Bubble scroll/wheel for consistency. Capturing does not matter

92fb9d8

Check event, not tracker

c6216e9

Revert flow-type change

e3e4eb5

Release events from tracking when they bubble to the root

079474d

Fix flow

adb0992

Add WeakSet to eslint config

310b87b

Add additional test cases from commit

635854d

Make the linter happy

4967ed1

Eliminate duplicate local listener tests in ReactDOMComponent-test

ebf431b

Touch events should be bubbled

ec4c2e2

nhunzaker force-pushed the nh-scroll-fix branch from d8aadd3 to d8b88fa Compare September 5, 2018 20:08

Attach touch events locally, add tests to ensure bubble order

e7884fc

nhunzaker force-pushed the nh-scroll-fix branch from d8b88fa to e7884fc Compare September 5, 2018 20:10

Add additional test cases and restructure tests

62ce2ff

nhunzaker mentioned this pull request Sep 11, 2018

Do not bind topLevelType to dispatch #13618

Merged

necolas mentioned this pull request May 28, 2019

[Umbrella] React Flare #15257

Closed

trueadm mentioned this pull request Jun 5, 2019

[Flare] Add basic Scroll event responder module #15827

Merged

stale bot added the Resolution: Stale Automatically closed due to inactivity label Jan 10, 2020

stale bot closed this Jan 17, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix wheel/touch browser locking in IE and Safari #9333

Fix wheel/touch browser locking in IE and Safari #9333

nhunzaker commented Apr 5, 2017 •

edited

Loading

nhunzaker Apr 5, 2017

sophiebits Apr 6, 2017

nhunzaker Apr 7, 2017

nhunzaker Apr 5, 2017

nhunzaker commented Apr 6, 2017

sophiebits left a comment

sophiebits Apr 6, 2017

sophiebits Apr 7, 2017

nhunzaker Apr 7, 2017

nhunzaker Apr 7, 2017

nhunzaker commented Apr 7, 2017

gaearon commented Apr 18, 2017

nhunzaker commented Apr 19, 2017

gaearon commented Apr 21, 2017

nhunzaker commented Apr 25, 2017

nhunzaker commented Sep 5, 2018

nhunzaker commented Sep 5, 2018

philipp-spiess commented Sep 5, 2018

nhunzaker commented Sep 5, 2018

stale bot commented Jan 10, 2020

stale bot commented Jan 17, 2020

		@@ -151,6 +152,39 @@ var topEventMapping = {
		topWheel: 'wheel',

Fix wheel/touch browser locking in IE and Safari #9333

Fix wheel/touch browser locking in IE and Safari #9333

Conversation

nhunzaker commented Apr 5, 2017 • edited Loading

Testing

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nhunzaker commented Apr 6, 2017

sophiebits left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nhunzaker commented Apr 7, 2017

gaearon commented Apr 18, 2017

nhunzaker commented Apr 19, 2017

gaearon commented Apr 21, 2017

nhunzaker commented Apr 25, 2017

nhunzaker commented Sep 5, 2018

nhunzaker commented Sep 5, 2018

philipp-spiess commented Sep 5, 2018

nhunzaker commented Sep 5, 2018

stale bot commented Jan 10, 2020

stale bot commented Jan 17, 2020

nhunzaker commented Apr 5, 2017 •

edited

Loading