ReactDOM.useEvent: Add support for experimental scopes API by trueadm · Pull Request #18375 · facebook/react

trueadm · 2020-03-24T12:22:50Z

This is the last big piece of the useEvent API puzzle. It ports the existing behavior in React Flare, where you can attach listeners to the experimental Scopes API. This means that with useEvent you can attach events to a "scope" rather than a physical EventTarget, making way for us to be able to port the internal a11y components and their usages of this over to useEvent, allowing us to remove React Flare. This feature is a very important one, as it allows us to attach events to something that doesn't require a physical DOM element, as these can be problematic in places with <table> and <select>, plus adding in additional DOM wrapper elements can break the semantics of accessibility tools and CSS styling – which is why we offered this feature for Scopes with Flare.

codesandbox-ci · 2020-03-24T12:26:44Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 00cfbaa:

Sandbox	Source
confident-lehmann-15t9p	Configuration

sizebot · 2020-03-24T12:28:54Z

	Warnings
⚠️	Base commit is broken: `0140118`

Size changes (stable)

Generated by 🚫 dangerJS against 2d1b222

sizebot · 2020-03-24T12:29:24Z

	Warnings
⚠️	Base commit is broken: `0140118`

Size changes (experimental)

Generated by 🚫 dangerJS against 2d1b222

threepointone

it looks fine to me, and I assume this work has already been validated by the flare effort.

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

threepointone · 2020-03-26T13:10:53Z

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

+          const type = ((event.type: any): DOMTopLevelEventType);
+          const listeners = eventTypeMap.get(type);
+          if (listeners !== undefined) {
+            const captureListeners = Array.from(listeners.captured);


btw I noticed we use this in a few places right now, so we should probably add Array.from to this page https://reactjs.org/docs/javascript-environment-requirements.html

Good point, although we don't need to add it till v17.

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

…acebook#18375)" This reverts commit a16b349. * ReactDOM.useEvent: Add support for experimental scopes API

* Revert "ReactDOM.useEvent: enable on internal www and add inspection test (#18395)" This reverts commit e0ab1a4. * Revert "ReactDOM.useEvent: Add support for experimental scopes API (#18375)" This reverts commit a16b349. * ReactDOM.useEvent: Add support for experimental scopes API

gaearon · 2020-03-31T02:17:52Z

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

+              const listener = bubbleListeners[i];
+              const {callback} = listener;
+              dispatchListeners.push(callback);
+              dispatchInstances.push(((lastHostComponent: any): Element));


As far as I can tell, this is what caused the production crashes with today’s sync. This is only set if we “met” a host component in the above traversal before we “met” a Scope. Does anything actually guarantee that? We should probably be more careful with any casts in the production codepaths.

Never mind, see below. I got confused between two code paths. I’m still not sure this was safe though — could the innermost instance be a TextInstance for example, followed by a Scope? I think that could still crash.

The target can only ever be a DOM element as you can't have listeners on text nodes, I'll add a null check for this in the follow up PR too though!

You can't have listeners, but could it be bubbling from a text node?

Text nodes can't have children, so there should be no case where we'd ever propagate through into or from a text node.

No, I mean something like this:

<div> <Scope> {'hello'}{'world'} </Scope> </div>

If I dispatch an event on the text node, won't it trigger this case?

How are you dispatching the event? Whilst you can manually create an bubbling event and dispatch it directly on a text node (using the Node.prototype.dispatchEvent), this won't ever occur from an actual real-world event that the user would do.

We only store the host component when it's an actual host component instance (by checking the tag). I can make it so we do for those too though. :)

this won't ever occur from an actual real-world event that the user would do.

I wouldn't be so sure, I remember text nodes firing events in Firefox in some cases. Not saying it should be common but may happen in production.

Added support for HostText + test in #18441.

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

trueadm requested review from gaearon, necolas, sebmarkbage and threepointone March 24, 2020 12:22

facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Mar 24, 2020

ReactDOM.useEvent: Add support for experimental scopes API

00cfbaa

trueadm force-pushed the use-event-scopes branch from 340ff36 to 00cfbaa Compare March 24, 2020 16:17

threepointone approved these changes Mar 26, 2020

View reviewed changes

Address feedback

2d1b222

trueadm merged commit a16b349 into facebook:master Mar 26, 2020

trueadm deleted the use-event-scopes branch March 26, 2020 13:29

acdlite added a commit to acdlite/react that referenced this pull request Mar 31, 2020

Revert "ReactDOM.useEvent: Add support for experimental scopes API (f…

8f3a84e

…acebook#18375)" This reverts commit a16b349. * ReactDOM.useEvent: Add support for experimental scopes API

acdlite added a commit to acdlite/react that referenced this pull request Mar 31, 2020

Revert "ReactDOM.useEvent: Add support for experimental scopes API (f…

fe0d060

…acebook#18375)" This reverts commit a16b349. * ReactDOM.useEvent: Add support for experimental scopes API

acdlite mentioned this pull request Mar 31, 2020

Out-of-band sync #18439

Closed

gaearon reviewed Mar 31, 2020

View reviewed changes

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

trueadm mentioned this pull request Mar 31, 2020

ReactDOM.useEvent: revert and add guard for null stateNode #18441

Merged

Comments

Conversation

trueadm commented Mar 24, 2020

Uh oh!

codesandbox-ci bot commented Mar 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sizebot commented Mar 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Size changes (stable)

Uh oh!

sizebot commented Mar 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Size changes (experimental)

Uh oh!

threepointone left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

trueadm Mar 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaearon Mar 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

trueadm Mar 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codesandbox-ci bot commented Mar 24, 2020 •

edited

Loading

sizebot commented Mar 24, 2020 •

edited

Loading

sizebot commented Mar 24, 2020 •

edited

Loading

trueadm Mar 31, 2020 •

edited

Loading

gaearon Mar 31, 2020 •

edited

Loading

trueadm Mar 31, 2020 •

edited

Loading