Add infrastructure for passive/non-passive event support for future API exploration #15036

trueadm · 2019-03-06T12:49:51Z

Summary

This PR makes a bunch of changes to the underlying implementation for how ReactDOM listens to events on the DOM. Notably, it opens up the possibility of exploring the proper handling of passive/non-passive event listeners. As to not break existing event listeners, they now fall under "legacy", where they work 100% as they do now without any changes in control flow. Also some old code has been tidied up and Flow annotations have been added where they were previously missing.

Why do we need this?

For future event API exploration we need to properly hook into controlling how event listeners are registered with respect to if they are passive or non-passive. The way ReactDOM deals with events now is to not specify if an event is passive or non-passive. By doing so, we let the browser determine the behaviour behind the scenes. Common cases like when touchmove is registered by React to the document mean browsers actually make the events passive behind the scenes. This is the ideal case, but in the future we also need ways of controlling this behaviour for future event API work.

Validate this works

This PR is a part of a larger part of work which has implementations and tests that validate that the passive/non-passive logic works. In the interests of keeping this PR slim, I've intentionally omitted that code. In further follow up PRs, I'll add that logic to validate the behaviour of this PR. I know this isn't the most ideal case, but it was the best approach I could come up with.

Polyfills

In the future, if we decide to ship an API that makes use of passive events, support for addEventListener and specifically the third argument on addEventListener where the argument is an object needs to supported. There is a polyfill we could advise people to use or we could try and check for this in DEV and warn. I'm not fully sure on the path we should take yet - maybe it's something we should consider in the future once we're ready to ship said features.

…vent API experimentation

philipp-spiess

This is great 🎉 I can't wait to see how we gonna use the new implementation.

The code path for legacy listeners looks pretty much unchanged expect that we now require WeakMap instead of using the expando.

Do we know which browsers we want to support in the new implementation? Based on that we might need a few feature tests.

packages/react-dom/src/events/EventListener.js

packages/react-dom/src/events/ReactBrowserEventEmitter.js

packages/react-dom/src/events/ReactDOMEventListener.js

packages/react-dom/src/events/ReactBrowserEventEmitter.js

nhunzaker

Awesome to see this work happening, 👍

trueadm · 2019-03-13T15:56:36Z

As per the request of @gaearon, I've run a bunch of performance tests on this PR vs master. I slightly optimized the approach and now I see a slight performance gain in Chrome and Safari. No noticeable differences in Edge and Firefox.

I'd really appreciate it if we could get this PR landed soon.

nhunzaker · 2019-03-13T16:21:20Z

Where did you land on feature checking event listener options objects? Adding a feature detection like the MDN recommendation seems fine and with minimal impact.

Also if passive events won't be publicly exposed for a while maybe it could be kicked down the road a bit.

Does this need additional testing? I'd be happy to give it a run across browsers.

trueadm · 2019-03-13T16:58:01Z

@nhunzaker I added that support just now. It would be awesome if you could do some manual tests for it too :)

packages/react-dom/src/events/BeforeInputEventPlugin.js

packages/events/EventPluginHub.js

packages/events/ResponderEventPlugin.js

packages/react-dom/src/events/ReactBrowserEventEmitter.js

packages/react-dom/src/events/ReactDOMEventListener.js

packages/react-dom/src/events/SimpleEventPlugin.js

trueadm · 2019-03-13T20:35:31Z

I've updated the PR. I've removed the confusing null | boolean logic for passive and instead went with passing through an enum, which shows intent better. I've also addressed the PR feedback. There are no actors using the new passive/non-passive event system yet, but that will come soon with the experimental event API proposal I have brewing (which also doesn't require the event plugin system too).

philipp-spiess

Nice! The enum helped to clear up my last questions as well.

threepointone

this lgtm. I don't have context around some of it, but the changes look fine. bugging @trueadm for a walkthrough today.

gaearon · 2019-03-14T16:42:24Z

packages/react-dom/src/client/ReactDOMComponent.js

      props = rawProps;
      break;
    case 'input':
      ReactDOMInputInitWrapperState(domElement, rawProps);
      props = ReactDOMInputGetHostProps(domElement, rawProps);
-      trapBubbledEvent(TOP_INVALID, domElement);
+      trapBubbledEvent(TOP_INVALID, domElement, true);


Per our discussion, it seems like these code paths will never "want" to get two listeners attached/invoked. So instead of branching deeply and passing an argument through, let's fork the innermost implementation and call the fork from the new code when we want to. That will also likely let us DCE more based on a feature flag.

sebmarkbage

