From 108faede7be0c61e4e5c73c9f2ca3ad8dfe6d9f1 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Wed, 21 Jul 2021 20:41:47 -0400 Subject: [PATCH 1/3] Fix type of Offscreen props argument Fixes an oversight from a previous refactor. The fiber that wraps a Suspense component's children used to be a Fragment but now it's on Offscreen fiber, so its props type has changed. There's a special hydration path where I forgot to update this. This isn't observable because we don't ever end up rendering this particular fiber (because the Suspense boundary is in its fallback state) but we should fix it anyway to avoid a potential regression in the future. --- .../react-reconciler/src/ReactFiberBeginWork.new.js | 12 ++++++++---- .../react-reconciler/src/ReactFiberBeginWork.old.js | 12 ++++++++---- 2 files changed, 16 insertions(+), 8 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.new.js b/packages/react-reconciler/src/ReactFiberBeginWork.new.js index 50a5a81a1f099..fc6b24fa36639 100644 --- a/packages/react-reconciler/src/ReactFiberBeginWork.new.js +++ b/packages/react-reconciler/src/ReactFiberBeginWork.new.js @@ -2355,16 +2355,20 @@ function mountSuspenseFallbackAfterRetryWithoutHydrating( fallbackChildren, renderLanes, ) { - const mode = workInProgress.mode; + const fiberMode = workInProgress.mode; + const primaryChildProps: OffscreenProps = { + mode: 'visible', + children: primaryChildren, + }; const primaryChildFragment = createFiberFromOffscreen( - primaryChildren, - mode, + primaryChildProps, + fiberMode, NoLanes, null, ); const fallbackChildFragment = createFiberFromFragment( fallbackChildren, - mode, + fiberMode, renderLanes, null, ); diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.old.js b/packages/react-reconciler/src/ReactFiberBeginWork.old.js index fa0a9840e758f..938848317090e 100644 --- a/packages/react-reconciler/src/ReactFiberBeginWork.old.js +++ b/packages/react-reconciler/src/ReactFiberBeginWork.old.js @@ -2355,16 +2355,20 @@ function mountSuspenseFallbackAfterRetryWithoutHydrating( fallbackChildren, renderLanes, ) { - const mode = workInProgress.mode; + const fiberMode = workInProgress.mode; + const primaryChildProps: OffscreenProps = { + mode: 'visible', + children: primaryChildren, + }; const primaryChildFragment = createFiberFromOffscreen( - primaryChildren, - mode, + primaryChildProps, + fiberMode, NoLanes, null, ); const fallbackChildFragment = createFiberFromFragment( fallbackChildren, - mode, + fiberMode, renderLanes, null, ); From 1d8eac964794f674dd9da9fbde1713a6453d61aa Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Wed, 21 Jul 2021 20:51:16 -0400 Subject: [PATCH 2/3] Extract createOffscreenFromFiber logic ...into a new method called `createWorkInProgressOffscreenFiber`. Just for symmetry with `updateWorkInProgressOffscreenFiber`. Doesn't change any behavior. --- .../src/ReactFiberBeginWork.new.js | 29 ++++++++++++------- .../src/ReactFiberBeginWork.old.js | 29 ++++++++++++------- 2 files changed, 36 insertions(+), 22 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.new.js b/packages/react-reconciler/src/ReactFiberBeginWork.new.js index fc6b24fa36639..adce432d04a79 100644 --- a/packages/react-reconciler/src/ReactFiberBeginWork.new.js +++ b/packages/react-reconciler/src/ReactFiberBeginWork.new.js @@ -9,8 +9,8 @@ import type {ReactProviderType, ReactContext} from 'shared/ReactTypes'; import type {LazyComponent as LazyComponentType} from 'react/src/ReactLazy'; -import type {Fiber} from './ReactInternalTypes'; -import type {FiberRoot} from './ReactInternalTypes'; +import type {Fiber, FiberRoot} from './ReactInternalTypes'; +import type {TypeOfMode} from './ReactTypeOfMode'; import type {Lanes, Lane} from './ReactFiberLane.new'; import type {MutableSource} from 'shared/ReactTypes'; import type { @@ -2112,11 +2112,10 @@ function mountSuspensePrimaryChildren( mode: 'visible', children: primaryChildren, }; - const primaryChildFragment = createFiberFromOffscreen( + const primaryChildFragment = mountWorkInProgressOffscreenFiber( primaryChildProps, mode, renderLanes, - null, ); primaryChildFragment.return = workInProgress; workInProgress.child = primaryChildFragment; @@ -2167,11 +2166,10 @@ function mountSuspenseFallbackChildren( null, ); } else { - primaryChildFragment = createFiberFromOffscreen( + primaryChildFragment = mountWorkInProgressOffscreenFiber( primaryChildProps, mode, NoLanes, - null, ); fallbackChildFragment = createFiberFromFragment( fallbackChildren, @@ -2188,7 +2186,17 @@ function mountSuspenseFallbackChildren( return fallbackChildFragment; } -function createWorkInProgressOffscreenFiber( +function mountWorkInProgressOffscreenFiber( + offscreenProps: OffscreenProps, + mode: TypeOfMode, + renderLanes: Lanes, +) { + // The props argument to `createFiberFromOffscreen` is `any` typed, so we use + // this wrapper function to constrain it. + return createFiberFromOffscreen(offscreenProps, mode, NoLanes, null); +} + +function updateWorkInProgressOffscreenFiber( current: Fiber, offscreenProps: OffscreenProps, ) { @@ -2207,7 +2215,7 @@ function updateSuspensePrimaryChildren( const currentFallbackChildFragment: Fiber | null = currentPrimaryChildFragment.sibling; - const primaryChildFragment = createWorkInProgressOffscreenFiber( + const primaryChildFragment = updateWorkInProgressOffscreenFiber( currentPrimaryChildFragment, { mode: 'visible', @@ -2287,7 +2295,7 @@ function updateSuspenseFallbackChildren( // to delete it. workInProgress.deletions = null; } else { - primaryChildFragment = createWorkInProgressOffscreenFiber( + primaryChildFragment = updateWorkInProgressOffscreenFiber( currentPrimaryChildFragment, primaryChildProps, ); @@ -2360,11 +2368,10 @@ function mountSuspenseFallbackAfterRetryWithoutHydrating( mode: 'visible', children: primaryChildren, }; - const primaryChildFragment = createFiberFromOffscreen( + const primaryChildFragment = mountWorkInProgressOffscreenFiber( primaryChildProps, fiberMode, NoLanes, - null, ); const fallbackChildFragment = createFiberFromFragment( fallbackChildren, diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.old.js b/packages/react-reconciler/src/ReactFiberBeginWork.old.js index 938848317090e..df061e0b22583 100644 --- a/packages/react-reconciler/src/ReactFiberBeginWork.old.js +++ b/packages/react-reconciler/src/ReactFiberBeginWork.old.js @@ -9,8 +9,8 @@ import type {ReactProviderType, ReactContext} from 'shared/ReactTypes'; import type {LazyComponent as LazyComponentType} from 'react/src/ReactLazy'; -import type {Fiber} from './ReactInternalTypes'; -import type {FiberRoot} from './ReactInternalTypes'; +import type {Fiber, FiberRoot} from './ReactInternalTypes'; +import type {TypeOfMode} from './ReactTypeOfMode'; import type {Lanes, Lane} from './ReactFiberLane.old'; import type {MutableSource} from 'shared/ReactTypes'; import type { @@ -2112,11 +2112,10 @@ function mountSuspensePrimaryChildren( mode: 'visible', children: primaryChildren, }; - const primaryChildFragment = createFiberFromOffscreen( + const primaryChildFragment = mountWorkInProgressOffscreenFiber( primaryChildProps, mode, renderLanes, - null, ); primaryChildFragment.return = workInProgress; workInProgress.child = primaryChildFragment; @@ -2167,11 +2166,10 @@ function mountSuspenseFallbackChildren( null, ); } else { - primaryChildFragment = createFiberFromOffscreen( + primaryChildFragment = mountWorkInProgressOffscreenFiber( primaryChildProps, mode, NoLanes, - null, ); fallbackChildFragment = createFiberFromFragment( fallbackChildren, @@ -2188,7 +2186,17 @@ function mountSuspenseFallbackChildren( return fallbackChildFragment; } -function createWorkInProgressOffscreenFiber( +function mountWorkInProgressOffscreenFiber( + offscreenProps: OffscreenProps, + mode: TypeOfMode, + renderLanes: Lanes, +) { + // The props argument to `createFiberFromOffscreen` is `any` typed, so we use + // this wrapper function to constrain it. + return createFiberFromOffscreen(offscreenProps, mode, NoLanes, null); +} + +function updateWorkInProgressOffscreenFiber( current: Fiber, offscreenProps: OffscreenProps, ) { @@ -2207,7 +2215,7 @@ function updateSuspensePrimaryChildren( const currentFallbackChildFragment: Fiber | null = currentPrimaryChildFragment.sibling; - const primaryChildFragment = createWorkInProgressOffscreenFiber( + const primaryChildFragment = updateWorkInProgressOffscreenFiber( currentPrimaryChildFragment, { mode: 'visible', @@ -2287,7 +2295,7 @@ function updateSuspenseFallbackChildren( // to delete it. workInProgress.deletions = null; } else { - primaryChildFragment = createWorkInProgressOffscreenFiber( + primaryChildFragment = updateWorkInProgressOffscreenFiber( currentPrimaryChildFragment, primaryChildProps, ); @@ -2360,11 +2368,10 @@ function mountSuspenseFallbackAfterRetryWithoutHydrating( mode: 'visible', children: primaryChildren, }; - const primaryChildFragment = createFiberFromOffscreen( + const primaryChildFragment = mountWorkInProgressOffscreenFiber( primaryChildProps, fiberMode, NoLanes, - null, ); const fallbackChildFragment = createFiberFromFragment( fallbackChildren, From 4a93531139e26433372b09cad7a455114badd835 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Sun, 25 Jul 2021 16:03:11 -0400 Subject: [PATCH 3/3] [Fabric] Use container node to hide/show tree MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This changes how we hide and show the contents of Offscreen boundaries in the React Fabric renderer (persistent mode), and also Suspense boundaries which use the same feature.= The way it used to work was that when a boundary is hidden, in the complete phase, instead of calling the normal `cloneInstance` method inside `appendAllChildren`, we would call a forked method called `cloneHiddenInstance` for each of the nearest host nodes within the subtree. This design was largely based on how it works in React DOM (mutation mode), where instead of cloning the nearest host nodes, we mutate their `style.display` property. The motivation for doing it this way in React DOM was because there's no built-in browser API for hiding a collection of DOM nodes without affecting their layout. In Fabric, however, there is no such limitation, so we can instead wrap in an extra host node and apply a hidden style. The immediate motivation for this change is that Fabric on Android has a view pooling mechanism for instances that relies on the assumption that a current Fiber that is cloned and replaced by a new Fiber will never appear in a future commit. When this assumption is broken, it may cause crashes. In the current implementation, that can indeed happen when a node that was previously hidden is toggled back to visible. Although this change sidesteps the issue, we may introduce in other features in the future that would benefit from being able to revert back to an older node without cloning it again, such as animations. The way I've implemented this is to insert an additional HostComponent fiber as the child of each OffscreenComponent. The extra fiber is not ideal — the way I'd prefer to do it is to attach the host instance to the OffscreenComponent. However, the native Fabric implementation currently expects a 1:1 correspondence between HostComponents and host instances, so I've deferred that optimization to a future PR to derisk fixing the Fabric pooling crash. I left a TODO in the host config with a description of the remaining steps, but this alone should be sufficient to unblock. --- .../src/ReactFabricHostConfig.js | 50 +++-- .../src/createReactNoop.js | 200 ++++++++++++++---- .../react-reconciler/src/ReactFiber.new.js | 25 ++- .../react-reconciler/src/ReactFiber.old.js | 25 ++- .../src/ReactFiberBeginWork.new.js | 126 ++++++++++- .../src/ReactFiberBeginWork.old.js | 126 ++++++++++- .../src/ReactFiberCompleteWork.new.js | 135 ++++++------ .../src/ReactFiberCompleteWork.old.js | 135 ++++++------ .../ReactFiberHostConfigWithNoPersistence.js | 4 +- .../src/ReactFiberOffscreenComponent.js | 4 +- .../src/forks/ReactFiberHostConfig.custom.js | 6 +- packages/shared/ReactTypes.js | 5 + 12 files changed, 632 insertions(+), 209 deletions(-) diff --git a/packages/react-native-renderer/src/ReactFabricHostConfig.js b/packages/react-native-renderer/src/ReactFabricHostConfig.js index 842a33291fb07..39058e9ac5a31 100644 --- a/packages/react-native-renderer/src/ReactFabricHostConfig.js +++ b/packages/react-native-renderer/src/ReactFabricHostConfig.js @@ -7,6 +7,7 @@ * @flow */ +import type {ReactNodeList, OffscreenMode} from 'shared/ReactTypes'; import type {ElementRef} from 'react'; import type { HostComponent, @@ -301,6 +302,9 @@ export function getChildHostContext( type === 'RCTText' || type === 'RCTVirtualText'; + // TODO: If this is an offscreen host container, we should reuse the + // parent context. + if (prevIsInAParentText !== isInAParentText) { return {isInAParentText}; } else { @@ -413,30 +417,32 @@ export function cloneInstance( }; } -export function cloneHiddenInstance( - instance: Instance, - type: string, - props: Props, - internalInstanceHandle: Object, -): Instance { - const viewConfig = instance.canonical.viewConfig; - const node = instance.node; - const updatePayload = create( - {style: {display: 'none'}}, - viewConfig.validAttributes, - ); - return { - node: cloneNodeWithNewProps(node, updatePayload), - canonical: instance.canonical, - }; +// TODO: These two methods should be replaced with `createOffscreenInstance` and +// `cloneOffscreenInstance`. I did it this way for now because the offscreen +// instance is stored on an extra HostComponent fiber instead of the +// OffscreenComponent fiber, and I didn't want to add an extra check to the +// generic HostComponent path. Instead we should use the OffscreenComponent +// fiber, but currently Fabric expects a 1:1 correspondence between Fabric +// instances and host fibers, so I'm leaving this optimization for later once +// we can confirm this won't break any downstream expectations. +export function getOffscreenContainerType(): string { + return 'RCTView'; } -export function cloneHiddenTextInstance( - instance: Instance, - text: string, - internalInstanceHandle: Object, -): TextInstance { - throw new Error('Not yet implemented.'); +export function getOffscreenContainerProps( + mode: OffscreenMode, + children: ReactNodeList, +): Props { + if (mode === 'hidden') { + return { + children, + style: {display: 'none'}, + }; + } else { + return { + children, + }; + } } export function createContainerChildSet(container: Container): ChildSet { diff --git a/packages/react-noop-renderer/src/createReactNoop.js b/packages/react-noop-renderer/src/createReactNoop.js index 652f32521c249..f6af7341c2e0c 100644 --- a/packages/react-noop-renderer/src/createReactNoop.js +++ b/packages/react-noop-renderer/src/createReactNoop.js @@ -16,7 +16,7 @@ import type {Fiber} from 'react-reconciler/src/ReactInternalTypes'; import type {UpdateQueue} from 'react-reconciler/src/ReactUpdateQueue'; -import type {ReactNodeList} from 'shared/ReactTypes'; +import type {ReactNodeList, OffscreenMode} from 'shared/ReactTypes'; import type {RootTag} from 'react-reconciler/src/ReactRootTags'; import * as Scheduler from 'scheduler/unstable_mock'; @@ -258,6 +258,9 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { type: string, rootcontainerInstance: Container, ) { + if (type === 'offscreen') { + return parentHostContext; + } if (type === 'uppercase') { return UPPERCASE_CONTEXT; } @@ -539,47 +542,18 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { container.children = newChildren; }, - cloneHiddenInstance( - instance: Instance, - type: string, - props: Props, - internalInstanceHandle: Object, - ): Instance { - const clone = cloneInstance( - instance, - null, - type, - props, - props, - internalInstanceHandle, - true, - null, - ); - clone.hidden = true; - return clone; + getOffscreenContainerType(): string { + return 'offscreen'; }, - cloneHiddenTextInstance( - instance: TextInstance, - text: string, - internalInstanceHandle: Object, - ): TextInstance { - const clone = { - text: instance.text, - id: instanceCounter++, - hidden: true, - context: instance.context, + getOffscreenContainerProps( + mode: OffscreenMode, + children: ReactNodeList, + ): Props { + return { + hidden: mode === 'hidden', + children, }; - // Hide from unit tests - Object.defineProperty(clone, 'id', { - value: clone.id, - enumerable: false, - }); - Object.defineProperty(clone, 'context', { - value: clone.context, - enumerable: false, - }); - return clone; }, }; @@ -646,7 +620,9 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { function getChildren(root) { if (root) { - return root.children; + return useMutation + ? root.children + : removeOffscreenContainersFromChildren(root.children, false); } else { return null; } @@ -654,12 +630,154 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { function getPendingChildren(root) { if (root) { - return root.pendingChildren; + return useMutation + ? root.children + : removeOffscreenContainersFromChildren(root.pendingChildren, false); } else { return null; } } + function removeOffscreenContainersFromChildren(children, hideNearestNode) { + // Mutation mode and persistent mode have different outputs for Offscreen + // and Suspense trees. Persistent mode adds an additional host node wrapper, + // whereas mutation mode does not. + // + // This function removes the offscreen host wrappers so that the output is + // consistent. If the offscreen node is hidden, it transfers the hiddenness + // to the child nodes, to mimic how it works in mutation mode. That way our + // tests don't have to fork tree assertions. + // + // So, it takes a tree that looks like this: + // + //