Prevent view flattening for direct host children of ViewTransition#690
Prevent view flattening for direct host children of ViewTransition#690
Conversation
Greptile SummaryThis PR introduces Key observations:
Confidence Score: 2/5
Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const transition = fabricStartViewTransition( | ||
| // mutation | ||
| () => { | ||
| mutationCallback(); // completeRoot should run here | ||
| layoutCallback(); | ||
| afterMutationCallback(); | ||
| }, | ||
| ); | ||
|
|
||
| if (transition == null) { | ||
| if (__DEV__) { | ||
| console.warn( | ||
| "startViewTransition didn't kick off transition in Fabric, the ViewTransition ReactNativeFeatureFlag might not be enabled.", | ||
| ); | ||
| } | ||
| // Flush remaining work synchronously. | ||
| mutationCallback(); | ||
| layoutCallback(); | ||
| // Skip afterMutationCallback(). We don't need it since we're not animating. | ||
| spawnedWorkCallback(); | ||
| // Skip passiveCallback(). Spawned work will schedule a task. | ||
| return null; | ||
| } | ||
|
|
||
| transition.ready.then(() => { | ||
| spawnedWorkCallback(); | ||
| }); | ||
|
|
||
| transition.finished.finally(() => { | ||
| passiveCallback(); |
There was a problem hiding this comment.
fabricStartViewTransition called with wrong arity and return type mismatch
The Flow type added in scripts/flow/react-native-host-hooks.js declares nativeFabricUIManager.startViewTransition as:
startViewTransition: (
mutationCallback: () => void,
onReady: () => void,
onComplete: () => void,
) => boolean
But fabricStartViewTransition is called here with only 1 argument, omitting onReady and onComplete. If the native side invokes those callbacks, it will call undefined() and crash.
More critically, the return type is declared as boolean, but the code immediately treats it as an object with .ready and .finished Promise properties. A boolean true (successful transition) would pass the transition == null check (since true != null), and then transition.ready.then(...) on line 263 would throw a TypeError: Cannot read properties of true (reading 'ready').
Either the Flow type declaration for startViewTransition is incorrect (it should return RunningViewTransition | null and accept 1 callback), or the call site here is missing the two callback arguments. The type and the usage need to be reconciled.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/react-native-renderer/src/ReactFiberConfigFabricWithViewTransition.js
Line: 239-268
Comment:
**`fabricStartViewTransition` called with wrong arity and return type mismatch**
The Flow type added in `scripts/flow/react-native-host-hooks.js` declares `nativeFabricUIManager.startViewTransition` as:
```
startViewTransition: (
mutationCallback: () => void,
onReady: () => void,
onComplete: () => void,
) => boolean
```
But `fabricStartViewTransition` is called here with only **1 argument**, omitting `onReady` and `onComplete`. If the native side invokes those callbacks, it will call `undefined()` and crash.
More critically, the return type is declared as `boolean`, but the code immediately treats it as an object with `.ready` and `.finished` Promise properties. A boolean `true` (successful transition) would pass the `transition == null` check (since `true != null`), and then `transition.ready.then(...)` on line 263 would throw a `TypeError: Cannot read properties of true (reading 'ready')`.
Either the Flow type declaration for `startViewTransition` is incorrect (it should return `RunningViewTransition | null` and accept 1 callback), or the call site here is missing the two callback arguments. The type and the usage need to be reconciled.
How can I resolve this? If you propose a fix, please make it concise.
| interface ViewTransitionPseudoElementType extends mixin$Animatable { | ||
| _scope: HTMLElement; | ||
| _selector: string; | ||
| getComputedStyle(): CSSStyleDeclaration; | ||
| } | ||
|
|
||
| function ViewTransitionPseudoElement( | ||
| this: ViewTransitionPseudoElementType, | ||
| pseudo: string, | ||
| name: string, | ||
| ) { | ||
| // TODO: Get the owner document from the root container. | ||
| this._pseudo = pseudo; | ||
| this._name = name; | ||
| } |
There was a problem hiding this comment.
DOM types and mismatched property names in RN interface
This ViewTransitionPseudoElementType interface references HTMLElement and CSSStyleDeclaration, which are DOM-specific types that do not exist in the React Native environment. Beyond the type-checking errors this will produce under Flow, the property names in the interface (_scope, _selector) don't match the properties the constructor actually sets: this._pseudo and this._name. The interface is effectively describing a different object than what ViewTransitionPseudoElement constructs.
This looks like it was copied from the DOM view-transition implementation without being adapted for React Native. The interface should either be removed or updated to reflect the actual RN structure (using _pseudo: string and _name: string).
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/react-native-renderer/src/ReactFiberConfigFabricWithViewTransition.js
Line: 43-57
Comment:
**DOM types and mismatched property names in RN interface**
This `ViewTransitionPseudoElementType` interface references `HTMLElement` and `CSSStyleDeclaration`, which are DOM-specific types that do not exist in the React Native environment. Beyond the type-checking errors this will produce under Flow, the property names in the interface (`_scope`, `_selector`) don't match the properties the constructor actually sets: `this._pseudo` and `this._name`. The interface is effectively describing a different object than what `ViewTransitionPseudoElement` constructs.
This looks like it was copied from the DOM view-transition implementation without being adapted for React Native. The interface should either be removed or updated to reflect the actual RN structure (using `_pseudo: string` and `_name: string`).
How can I resolve this? If you propose a fix, please make it concise.| // Ensure memoizedProps reflects the finalized props so that | ||
| // future renders diff against the correct props. | ||
| if (instanceProps !== newProps) { | ||
| workInProgress.memoizedProps = instanceProps; | ||
| } |
There was a problem hiding this comment.
Silent override of user-specified collapsable prop
finalizeViewTransitionChild in the Fabric config uses Object.assign({}, props, {collapsable: false}), which unconditionally overrides any collapsable prop the user may have explicitly set on their component. A developer who writes <View collapsable={true}> as a direct child of <ViewTransition> will silently have their prop ignored — the renderer will receive collapsable: false without any indication.
A __DEV__-only warning when the user's original props contain collapsable: true (i.e., explicitly opting into flattening) would help surface this conflict to developers during development.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/react-reconciler/src/ReactFiberCompleteWork.js
Line: 1449-1453
Comment:
**Silent override of user-specified `collapsable` prop**
`finalizeViewTransitionChild` in the Fabric config uses `Object.assign({}, props, {collapsable: false})`, which unconditionally overrides any `collapsable` prop the user may have explicitly set on their component. A developer who writes `<View collapsable={true}>` as a direct child of `<ViewTransition>` will silently have their prop ignored — the renderer will receive `collapsable: false` without any indication.
A `__DEV__`-only warning when the user's original props contain `collapsable: true` (i.e., explicitly opting into flattening) would help surface this conflict to developers during development.
How can I resolve this? If you propose a fix, please make it concise.
Wherever the reconciler checks supportsMutation before running view transition logic, add an else-if branch gated on enableViewTransitionForPersistenceMode for persistent renderers (Fabric). This follows the pattern that supportsMutation guards mutation-mode-only logic, and persistent mode should have its own branch rather than being lumped under a single supportsViewTransition capability flag. For now the persistent mode branches duplicate the mutation logic. The flag defaults to false in all channels.
…n out of ReactFiberConfigWithNoMutation
…n out of ReactFiberConfigWithNoMutation
Summary: - turn on enableViewTransition feature - stub shim - run some mutation config fn at persistence mode too
Better reflects that this signals the transition has started rather than initiating it.
…enable startViewTransition
Add finalizeViewTransitionChild renderer config function called during completeWork for HostComponents that are direct children of a ViewTransition boundary. Fabric implements it by injecting collapsable: false to prevent native view flattening. DOM is a no-op. Uses a stack cursor (viewTransitionCursor) to track whether the current fiber is inside a ViewTransition — ViewTransition pushes true, HostComponent pushes false (acting as a boundary).
everettbu
force-pushed
the
viewtransition-child-non-collapsible
branch
from
85b3cb9 to
0d55141
Compare
2 participants
Mirror of facebook/react#36012
Original author: zeyap
Summary
finalizeViewTransitionChild(type, props)renderer config function, called duringcompleteWorkfor HostComponents that are direct children of a<ViewTransition>boundarycollapsable: falseto prevent native view flattening, which would remove the view from the platform hierarchy and break view transition animationsviewTransitionCursorstack cursor to efficiently track whether the current fiber is inside a ViewTransition —ViewTransitionComponentpushestrue,HostComponentpushesfalse(acting as a boundary so only direct host children are affected)Test plan