Prevent errors from comment node roots with enableViewTransition #33205
Conversation
There was a problem hiding this comment.
Any logic around comment nodes should be gated on disableCommentsAsDOMContainers.
However, I don't think it's quite the right fix just to cover it up though.
It also needs to pair with cancelRootViewTransitionName. If we just cover it up, we might instead cause bugs that leave things not cleaned up.
I think maybe the issue here is that we're calling restoreRootViewTransitionName even if no transition was started. We should probably ideally gate that call on if we called the cancelRootViewTransitionName. That way if you didn't start a ViewTransition this error can't happen.
However, if a ViewTransition started in one of these roots, what should happen?
| if ( | ||
| containerInstance.style && | ||
| containerInstance.style.viewTransitionName === 'root' | ||
| ) { |
There was a problem hiding this comment.
| if ( | |
| containerInstance.style && | |
| containerInstance.style.viewTransitionName === 'root' | |
| ) { | |
| if ( | |
| (disableCommentsAsDOMContainers || containerInstance.nodeType !== COMMENT_NODE) && | |
| // $FlowFixMe[prop-missing] | |
| containerInstance.style.viewTransitionName === 'root' | |
| ) { |
Since most of this applying and restoring only applies to the documentElement which we get even if it's a comment. This code is mainly gated on the gesture case. So we can ignore it if it's not a gesture case.
However, I think we probably should throw in the gesture case if it's a comment container so that it throws before getting to the restore.
There was a problem hiding this comment.
We should throw here if it's a comment node container:
An alternative could be to take the same path as Or we could skip the clear/restore of the root naming altogether for comment nodes and provide a dev warning that you are have a ViewTransition with a comment root and it will be a global animation by default. |
jackpope
force-pushed
the
view-transition-comment-containers
branch
from
987e85e to
9536fc1
Compare
jackpope
force-pushed
the
view-transition-comment-containers
branch
from
9536fc1 to
33b5931
Compare
|
Comparing: 50389e1...33b5931 Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: (No significant changes) |
|
Updated to skip the cancel/restore and warn in dev |
) We have many cases internally where the `containerInstance` resolves to a comment node. `restoreRootViewTransitionName` is called when `enableViewTransition` is on, even without introducing a `<ViewTransition />`. So that means it can crash pages because `containerInstance.style` is `undefined` just by turning on the flag. This skips cancel/restore of root view transition name if a comment node is the root. DiffTrain build for [3710c4d](3710c4d)
…ebook#33205) We have many cases internally where the `containerInstance` resolves to a comment node. `restoreRootViewTransitionName` is called when `enableViewTransition` is on, even without introducing a `<ViewTransition />`. So that means it can crash pages because `containerInstance.style` is `undefined` just by turning on the flag. This skips cancel/restore of root view transition name if a comment node is the root. DiffTrain build for [3710c4d](facebook@3710c4d)
We have many cases internally where the
containerInstanceresolves to a comment node.restoreRootViewTransitionNameis called whenenableViewTransitionis on, even without introducing a<ViewTransition />. So that means it can crash pages becausecontainerInstance.styleisundefinedjust by turning on the flag.This adds a check for
undefinedso we can roll the flag out more broadly.