Skip to content

Commit

Permalink
Fix asserts caused by OffscreenComponent rendering in React Native wi…
Browse files Browse the repository at this point in the history
…th passChildrenWhenCloningPersistedNodes (#32528)

<!--
  Thanks for submitting a pull request!
We appreciate you spending the time to work on these changes. Please
provide enough information so that others can review your pull request.
The three fields below are mandatory.

Before submitting a pull request, please make sure the following is
done:

1. Fork [the repository](https://github.com/facebook/react) and create
your branch from `main`.
  2. Run `yarn` in the repository root.
3. If you've fixed a bug or added code that should be tested, add tests!
4. Ensure the test suite passes (`yarn test`). Tip: `yarn test --watch
TestName` is helpful in development.
5. Run `yarn test --prod` to test in the production environment. It
supports the same options as `yarn test`.
6. If you need a debugger, run `yarn test --debug --watch TestName`,
open `chrome://inspect`, and press "Inspect".
7. Format your code with
[prettier](https://github.com/prettier/prettier) (`yarn prettier`).
8. Make sure your code lints (`yarn lint`). Tip: `yarn linc` to only
check changed files.
  9. Run the [Flow](https://flowtype.org/) type checks (`yarn flow`).
  10. If you haven't already, complete the CLA.

Learn more about contributing:
https://reactjs.org/docs/how-to-contribute.html
-->

## Summary

<!--
Explain the **motivation** for making this change. What existing problem
does the pull request solve?
-->

This PR fixes asserts when `passChildrenWhenCloningPersistedNodes` is
enabled for React Native and OffscreenComponent child rendering unhides
host components.

Discussions around possible fixes for the asserts seen in React Native
suggested changing the way we handle hiding/unhiding host components by
updating the fiber state with the hidden host component instead of
submitting a hidden clone Fabric and keeping the original as the current
fiber.

Implementing this fix would require holding onto the original styling of
the hidden host component. The reconciler updates the styling by adding
`display: none` to hide the contents. If the original host component was
already hidden, the renderer would lose that information and remove the
styling when showing the contents again.

To reduce the changes required to make
`passChildrenWhenCloningPersistedNodes` work, this PR falls back to the
original cloning method when OffscreenComponents are part of the
children needed to be added back. This effectively resolve the asserts
triggered by the feature in RN and improves overall performance.

## How did you test this change?

<!--
Demonstrate the code is solid. Example: The exact commands you ran and
their output, screenshots / videos if the pull request changes the user
interface.
How exactly did you verify that your PR solves the issue you wanted to
solve?
  If you leave this empty, your PR will very likely be closed.
-->

This fix was tested by enabling `passChildrenWhenCloningPersistedNodes`
in an app built with React Native that had a repro for triggering the
asserts. The asserts do not occur anymore when using the changes in this
PR.

---------

Co-authored-by: Nick <[email protected]>
  • Loading branch information
lenaic and Nick authored Mar 7, 2025
1 parent f9d7808 commit cc68006
Showing 1 changed file with 19 additions and 6 deletions.
25 changes: 19 additions & 6 deletions packages/react-reconciler/src/ReactFiberCompleteWork.js
Original file line number Diff line number Diff line change
Expand Up @@ -341,7 +341,12 @@ function appendAllChildrenToContainer(
workInProgress: Fiber,
needsVisibilityToggle: boolean,
isHidden: boolean,
) {
): boolean {
// Host components that have their visibility toggled by an OffscreenComponent
// do not support passChildrenWhenCloningPersistedNodes. To inform the callee
// about their presence, we track and return if they were added to the
// child set.
let hasOffscreenComponentChild = false;
if (supportsPersistence) {
// We only have the top Fiber that was created but we need recurse down its
// children to find all the terminal nodes.
Expand Down Expand Up @@ -386,20 +391,22 @@ function appendAllChildrenToContainer(
/* needsVisibilityToggle */ _needsVisibilityToggle,
/* isHidden */ true,
);

hasOffscreenComponentChild = true;
} else if (node.child !== null) {
node.child.return = node;
node = node.child;
continue;
}
node = (node: Fiber);
if (node === workInProgress) {
return;
return hasOffscreenComponentChild;
}
// $FlowFixMe[incompatible-use] found when upgrading Flow
while (node.sibling === null) {
// $FlowFixMe[incompatible-use] found when upgrading Flow
if (node.return === null || node.return === workInProgress) {
return;
return hasOffscreenComponentChild;
}
node = node.return;
}
Expand All @@ -408,6 +415,8 @@ function appendAllChildrenToContainer(
node = node.sibling;
}
}

return hasOffscreenComponentChild;
}

function updateHostContainer(current: null | Fiber, workInProgress: Fiber) {
Expand Down Expand Up @@ -468,11 +477,12 @@ function updateHostComponent(
const currentHostContext = getHostContext();

let newChildSet = null;
let hasOffscreenComponentChild = false;
if (requiresClone && passChildrenWhenCloningPersistedNodes) {
markCloned(workInProgress);
newChildSet = createContainerChildSet();
// If children might have changed, we have to add them all to the set.
appendAllChildrenToContainer(
hasOffscreenComponentChild = appendAllChildrenToContainer(
newChildSet,
workInProgress,
/* needsVisibilityToggle */ false,
Expand All @@ -486,7 +496,7 @@ function updateHostComponent(
oldProps,
newProps,
!requiresClone,
newChildSet,
!hasOffscreenComponentChild ? newChildSet : undefined,
);
if (newInstance === currentInstance) {
// No changes, just reuse the existing instance.
Expand All @@ -513,7 +523,10 @@ function updateHostComponent(
// Otherwise parents won't know that there are new children to propagate upwards.
markUpdate(workInProgress);
}
} else if (!passChildrenWhenCloningPersistedNodes) {
} else if (
!passChildrenWhenCloningPersistedNodes ||
hasOffscreenComponentChild
) {
// If children have changed, we have to add them all to the set.
appendAllChildren(
newInstance,
Expand Down

0 comments on commit cc68006

Please sign in to comment.