Skip to content

Commit cc68006

Browse files
lenaicNick
andauthored
Fix asserts caused by OffscreenComponent rendering in React Native with 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]>
1 parent f9d7808 commit cc68006

File tree

1 file changed

+19
-6
lines changed

1 file changed

+19
-6
lines changed

packages/react-reconciler/src/ReactFiberCompleteWork.js

Lines changed: 19 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -341,7 +341,12 @@ function appendAllChildrenToContainer(
341341
workInProgress: Fiber,
342342
needsVisibilityToggle: boolean,
343343
isHidden: boolean,
344-
) {
344+
): boolean {
345+
// Host components that have their visibility toggled by an OffscreenComponent
346+
// do not support passChildrenWhenCloningPersistedNodes. To inform the callee
347+
// about their presence, we track and return if they were added to the
348+
// child set.
349+
let hasOffscreenComponentChild = false;
345350
if (supportsPersistence) {
346351
// We only have the top Fiber that was created but we need recurse down its
347352
// children to find all the terminal nodes.
@@ -386,20 +391,22 @@ function appendAllChildrenToContainer(
386391
/* needsVisibilityToggle */ _needsVisibilityToggle,
387392
/* isHidden */ true,
388393
);
394+
395+
hasOffscreenComponentChild = true;
389396
} else if (node.child !== null) {
390397
node.child.return = node;
391398
node = node.child;
392399
continue;
393400
}
394401
node = (node: Fiber);
395402
if (node === workInProgress) {
396-
return;
403+
return hasOffscreenComponentChild;
397404
}
398405
// $FlowFixMe[incompatible-use] found when upgrading Flow
399406
while (node.sibling === null) {
400407
// $FlowFixMe[incompatible-use] found when upgrading Flow
401408
if (node.return === null || node.return === workInProgress) {
402-
return;
409+
return hasOffscreenComponentChild;
403410
}
404411
node = node.return;
405412
}
@@ -408,6 +415,8 @@ function appendAllChildrenToContainer(
408415
node = node.sibling;
409416
}
410417
}
418+
419+
return hasOffscreenComponentChild;
411420
}
412421

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

470479
let newChildSet = null;
480+
let hasOffscreenComponentChild = false;
471481
if (requiresClone && passChildrenWhenCloningPersistedNodes) {
472482
markCloned(workInProgress);
473483
newChildSet = createContainerChildSet();
474484
// If children might have changed, we have to add them all to the set.
475-
appendAllChildrenToContainer(
485+
hasOffscreenComponentChild = appendAllChildrenToContainer(
476486
newChildSet,
477487
workInProgress,
478488
/* needsVisibilityToggle */ false,
@@ -486,7 +496,7 @@ function updateHostComponent(
486496
oldProps,
487497
newProps,
488498
!requiresClone,
489-
newChildSet,
499+
!hasOffscreenComponentChild ? newChildSet : undefined,
490500
);
491501
if (newInstance === currentInstance) {
492502
// No changes, just reuse the existing instance.
@@ -513,7 +523,10 @@ function updateHostComponent(
513523
// Otherwise parents won't know that there are new children to propagate upwards.
514524
markUpdate(workInProgress);
515525
}
516-
} else if (!passChildrenWhenCloningPersistedNodes) {
526+
} else if (
527+
!passChildrenWhenCloningPersistedNodes ||
528+
hasOffscreenComponentChild
529+
) {
517530
// If children have changed, we have to add them all to the set.
518531
appendAllChildren(
519532
newInstance,

0 commit comments

Comments
 (0)