I'm still reviewing the implementation details but can we wrap some of the forked branches in a featureFlag before landing? A small increase is expected but this is increasing a little more than I'd expect for a flag that's turned off. Presumably it'll keep growing as you add more requirements/features so good to make sure the boundaries are clear from the beginning.

trueadm · 2019-03-14T19:37:57Z

@gaearon @sebmarkbage I've re-written the implementation based on feedback you both gave me. It now uses bitwise flags to express the intention of the event system. Furthermore, everything should almost now get DCE due to enableEventAPI. Thanks!

trueadm · 2019-03-14T19:54:23Z

Nice, this actually makes ReactDOM 0.1% smaller :P

sebmarkbage · 2019-03-15T01:01:21Z

packages/react-dom/src/events/forks/EventListener-www.js

 ): void {
-  EventListenerWWW.listen(element, eventType, listener);
+  EventListenerWWW.listen(element, eventType, listener, passive);


This doesn't seem right. The internal EventListener module doesn't accept a 4th argument (and Event.listen accepts a forth argument which is a priority and won't work with this). What's your plan here?

I’ll update the www version

sebmarkbage

Seems fine to me. Just make sure it works with www before you land so we can sync.

sebmarkbage · 2019-03-22T04:34:40Z

packages/react-dom/src/events/ReactDOMEventListener.js

+    // supported too. This way the responder event plugins know,
+    // and can provide polyfills if needed.
+    if (passive) {
+      if (passiveBrowserEventsSupported) {


This seems like the wrong check to me. Shouldn't this case be inverted? This will mark it as active and PASSIVE_NOT_SUPPORTED only if passive events are supported.

Well spotted!

NE-SmallTown · 2019-04-02T09:37:30Z

packages/events/EventSystemFlags.js

+export const RESPONDER_EVENT_SYSTEM = 1 << 1;
+export const IS_PASSIVE = 1 << 2;
+export const IS_ACTIVE = 1 << 3;
+export const PASSIVE_NOT_SUPPORTED = 1 << 4;


why we don't use literal number(and add comment) here just like maxSigned31BitInt?

These are used to do bitwise operations on, as there can be a combination of the above being used.

I think the question was why 1 << 4 is used over 16. 1 << 4 makes the intent clearer. maxSigned31BitInt "solves" this by adding a comment. I don't think it matters what pattern is used.

yea, just like @eps1lon said

This pattern is clear, as we may add more in the future, which is simply 1 << 5, 1 << 6 etc. We use this approach throughout the React codebase and we've verified that Google Closure Compiler simplifies the constant value.

Add infrastructure for passive/non-passive event support for future e…

858608b

…vent API experimentation

trueadm requested review from threepointone, bvaughn, sebmarkbage, philipp-spiess, gaearon and aweary March 6, 2019 12:49

facebook-github-bot added the CLA Signed label Mar 6, 2019

More Flow fixes

0ae3ba3

trueadm requested a review from sophiebits March 6, 2019 16:06

philipp-spiess reviewed Mar 6, 2019

View reviewed changes

trueadm added 3 commits March 6, 2019 16:35

Add fallback to Map if there is no WeakMap

3bd6176

Fix prettier

06f2c3b

Update comment

bbae02d

philipp-spiess reviewed Mar 6, 2019

View reviewed changes

packages/react-dom/src/events/ReactBrowserEventEmitter.js Outdated Show resolved Hide resolved

nhunzaker reviewed Mar 8, 2019

View reviewed changes

packages/react-dom/src/events/ReactBrowserEventEmitter.js Show resolved Hide resolved

nhunzaker reviewed Mar 8, 2019

View reviewed changes

packages/react-dom/src/events/ReactBrowserEventEmitter.js Outdated Show resolved Hide resolved

nhunzaker reviewed Mar 8, 2019

View reviewed changes

Add isLegacy flag to EventPlugins

68934d8

Handle fallback for no passive support

37224ea

Fixed typo + improve code size

c683859

nhunzaker reviewed Mar 13, 2019

View reviewed changes

packages/react-dom/src/events/BeforeInputEventPlugin.js Outdated Show resolved Hide resolved

gaearon reviewed Mar 13, 2019

View reviewed changes

trueadm added 2 commits March 13, 2019 20:01

Changed to enum for passive event types and addressed PR feedback

e7e742f

Added comment to describe intent

ccf3884

philipp-spiess approved these changes Mar 13, 2019

View reviewed changes

trueadm added 4 commits March 13, 2019 22:40

use canUseDOM

c9fc178

Cleaned up some logic and reduces more bytes

5d5b972

Fix typo

e186b09

Address nits

1dff1fc

threepointone approved these changes Mar 14, 2019

View reviewed changes

trueadm mentioned this pull request Mar 14, 2019

Adds experimental event API scaffolding #15108

Merged

gaearon reviewed Mar 14, 2019

View reviewed changes

sebmarkbage reviewed Mar 14, 2019

View reviewed changes

trueadm added 5 commits March 14, 2019 19:30

Addressed feedback and took a different approach with flags

8f79ee8

Merge branch 'master' into passive-event-listening

159280c

Wrap function in flags

2acacf8

Remove more code that is not needed anymore

3b67255

Adds flag to another location

5b044cf

sebmarkbage reviewed Mar 15, 2019

View reviewed changes

sebmarkbage approved these changes Mar 15, 2019

View reviewed changes

Address feedback on addEventListener

8b34efa

trueadm merged commit 371bbf3 into facebook:master Mar 15, 2019

trueadm deleted the passive-event-listening branch March 15, 2019 09:39

This was referenced Mar 20, 2019

Add internal logic for listening to event responders #15168

Merged

Add part of the event responder system for experimental event API #15179

Merged

sebmarkbage reviewed Mar 22, 2019

View reviewed changes

trueadm mentioned this pull request Mar 27, 2019

Adds experimental event component responder surfaces #15228

Merged

nhunzaker mentioned this pull request Mar 28, 2019

Chrome 73 breaks wheel events #14856

Closed

This was referenced Mar 28, 2019

Adds React event component and React event target support to SSR renderer #15242

Merged

Add additional event API responder surfaces #15248

Merged

NE-SmallTown reviewed Apr 2, 2019

View reviewed changes

trueadm added the React Flare label Apr 26, 2019

trueadm mentioned this pull request Apr 28, 2020

Refactor ElementListenerMap to use expando properties #18766

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add infrastructure for passive/non-passive event support for future API exploration #15036

Add infrastructure for passive/non-passive event support for future API exploration #15036

trueadm commented Mar 6, 2019 •

edited

Loading

philipp-spiess left a comment

nhunzaker left a comment

trueadm commented Mar 13, 2019

nhunzaker commented Mar 13, 2019

trueadm commented Mar 13, 2019

trueadm commented Mar 13, 2019

philipp-spiess left a comment

threepointone left a comment

gaearon Mar 14, 2019 •

edited

Loading

sebmarkbage left a comment

trueadm commented Mar 14, 2019

trueadm commented Mar 14, 2019

sebmarkbage Mar 15, 2019 •

edited

Loading

trueadm Mar 15, 2019

sebmarkbage left a comment

sebmarkbage Mar 22, 2019

trueadm Mar 22, 2019

NE-SmallTown Apr 2, 2019 •

edited

Loading

trueadm Apr 2, 2019

eps1lon Apr 2, 2019

NE-SmallTown Apr 2, 2019

trueadm Apr 2, 2019

Add infrastructure for passive/non-passive event support for future API exploration #15036

Add infrastructure for passive/non-passive event support for future API exploration #15036

Conversation

trueadm commented Mar 6, 2019 • edited Loading

Summary

Why do we need this?

Validate this works

Polyfills

philipp-spiess left a comment

Choose a reason for hiding this comment

nhunzaker left a comment

Choose a reason for hiding this comment

trueadm commented Mar 13, 2019

nhunzaker commented Mar 13, 2019

trueadm commented Mar 13, 2019

trueadm commented Mar 13, 2019

philipp-spiess left a comment

Choose a reason for hiding this comment

threepointone left a comment

Choose a reason for hiding this comment

gaearon Mar 14, 2019 • edited Loading

Choose a reason for hiding this comment

sebmarkbage left a comment

Choose a reason for hiding this comment

trueadm commented Mar 14, 2019

trueadm commented Mar 14, 2019

sebmarkbage Mar 15, 2019 • edited Loading

Choose a reason for hiding this comment

trueadm Mar 15, 2019

Choose a reason for hiding this comment

sebmarkbage left a comment

Choose a reason for hiding this comment

sebmarkbage Mar 22, 2019

Choose a reason for hiding this comment

trueadm Mar 22, 2019

Choose a reason for hiding this comment

NE-SmallTown Apr 2, 2019 • edited Loading

Choose a reason for hiding this comment

trueadm Apr 2, 2019

Choose a reason for hiding this comment

eps1lon Apr 2, 2019

Choose a reason for hiding this comment

NE-SmallTown Apr 2, 2019

Choose a reason for hiding this comment

trueadm Apr 2, 2019

Choose a reason for hiding this comment

trueadm commented Mar 6, 2019 •

edited

Loading

gaearon Mar 14, 2019 •

edited

Loading

sebmarkbage Mar 15, 2019 •

edited

Loading

NE-SmallTown Apr 2, 2019 •

edited

Loading