Skip to content

Commit

Permalink
[Fabric] Use container node to hide/show tree
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
acdlite committed Jul 26, 2021
1 parent 7888e48 commit 7a01271
Show file tree
Hide file tree
Showing 12 changed files with 614 additions and 211 deletions.
46 changes: 24 additions & 22 deletions packages/react-native-renderer/src/ReactFabricHostConfig.js
Original file line number Diff line number Diff line change
Expand Up @@ -413,30 +413,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 'RTCView';
}

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 {
Expand Down
188 changes: 145 additions & 43 deletions packages/react-noop-renderer/src/createReactNoop.js
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -62,8 +62,8 @@ type TextInstance = {|
|};
type HostContext = Object;

const NO_CONTEXT = {};
const UPPERCASE_CONTEXT = {};
const NO_CONTEXT = {uppercase: false};
const UPPERCASE_CONTEXT = {uppercase: true};
const UPDATE_SIGNAL = {};
if (__DEV__) {
Object.freeze(NO_CONTEXT);
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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;
},
};

Expand Down Expand Up @@ -646,20 +620,148 @@ 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;
}
}

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 in both modes. That way our tests don't have to fork tree
// assertions based on the renderer mode.
//
// We don't mutate the original tree, but instead return a copy. This
// function is only used in our test assertions.
let didClone = false;
const newChildren = [];
for (let i = 0; i < children.length; i++) {
const child = children[i];
const innerChildren = child.children;
if (innerChildren !== undefined) {
// This is a host instance instance
const instance: Instance = (child: any);
if (instance.type === 'offscreen') {
// This is an offscreen wrapper instance. Remove it from the tree
// and recursively return its children, as if it were a fragment.
didClone = true;
if (instance.text !== null) {
// If this offscreen tree contains only text, we replace it with
// a text child. Related to `shouldReplaceTextContent` feature.
const offscreenTextInstance: TextInstance = {
text: instance.text,
id: instanceCounter++,
hidden: hideNearestNode || instance.hidden,
context: instance.context,
};
// Hide from unit tests
Object.defineProperty(offscreenTextInstance, 'id', {
value: offscreenTextInstance.id,
enumerable: false,
});
Object.defineProperty(offscreenTextInstance, 'context', {
value: offscreenTextInstance.context,
enumerable: false,
});
newChildren.push(offscreenTextInstance);
} else {
// Skip the offscreen node and replace it with its children
const offscreenChildren = removeOffscreenContainersFromChildren(
innerChildren,
hideNearestNode || instance.hidden,
);
newChildren.push.apply(newChildren, offscreenChildren);
}
} else {
// This is a regular (non-offscreen) instance. If the nearest
// offscreen boundary is hidden, hide this node.
const hidden = hideNearestNode ? true : instance.hidden;
const clonedChildren = removeOffscreenContainersFromChildren(
instance.children,
// We never need to hide the children of this node, since if we're
// inside a hidden tree, then the hidden style will be applied to
// this node.
false,
);
if (
clonedChildren === instance.children &&
hidden === instance.hidden
) {
// No changes. Reuse the original instance without cloning.
newChildren.push(instance);
} else {
didClone = true;
const clone: Instance = {
id: instance.id,
type: instance.type,
children: clonedChildren,
text: instance.text,
prop: instance.prop,
hidden: hideNearestNode ? true : instance.hidden,
context: instance.context,
};
Object.defineProperty(clone, 'id', {
value: clone.id,
enumerable: false,
});
Object.defineProperty(clone, 'text', {
value: clone.text,
enumerable: false,
});
Object.defineProperty(clone, 'context', {
value: clone.context,
enumerable: false,
});
newChildren.push(clone);
}
}
} else {
// This is a text instance
const textInstance: TextInstance = (child: any);
if (hideNearestNode) {
didClone = true;
const clone = {
text: textInstance.text,
id: textInstance.id,
hidden: textInstance.hidden || hideNearestNode,
context: textInstance.context,
};
Object.defineProperty(clone, 'id', {
value: clone.id,
enumerable: false,
});
Object.defineProperty(clone, 'context', {
value: clone.context,
enumerable: false,
});

newChildren.push(clone);
} else {
newChildren.push(textInstance);
}
}
}
// There are some tests that assume reference equality, so preserve it
// when possible. An alternative would
return didClone ? newChildren : children;
}

