ReactDOM.useEvent: Add support for experimental scopes API #18375
Conversation
facebook-github-bot
added
CLA Signed
React Core Team
|
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:
|
trueadm
force-pushed
the
use-event-scopes
branch
from
340ff36 to
00cfbaa
Compare
| const type = ((event.type: any): DOMTopLevelEventType); | ||
| const listeners = eventTypeMap.get(type); | ||
| if (listeners !== undefined) { | ||
| const captureListeners = Array.from(listeners.captured); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Good point, although we don't need to add it till v17.
…acebook#18375)" This reverts commit a16b349. * ReactDOM.useEvent: Add support for experimental scopes API
…acebook#18375)" This reverts commit a16b349. * ReactDOM.useEvent: Add support for experimental scopes API
| const listener = bubbleListeners[i]; | ||
| const {callback} = listener; | ||
| dispatchListeners.push(callback); | ||
| dispatchInstances.push(((lastHostComponent: any): Element)); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
You can't have listeners, but could it be bubbling from a text node?
There was a problem hiding this comment.
Text nodes can't have children, so there should be no case where we'd ever propagate through into or from a text node.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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. :)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Added support for HostText + test in #18441.
This is the last big piece of the
useEventAPI puzzle. It ports the existing behavior in React Flare, where you can attach listeners to the experimental Scopes API. This means that withuseEventyou can attach events to a "scope" rather than a physicalEventTarget, making way for us to be able to port the internal a11y components and their usages of this over touseEvent, 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.