Attach Listeners Eagerly to Roots and Portal Containers #19659
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 46ee6be:
|
Details of bundled changes.Comparing: 90d212d...46ee6be react-dom
ReactDOM: size: 0.0%, gzip: -0.0% Size changes (stable) |
Details of bundled changes.Comparing: 90d212d...46ee6be react-dom
ReactDOM: size: 0.0%, gzip: -0.0% Size changes (experimental) |
gaearon
force-pushed
the
evv
branch
9 times, most recently
from
59f2795
to
2252c63
Compare
gaearon
changed the title
gaearon
force-pushed
the
evv
branch
3 times, most recently
from
2bf6238
to
6c6ddd7
Compare
| @@ -327,6 +329,41 @@ export function listenToNonDelegatedEvent( | |||
| } | |||
| } | |||
|
|
|||
| const listeningMarker = | |||
There was a problem hiding this comment.
Want to have a flag, but one per React copy. To guard around cases like "rendering a portal into a div managed by another React".
There was a problem hiding this comment.
Please can you explain more about why we need a marker?
There was a problem hiding this comment.
We need to mark a node as processed so we don’t keep adding more and more listeners to it every time. Like you mentioned below.
But we need to mark it with a field that’s local to this copy of React. In case two copies use the same portal container. You don’t want one of them to prevent another from adding listeners.
There’s a test that fails if we don’t do this (although it reproduces this problem mostly by accident due to resetModules and a shared container).
There was a problem hiding this comment.
Should we copy the test and scope it to this use case? It sounds like that test could change and we would lose the regression test here.
| if (!enableEagerRootListeners) { | ||
| ensureListeningTo(rootContainerElement, propKey, domElement); | ||
| } else if (propKey === 'onScroll') { | ||
| listenToNonDelegatedEvent('scroll', domElement); |
There was a problem hiding this comment.
Can't avoid for scroll since it only bubbles to document.
|
|
||
| export function listenToAllSupportedEvents(rootContainerElement: EventTarget) { | ||
| if (enableEagerRootListeners) { | ||
| if ((rootContainerElement: any)[listeningMarker]) { |
There was a problem hiding this comment.
Only do this once per container.
There was a problem hiding this comment.
What if we only did this once per event per container? Would that allow us to attach custom events lazily later?
| null, | ||
| ); | ||
| } | ||
| listenToNativeEvent( |
|
|
||
| if (enableEagerRootListeners) { | ||
| for (let i = 0; i < dependencies.length; i++) { | ||
| allNativeEvents.add(dependencies[i]); |
There was a problem hiding this comment.
Make a list once when registering plugins. Then use it for any root. Assumed not to change.
| ) { | ||
| return; | ||
| if (!enableEagerRootListeners) { | ||
| const eventListenerMap = getEventListenerMap(targetContainer); |
There was a problem hiding this comment.
Don't need to do this anymore since it can't happen with eager listeners.
There was a problem hiding this comment.
Most likely not, but we can always remove this later. Less changes at once make it easier to identify possible issues.
There was a problem hiding this comment.
I mean it doesn’t work unless I remove it because that condition no longer makes sense (we don’t fill the map anymore with React event names).
We could fill the map of course but that would complicate the code and the condition would always be false anyway because the onSelect event would by definition always be there.
So does this mean that |
It doesn't support events that are not listed in registered as top level listeners, correct. However, if there are specific events we want to support, we can add them to the list just for that purpose. (I've checked our existing usages and haven't found any that are being used.) |
|
In the case of having a portal on the same node, do we ensure we don't add the event listeners multiple times? i.e. |
|
Yeah, there's a check that marks the node as already having the listeners (per copy of React). |
We should definitely add a warning here then. Also, why doens't the custom event test in |
* Failing test for facebook#19608 * Attach Listeners Eagerly to Roots and Portal Containers * Forbid createEventHandle with custom events We can't support this without adding more complexity. It's not clear that this is even desirable, as none of our existing use cases need custom events. This API primarily exists as a deprecation strategy for Flare, so I don't think it is important to expand its support beyond what Flare replacement code currently needs. We can later revisit it with a better understanding of the eager/lazy tradeoff but for now let's remove the inconsistency. * Reduce risk by changing condition only under the flag Co-authored-by: koba04 <[email protected]>
Fixes #19608.
It uncovered a flaw in our current bubbling strategy: if the portal tree doesn't have a particular listener (e.g.
onClick), but the parent tree does, the event doesn't bubble (because we don't listen to that even on the portal container node).This fix is to listen to all supported events at the time the root or the portal container are attached. This means we need to know all of them ahead of time, but we already do.
I added a new flag called
enableEagerRootListenersthat turns this change on (set tofalseeverywhere except WWW where it's dynamic). All of the existing behavior is behind its falsy branch since we'd like a safety net. I edited all existing codesites that are branch-specific to be wrapped so they're easy to find and delete later, whichever way we decide to go.Aside from fixing #19608, this change also has a few other benefits:
The downside of this change is that it makes
createEventHandleless powerful, as it would only work for known events. However, the list of known events is finite, and we can manually add the missing ones if there is a need. Behind the flag, I've made it throw for unknown events.This also introduces some risk in case browsers do something unfortunate when a particular event is registered. (However, this would have already been the case for when you eventually add an event handler somewhere deep.) At least with the Passive flag on the bad ones should be mitigated.
Review without whitespace: https://github.com/facebook/react/pull/19659/files?w=1