function getChildrenAsJSX(root) {
const children = childToJSX(getChildren(root), null);
if (children === null) {
Expand Down
25 changes: 24 additions & 1 deletion packages/react-reconciler/src/ReactFiber.new.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import type {RootTag} from './ReactRootTags';
import type {WorkTag} from './ReactWorkTags';
import type {TypeOfMode} from './ReactTypeOfMode';
import type {Lanes} from './ReactFiberLane.new';
import type {SuspenseInstance} from './ReactFiberHostConfig';
import type {SuspenseInstance, Props} from './ReactFiberHostConfig';
import type {OffscreenProps} from './ReactFiberOffscreenComponent';

import invariant from 'shared/invariant';
Expand All @@ -27,6 +27,10 @@ import {
enableSyncDefaultUpdates,
allowConcurrentByDefault,
} from 'shared/ReactFeatureFlags';
import {
supportsPersistence,
getOffscreenContainerType,
} from './ReactFiberHostConfig';
import {NoFlags, Placement, StaticMask} from './ReactFiberFlags';
import {ConcurrentRoot} from './ReactRootTags';
import {
Expand Down Expand Up @@ -585,6 +589,25 @@ export function createFiberFromTypeAndProps(
return fiber;
}

export function createOffscreenHostContainerFiber(
props: Props,
fiberMode: TypeOfMode,
lanes: Lanes,
key: null | string,
): Fiber {
if (supportsPersistence) {
const type = getOffscreenContainerType();
const fiber = createFiber(HostComponent, props, key, fiberMode);
fiber.elementType = type;
fiber.type = type;
fiber.lanes = lanes;
return fiber;
} else {
// Only implemented in persistent mode
invariant(false, 'Not implemented.');
}
}

export function createFiberFromElement(
element: ReactElement,
mode: TypeOfMode,
Expand Down
25 changes: 24 additions & 1 deletion packages/react-reconciler/src/ReactFiber.old.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import type {RootTag} from './ReactRootTags';
import type {WorkTag} from './ReactWorkTags';
import type {TypeOfMode} from './ReactTypeOfMode';
import type {Lanes} from './ReactFiberLane.old';
import type {SuspenseInstance} from './ReactFiberHostConfig';
import type {SuspenseInstance, Props} from './ReactFiberHostConfig';
import type {OffscreenProps} from './ReactFiberOffscreenComponent';

import invariant from 'shared/invariant';
Expand All @@ -27,6 +27,10 @@ import {
enableSyncDefaultUpdates,
allowConcurrentByDefault,
} from 'shared/ReactFeatureFlags';
import {
supportsPersistence,
getOffscreenContainerType,
} from './ReactFiberHostConfig';
import {NoFlags, Placement, StaticMask} from './ReactFiberFlags';
import {ConcurrentRoot} from './ReactRootTags';
import {
Expand Down Expand Up @@ -585,6 +589,25 @@ export function createFiberFromTypeAndProps(
return fiber;
}

export function createOffscreenHostContainerFiber(
props: Props,
fiberMode: TypeOfMode,
lanes: Lanes,
key: null | string,
): Fiber {
if (supportsPersistence) {
const type = getOffscreenContainerType();
const fiber = createFiber(HostComponent, props, key, fiberMode);
fiber.elementType = type;
fiber.type = type;
fiber.lanes = lanes;
return fiber;
} else {
// Only implemented in persistent mode
invariant(false, 'Not implemented.');
}
}

export function createFiberFromElement(
element: ReactElement,
mode: TypeOfMode,
Expand Down
Loading

0 comments on commit 7a01271

Please sign in to comment.