Add more phases to the ReactFiberApplyGesture #32578
Merged
Conversation
This file contains hidden or 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
github-actions
bot
added
the
React Core Team
sebmarkbage
commented
Mar 12, 2025
| } | ||
| } | ||
|
|
||
| function trackExitViewTransitions(deletion: Fiber): void { |
There was a problem hiding this comment.
Naming becomes interesting for these because they're in reverse. This will actually trigger the enter className since deletions are treated as "enter" in reverse. So I'm inclined to rename this to "trackEnterViewTransitions" and vice versa.
sebmarkbage
force-pushed
the
swipeclones
branch
from
2703474 to
2da1fbe
Compare
sebmarkbage
force-pushed
the
swipeclones
branch
from
2da1fbe to
1701568
Compare
sebmarkbage
force-pushed
the
swipeclones
branch
from
1701568 to
a282d35
Compare
We need to be able to refer to these to measure and apply names to them in future passes.
We do this so that we can collect any exiting pairs that will have corresponding insertions. I don't love that we do that here instead of a previous phase but it would require another pass.
This makes it a bit easier not to mess it up at the cost of some more stack allocation (which may cause overflow).
This is a bit simpler because it doesn't do cloning so it matches the similar phases of ReactFiberCommitViewTransitions. The "old" phase doesn't need resetting because all the clones get deleted at the end. We don't need to visit the insertions other than just the top level "enter" because any pair will be visited by the deletion recursion.
This can reuse more from ReactFiberCommitViewTransitions because the restore code is more broad than technically necessary.
Since we're applying transitions in reverse, all "insertions" in the newly finished trees are actually "exits". All "deletions" are "enters".
…n the exit/unhide phase
Once we reach a HostComponent this will turn null and we can bail.
Since this is reversed. Also adding a stub for the missing measureUpdateViewTransition.
sebmarkbage
force-pushed
the
swipeclones
branch
from
68940c9 to
59a9547
Compare
rickhanlonii
approved these changes
Mar 14, 2025
| } | ||
| let child = deletion.child; | ||
| while (child !== null) { | ||
| if (child.tag === OffscreenComponent && child.memoizedState === null) { |
There was a problem hiding this comment.
Most other places we use a switch, which can be nice because you can grep case OffscreenComponent to find all the special component handling cases
There was a problem hiding this comment.
One annoying thing about closure is that it doesn't change from switch to if, when the switch is unnecessary. The switch can have special meaning (e.g. most VMs turn it into a jump table for integers and sometimes even strings) but for simple cases it's just more bytes.
But yea, I think it's probably also worth while adding some more cases here like Portals even if it's just a comment that we've considered it.
I have a to do to make a pass over some of the enter/exit passes which doesn't check the OffscreenComponent but maybe they do need to.
|
I think a lot of the actually recursion of flags and stuff could be covered by unit tests. It just needs a ton of them to cover all the edge cases. It's just not worth writing them all until we have the rough structure working and getting closer to the final API. Browser tests aren't actually that useful for most of the browser specific things anyway because they end up being things like flashes that don't show up in the assertions anyway, or you have to write it very specifically to assert a specific browser bug then you've already covered it anyway ideally with a comment for why. |
github-actions bot
pushed a commit
that referenced
this pull request
Mar 14, 2025
Stacked on #32585 and #32605. This adds more loops for the phases of "Apply Gesture". It doesn't implement the interesting bit yet like adding view-transition-names and measurements. I'll do that in a separate PR to keep reviewing easier. The three phases of this approach is roughly: - Clone and apply names to the "old" state. - Inside startViewTransition: Apply names to the "new" state. Measure both the "old" and "new" state to know whether to cancel some of them. Delete the clones which will include all the "old" names. - After startViewTransition: Restore "new" names back to no view-transition-name. Since we don't have any other Effects in these phases we have a bit more flexibility and we can avoid extra phases that traverse the tree. I've tried to avoid any additional passes. An interesting consequence of this approach is that we could measure both the "old" and "new" state before `startViewTransition`. This would be more efficient because we wouldn't need to take View Transition snapshots of parts of the tree that won't actually animate. However, that would require an extra pass and force layout earlier. It would also have different semantics from the fire-and-forget View Transitions because we could optimize better which can be visible. It would also not account for any late mutations. So I decided to instead let the layout be computed by painting as usual and then measure both "old" and "new" inside the startViewTransition instead. Then canceling anything that doesn't animate to keep it consistent. Unfortunately, though there's not a lot of code sharing possible in these phases because the strategy is so different with the cloning and because the animation is performed in reverse. The "finishedWork" Fiber represents the "old" state and the "current" Fiber represents the "new" state. The most complicated phase is the cloning. I actually ended up having to make a very different pattern from the other phases and CommitWork in general. Because we have to clone as we go and also do other things like apply names and finding pairs, it has more phases. I ended up with an approach that uses three different loops. The outer one for updated trees, one for inserted trees that don't need cloning (doesn't include reappearing offscreen) and one for not updated trees that still need cloning. Inside each loop it can also be in different phases which I track with the `visitPhase` enum - this pattern is kind of new. Additionally, we need to measure the cloned nodes after we've applied mutations to them and we have to wait until the whole tree is inserted. We don't have a reference to these DOM elements in the Fiber tree since that still refers to the original ones. We need to store the cloned elements somewhere. So I added a temporary field on the ViewTransitionState to keep track of any clones owned by that ViewTransition. When we deep clone an unchanged subtree we don't have DOM element instances. It wouldn't be quite safe to try to find them from the tree structure. So we need to avoid the deep clones if we might need DOM elements. Therefore we keep traversing in the case where we need to find nested ViewTransition boundaries that are either potentially affected by layout or a "pair". For the other two phases the pattern there's a lot of code duplication since it's slightly different from the commit ones but they at least follow the same pattern. For the restore phase I was actually able to reuse most of the code. I don't love how much code this is. DiffTrain build for [c4a3b92](c4a3b92)
github-actions bot
pushed a commit
that referenced
this pull request
Mar 14, 2025
Stacked on #32585 and #32605. This adds more loops for the phases of "Apply Gesture". It doesn't implement the interesting bit yet like adding view-transition-names and measurements. I'll do that in a separate PR to keep reviewing easier. The three phases of this approach is roughly: - Clone and apply names to the "old" state. - Inside startViewTransition: Apply names to the "new" state. Measure both the "old" and "new" state to know whether to cancel some of them. Delete the clones which will include all the "old" names. - After startViewTransition: Restore "new" names back to no view-transition-name. Since we don't have any other Effects in these phases we have a bit more flexibility and we can avoid extra phases that traverse the tree. I've tried to avoid any additional passes. An interesting consequence of this approach is that we could measure both the "old" and "new" state before `startViewTransition`. This would be more efficient because we wouldn't need to take View Transition snapshots of parts of the tree that won't actually animate. However, that would require an extra pass and force layout earlier. It would also have different semantics from the fire-and-forget View Transitions because we could optimize better which can be visible. It would also not account for any late mutations. So I decided to instead let the layout be computed by painting as usual and then measure both "old" and "new" inside the startViewTransition instead. Then canceling anything that doesn't animate to keep it consistent. Unfortunately, though there's not a lot of code sharing possible in these phases because the strategy is so different with the cloning and because the animation is performed in reverse. The "finishedWork" Fiber represents the "old" state and the "current" Fiber represents the "new" state. The most complicated phase is the cloning. I actually ended up having to make a very different pattern from the other phases and CommitWork in general. Because we have to clone as we go and also do other things like apply names and finding pairs, it has more phases. I ended up with an approach that uses three different loops. The outer one for updated trees, one for inserted trees that don't need cloning (doesn't include reappearing offscreen) and one for not updated trees that still need cloning. Inside each loop it can also be in different phases which I track with the `visitPhase` enum - this pattern is kind of new. Additionally, we need to measure the cloned nodes after we've applied mutations to them and we have to wait until the whole tree is inserted. We don't have a reference to these DOM elements in the Fiber tree since that still refers to the original ones. We need to store the cloned elements somewhere. So I added a temporary field on the ViewTransitionState to keep track of any clones owned by that ViewTransition. When we deep clone an unchanged subtree we don't have DOM element instances. It wouldn't be quite safe to try to find them from the tree structure. So we need to avoid the deep clones if we might need DOM elements. Therefore we keep traversing in the case where we need to find nested ViewTransition boundaries that are either potentially affected by layout or a "pair". For the other two phases the pattern there's a lot of code duplication since it's slightly different from the commit ones but they at least follow the same pattern. For the restore phase I was actually able to reuse most of the code. I don't love how much code this is. DiffTrain build for [c4a3b92](c4a3b92)
sebmarkbage
added a commit
that referenced
this pull request
Mar 14, 2025
… Phase (#32599) Stacked on #32578. We need to apply view-transition-names to the clones that we create in the "old" phase for the ViewTransition boundaries that should activate. Finding pairs is a little trickier than in ReactFiberCommitViewTransitions. Normally we collect all name "insertions" in the `accumulateSuspenseyCommit` phase before we even commit. Then in the snapshot do we visit all "deletions" and since we already collected all the insertions we know immediately if the deletion had a pair and should therefore get a "name" assigned to activate the boundary. For ReactFiberApplyGesture we need to assign names to "insertions" since it's in reverse but we don't already have a map of deletions. Therefore we need to first visit all deletions. Instead of doing that in a completely separate pass, we instead visit deletions in the same pass to find pairs. Since this is in the same pass we might visit insertions before deletions or vice versa depending on document order. However, we can deal with this by applying the name to the insertion when we find the deletion if we've already made the clones at that point. Applying names to pure exits, updates or nested (relayout) is a bit more straight-forward.
github-actions bot
pushed a commit
that referenced
this pull request
Mar 14, 2025
… Phase (#32599) Stacked on #32578. We need to apply view-transition-names to the clones that we create in the "old" phase for the ViewTransition boundaries that should activate. Finding pairs is a little trickier than in ReactFiberCommitViewTransitions. Normally we collect all name "insertions" in the `accumulateSuspenseyCommit` phase before we even commit. Then in the snapshot do we visit all "deletions" and since we already collected all the insertions we know immediately if the deletion had a pair and should therefore get a "name" assigned to activate the boundary. For ReactFiberApplyGesture we need to assign names to "insertions" since it's in reverse but we don't already have a map of deletions. Therefore we need to first visit all deletions. Instead of doing that in a completely separate pass, we instead visit deletions in the same pass to find pairs. Since this is in the same pass we might visit insertions before deletions or vice versa depending on document order. However, we can deal with this by applying the name to the insertion when we find the deletion if we've already made the clones at that point. Applying names to pure exits, updates or nested (relayout) is a bit more straight-forward. DiffTrain build for [2e38573](2e38573)
This was referenced Mar 17, 2025
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.
Stacked on #32585 and #32605.
This adds more loops for the phases of "Apply Gesture". It doesn't implement the interesting bit yet like adding view-transition-names and measurements. I'll do that in a separate PR to keep reviewing easier.
The three phases of this approach is roughly:
Since we don't have any other Effects in these phases we have a bit more flexibility and we can avoid extra phases that traverse the tree. I've tried to avoid any additional passes.
An interesting consequence of this approach is that we could measure both the "old" and "new" state before
startViewTransition. This would be more efficient because we wouldn't need to take View Transition snapshots of parts of the tree that won't actually animate. However, that would require an extra pass and force layout earlier. It would also have different semantics from the fire-and-forget View Transitions because we could optimize better which can be visible. It would also not account for any late mutations. So I decided to instead let the layout be computed by painting as usual and then measure both "old" and "new" inside the startViewTransition instead. Then canceling anything that doesn't animate to keep it consistent.Unfortunately, though there's not a lot of code sharing possible in these phases because the strategy is so different with the cloning and because the animation is performed in reverse. The "finishedWork" Fiber represents the "old" state and the "current" Fiber represents the "new" state.
The most complicated phase is the cloning. I actually ended up having to make a very different pattern from the other phases and CommitWork in general. Because we have to clone as we go and also do other things like apply names and finding pairs, it has more phases. I ended up with an approach that uses three different loops. The outer one for updated trees, one for inserted trees that don't need cloning (doesn't include reappearing offscreen) and one for not updated trees that still need cloning. Inside each loop it can also be in different phases which I track with the
visitPhaseenum - this pattern is kind of new.Additionally, we need to measure the cloned nodes after we've applied mutations to them and we have to wait until the whole tree is inserted. We don't have a reference to these DOM elements in the Fiber tree since that still refers to the original ones. We need to store the cloned elements somewhere. So I added a temporary field on the ViewTransitionState to keep track of any clones owned by that ViewTransition.
When we deep clone an unchanged subtree we don't have DOM element instances. It wouldn't be quite safe to try to find them from the tree structure. So we need to avoid the deep clones if we might need DOM elements. Therefore we keep traversing in the case where we need to find nested ViewTransition boundaries that are either potentially affected by layout or a "pair".
For the other two phases the pattern there's a lot of code duplication since it's slightly different from the commit ones but they at least follow the same pattern. For the restore phase I was actually able to reuse most of the code.
I don't love how much code this is.