diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js index 99161bd97f48..a68e489dccc9 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.new.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.new.js @@ -2121,34 +2121,12 @@ function commitMutationEffectsOnFiber(finishedWork: Fiber, root: FiberRoot) { case SuspenseComponent: { const newState: OffscreenState | null = finishedWork.memoizedState; const isHidden = newState !== null; - const current = finishedWork.alternate; - const wasHidden = current !== null && current.memoizedState !== null; - const offscreenBoundary: Fiber = (finishedWork.child: any); - if (isHidden) { + const current = finishedWork.alternate; + const wasHidden = current !== null && current.memoizedState !== null; if (!wasHidden) { + // TODO: Move to passive phase markCommitTimeOfFallback(); - if (supportsMutation) { - hideOrUnhideAllChildren(offscreenBoundary, true); - } - if ( - enableSuspenseLayoutEffectSemantics && - (offscreenBoundary.mode & ConcurrentMode) !== NoMode - ) { - let offscreenChild = offscreenBoundary.child; - while (offscreenChild !== null) { - nextEffect = offscreenChild; - disappearLayoutEffects_begin(offscreenChild); - offscreenChild = offscreenChild.sibling; - } - } - } - } else { - if (wasHidden) { - if (supportsMutation) { - hideOrUnhideAllChildren(offscreenBoundary, false); - } - // TODO: Move re-appear call here for symmetry? } } break; diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.old.js b/packages/react-reconciler/src/ReactFiberCommitWork.old.js index 8fcdb5920756..aea3e36ed09d 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.old.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.old.js @@ -2121,34 +2121,12 @@ function commitMutationEffectsOnFiber(finishedWork: Fiber, root: FiberRoot) { case SuspenseComponent: { const newState: OffscreenState | null = finishedWork.memoizedState; const isHidden = newState !== null; - const current = finishedWork.alternate; - const wasHidden = current !== null && current.memoizedState !== null; - const offscreenBoundary: Fiber = (finishedWork.child: any); - if (isHidden) { + const current = finishedWork.alternate; + const wasHidden = current !== null && current.memoizedState !== null; if (!wasHidden) { + // TODO: Move to passive phase markCommitTimeOfFallback(); - if (supportsMutation) { - hideOrUnhideAllChildren(offscreenBoundary, true); - } - if ( - enableSuspenseLayoutEffectSemantics && - (offscreenBoundary.mode & ConcurrentMode) !== NoMode - ) { - let offscreenChild = offscreenBoundary.child; - while (offscreenChild !== null) { - nextEffect = offscreenChild; - disappearLayoutEffects_begin(offscreenChild); - offscreenChild = offscreenChild.sibling; - } - } - } - } else { - if (wasHidden) { - if (supportsMutation) { - hideOrUnhideAllChildren(offscreenBoundary, false); - } - // TODO: Move re-appear call here for symmetry? } } break; diff --git a/packages/react-reconciler/src/ReactFiberCompleteWork.new.js b/packages/react-reconciler/src/ReactFiberCompleteWork.new.js index c919f1522246..32b9587f2631 100644 --- a/packages/react-reconciler/src/ReactFiberCompleteWork.new.js +++ b/packages/react-reconciler/src/ReactFiberCompleteWork.new.js @@ -324,37 +324,17 @@ if (supportsMutation) { // If we have a portal child, then we don't want to traverse // down its children. Instead, we'll get insertions from each child in // the portal directly. - } else if (node.tag === SuspenseComponent) { - if ((node.flags & Visibility) !== NoFlags) { - // Need to toggle the visibility of the primary children. - const newIsHidden = node.memoizedState !== null; - if (newIsHidden) { - const primaryChildParent = node.child; - if (primaryChildParent !== null) { - if (primaryChildParent.child !== null) { - primaryChildParent.child.return = primaryChildParent; - appendAllChildren( - parent, - primaryChildParent, - true, - newIsHidden, - ); - } - const fallbackChildParent = primaryChildParent.sibling; - if (fallbackChildParent !== null) { - fallbackChildParent.return = node; - node = fallbackChildParent; - continue; - } - } - } - } - if (node.child !== null) { - // Continue traversing like normal - node.child.return = node; - node = node.child; - continue; + } else if ( + node.tag === OffscreenComponent && + node.memoizedState !== null + ) { + // The children in this boundary are hidden. Toggle their visibility + // before appending. + const child = node.child; + if (child !== null) { + child.return = node; } + appendAllChildren(parent, node, true, true); } else if (node.child !== null) { node.child.return = node; node = node.child; @@ -409,37 +389,17 @@ if (supportsMutation) { // If we have a portal child, then we don't want to traverse // down its children. Instead, we'll get insertions from each child in // the portal directly. - } else if (node.tag === SuspenseComponent) { - if ((node.flags & Visibility) !== NoFlags) { - // Need to toggle the visibility of the primary children. - const newIsHidden = node.memoizedState !== null; - if (newIsHidden) { - const primaryChildParent = node.child; - if (primaryChildParent !== null) { - if (primaryChildParent.child !== null) { - primaryChildParent.child.return = primaryChildParent; - appendAllChildrenToContainer( - containerChildSet, - primaryChildParent, - true, - newIsHidden, - ); - } - const fallbackChildParent = primaryChildParent.sibling; - if (fallbackChildParent !== null) { - fallbackChildParent.return = node; - node = fallbackChildParent; - continue; - } - } - } - } - if (node.child !== null) { - // Continue traversing like normal - node.child.return = node; - node = node.child; - continue; + } else if ( + node.tag === OffscreenComponent && + node.memoizedState !== null + ) { + // The children in this boundary are hidden. Toggle their visibility + // before appending. + const child = node.child; + if (child !== null) { + child.return = node; } + appendAllChildrenToContainer(containerChildSet, node, true, true); } else if (node.child !== null) { node.child.return = node; node = node.child; @@ -1056,7 +1016,21 @@ function completeWork( prevDidTimeout = prevState !== null; } + // If the suspended state of the boundary changes, we need to schedule + // an effect to toggle the subtree's visibility. When we switch from + // fallback -> primary, the inner Offscreen fiber schedules this effect + // as part of its normal complete phase. But when we switch from + // primary -> fallback, the inner Offscreen fiber does not have a complete + // phase. So we need to schedule its effect here. + // + // We also use this flag to connect/disconnect the effects, but the same + // logic applies: when re-connecting, the Offscreen fiber's complete + // phase will handle scheduling the effect. It's only when the fallback + // is active that we have to do anything special. if (nextDidTimeout && !prevDidTimeout) { + const offscreenFiber: Fiber = (workInProgress.child: any); + offscreenFiber.flags |= Visibility; + // TODO: This will still suspend a synchronous tree if anything // in the concurrent tree already suspended during this render. // This is a known bug. @@ -1096,26 +1070,6 @@ function completeWork( workInProgress.flags |= Update; } - if (supportsMutation) { - if (nextDidTimeout !== prevDidTimeout) { - // In mutation mode, visibility is toggled by mutating the nearest - // host nodes whenever they switch from hidden -> visible or vice - // versa. We don't need to switch when the boundary updates but its - // visibility hasn't changed. - workInProgress.flags |= Visibility; - } - } - if (supportsPersistence) { - if (nextDidTimeout) { - // In persistent mode, visibility is toggled by cloning the nearest - // host nodes in the complete phase whenever the boundary is hidden. - // TODO: The plan is to add a transparent host wrapper (no layout) - // around the primary children and hide that node. Then we don't need - // to do the funky cloning business. - workInProgress.flags |= Visibility; - } - } - if ( enableSuspenseCallback && workInProgress.updateQueue !== null && diff --git a/packages/react-reconciler/src/ReactFiberCompleteWork.old.js b/packages/react-reconciler/src/ReactFiberCompleteWork.old.js index 59d72380dbee..60359fd42d8d 100644 --- a/packages/react-reconciler/src/ReactFiberCompleteWork.old.js +++ b/packages/react-reconciler/src/ReactFiberCompleteWork.old.js @@ -324,37 +324,17 @@ if (supportsMutation) { // If we have a portal child, then we don't want to traverse // down its children. Instead, we'll get insertions from each child in // the portal directly. - } else if (node.tag === SuspenseComponent) { - if ((node.flags & Visibility) !== NoFlags) { - // Need to toggle the visibility of the primary children. - const newIsHidden = node.memoizedState !== null; - if (newIsHidden) { - const primaryChildParent = node.child; - if (primaryChildParent !== null) { - if (primaryChildParent.child !== null) { - primaryChildParent.child.return = primaryChildParent; - appendAllChildren( - parent, - primaryChildParent, - true, - newIsHidden, - ); - } - const fallbackChildParent = primaryChildParent.sibling; - if (fallbackChildParent !== null) { - fallbackChildParent.return = node; - node = fallbackChildParent; - continue; - } - } - } - } - if (node.child !== null) { - // Continue traversing like normal - node.child.return = node; - node = node.child; - continue; + } else if ( + node.tag === OffscreenComponent && + node.memoizedState !== null + ) { + // The children in this boundary are hidden. Toggle their visibility + // before appending. + const child = node.child; + if (child !== null) { + child.return = node; } + appendAllChildren(parent, node, true, true); } else if (node.child !== null) { node.child.return = node; node = node.child; @@ -409,37 +389,17 @@ if (supportsMutation) { // If we have a portal child, then we don't want to traverse // down its children. Instead, we'll get insertions from each child in // the portal directly. - } else if (node.tag === SuspenseComponent) { - if ((node.flags & Visibility) !== NoFlags) { - // Need to toggle the visibility of the primary children. - const newIsHidden = node.memoizedState !== null; - if (newIsHidden) { - const primaryChildParent = node.child; - if (primaryChildParent !== null) { - if (primaryChildParent.child !== null) { - primaryChildParent.child.return = primaryChildParent; - appendAllChildrenToContainer( - containerChildSet, - primaryChildParent, - true, - newIsHidden, - ); - } - const fallbackChildParent = primaryChildParent.sibling; - if (fallbackChildParent !== null) { - fallbackChildParent.return = node; - node = fallbackChildParent; - continue; - } - } - } - } - if (node.child !== null) { - // Continue traversing like normal - node.child.return = node; - node = node.child; - continue; + } else if ( + node.tag === OffscreenComponent && + node.memoizedState !== null + ) { + // The children in this boundary are hidden. Toggle their visibility + // before appending. + const child = node.child; + if (child !== null) { + child.return = node; } + appendAllChildrenToContainer(containerChildSet, node, true, true); } else if (node.child !== null) { node.child.return = node; node = node.child; @@ -1056,7 +1016,21 @@ function completeWork( prevDidTimeout = prevState !== null; } + // If the suspended state of the boundary changes, we need to schedule + // an effect to toggle the subtree's visibility. When we switch from + // fallback -> primary, the inner Offscreen fiber schedules this effect + // as part of its normal complete phase. But when we switch from + // primary -> fallback, the inner Offscreen fiber does not have a complete + // phase. So we need to schedule its effect here. + // + // We also use this flag to connect/disconnect the effects, but the same + // logic applies: when re-connecting, the Offscreen fiber's complete + // phase will handle scheduling the effect. It's only when the fallback + // is active that we have to do anything special. if (nextDidTimeout && !prevDidTimeout) { + const offscreenFiber: Fiber = (workInProgress.child: any); + offscreenFiber.flags |= Visibility; + // TODO: This will still suspend a synchronous tree if anything // in the concurrent tree already suspended during this render. // This is a known bug. @@ -1096,26 +1070,6 @@ function completeWork( workInProgress.flags |= Update; } - if (supportsMutation) { - if (nextDidTimeout !== prevDidTimeout) { - // In mutation mode, visibility is toggled by mutating the nearest - // host nodes whenever they switch from hidden -> visible or vice - // versa. We don't need to switch when the boundary updates but its - // visibility hasn't changed. - workInProgress.flags |= Visibility; - } - } - if (supportsPersistence) { - if (nextDidTimeout) { - // In persistent mode, visibility is toggled by cloning the nearest - // host nodes in the complete phase whenever the boundary is hidden. - // TODO: The plan is to add a transparent host wrapper (no layout) - // around the primary children and hide that node. Then we don't need - // to do the funky cloning business. - workInProgress.flags |= Visibility; - } - } - if ( enableSuspenseCallback && workInProgress.updateQueue !== null && diff --git a/packages/react-reconciler/src/__tests__/ReactOffscreen-test.js b/packages/react-reconciler/src/__tests__/ReactOffscreen-test.js index 327e72fdc972..2915b1e769f0 100644 --- a/packages/react-reconciler/src/__tests__/ReactOffscreen-test.js +++ b/packages/react-reconciler/src/__tests__/ReactOffscreen-test.js @@ -201,10 +201,14 @@ describe('ReactOffscreen', () => { }); // No layout effect. expect(Scheduler).toHaveYielded(['Child']); - // TODO: Offscreen does not yet hide/unhide children correctly. Until we do, - // it should only be used inside a host component wrapper whose visibility - // is toggled simultaneously. - expect(root).toMatchRenderedOutput(); + if (gate(flags => flags.persistent)) { + expect(root).toMatchRenderedOutput(