Replay capture phase for continuous events #22680
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
facebook-github-bot
added
CLA Signed
React Core Team
salazarm
force-pushed
the
replayCapturePhaseContinuousEvents
branch
from
04d2106
to
fb47c18
Compare
|
Comparing: 4ff5f57...bfe8062 Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: Expand to show
|
salazarm
changed the title
salazarm
force-pushed
the
replayCapturePhaseContinuousEvents
branch
from
4f77598
to
e7cc04d
Compare
salazarm
commented
Nov 3, 2021
salazarm
changed the title
salazarm
force-pushed
the
replayCapturePhaseContinuousEvents
branch
from
8a7e8c8
to
236af6c
Compare
gaearon
reviewed
Nov 9, 2021
packages/react-dom/src/__tests__/ReactDOMServerSelectiveHydration-test.internal.js
Show resolved
Hide resolved
…aseContinuousEvents
…aseContinuousEvents
salazarm
force-pushed
the
replayCapturePhaseContinuousEvents
branch
from
6e5bdcb
to
6bf50cb
Compare
gaearon
reviewed
Nov 23, 2021
gaearon
reviewed
Nov 23, 2021
gaearon
reviewed
Nov 23, 2021
salazarm
commented
Nov 30, 2021
| } | ||
| } else { | ||
| // This tree has been unmounted already. Dispatch without a target. | ||
| return nullTarget; |
There was a problem hiding this comment.
I couldn't come up with a test case where we findInstanceBlockingEvent would return nullTarget. It doesn't seem possible that it would ever happen because it would mean that there is a React Instance defined on the target but then nearestMountedFiber returns null for this case. I added a test which attempted to simulate this by removing the target node from its react tree into a tree where there is no react root but that didn't seem to work.
getClosestInstanceFromNode(getEventTarget(queuedEvent.nativeEvent)) did return null for this case though.
Based off how hydration works it shouldn't be possible for the nearestMountedFiber to ever be anything other than a SuspenseInstance. Unless the node is manually moved in userland to a different tree thats already hydrated, I tried doing that but that also didn't trigger the nullTarget. I'm unsure what else to try to trigger it. Any ideas?
salazarm
force-pushed
the
replayCapturePhaseContinuousEvents
branch
from
83a3868
to
ea448a4
Compare
salazarm
force-pushed
the
replayCapturePhaseContinuousEvents
branch
from
ea448a4
to
a88e173
Compare
This was referenced Dec 1, 2021
|
Merged via #22856. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
We want to replay the capture phase for continuous events on hydration (currently we only replay the bubble phase).
This will allow us to call
stopPropagation()in the capture phase to prevent the bubble phase.With this change we also use the browsers native event dispatching to replay events. This allows stopPropagation in the capture phase to actually stop the bubble phase from firing without needing to hook into the native event / simulating the capture / bubble phase in a single phase.
Quirks:
mouseout(normally) andmouseover(replay). (existing behavior)stopPropagationduring the capture phase ofmouseoutwould normally preventmouseenter/mouseleavelisteners from firing (Codesandbox showing this existing behavior: https://codesandbox.io/s/silly-microservice-n3vv1?file=/src/App.js) but since before this PR we didn't replay the capture phase there was no way to stop it during replaying.stopPropagationduring the capture phase ofmouseoverwould preventmouseenter(Replay usesmouseoverto emulatemouseEnterbecause we don't replaymouseoutevents, perhaps we could replay them in a special way for replay where we actually ignore them from simpleEventPlugin if they're being replayed).mouseoutis never replayed somouseleaveis also never replayed (existing behavior)I included these changes under the existing
enableCapturePhaseSelectiveHydrationWithoutDiscreteEventReplayso that I don't need a new experiment and since we want to enable all of these changes togetherHow did you test this change?
jest