From 86b97aa0082c2313582eb8fd7ed86ad43780f2f6 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Tue, 20 Jul 2021 19:05:16 -0400 Subject: [PATCH] Clean-up: Move Offscreen logic from Suspense fiber (#21925) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Much of the visibility-toggling logic is shared between the Suspense and Offscreen types, but there is some duplicated code that exists in both. Specifically, when a Suspense fiber's state switches from suspended to resolved, we schedule an effect on the parent Suspense fiber, rather than the inner Offscreen fiber. Then in the commit phase, the Suspense fiber is responsible for committing the visibility effect on Offscreen. There two main reasons we implemented it this way, neither of which apply any more: - The inner Offscreen fiber that wraps around the Suspense children used to be conditionally added only when the boundary was in its fallback state. So when toggling back to visible, there was no inner fiber to handle the visibility effect. This is no longer the case — the primary children are always wrapped in an Offscreen fiber. - When the Suspense fiber is in its fallback state, the inner Offscreen fiber does not have a complete phase, because we bail out of rendering that tree. In the old effects list implementation, that meant the Offscreen fiber did not get added to the effect list, so it didn't have a commit phase. In the new recursive effects implementation, there's no list to maintain. Marking a flag on the inner fiber is sufficient to schedule a commit effect. Given that these are no relevant, I was able to remove a lot of old code and shift more of the logic out of the Suspense implementation and into the Offscreen implementation so that it is shared by both. (Being able to share the implementaiton like this was in fact one of the reasons we stopped conditionally removing the inner Offscreen fiber.) As a bonus, this happens to fix a TODO in the Offscreen implementation for persistent (Fabric) mode, where newly inserted nodes inside an already hidden tree must also be hidden. Though we'll still need to make this work in mutation (DOM) mode, too. --- .../src/ReactFiberCommitWork.new.js | 28 +---- .../src/ReactFiberCommitWork.old.js | 28 +---- .../src/ReactFiberCompleteWork.new.js | 114 ++++++------------ .../src/ReactFiberCompleteWork.old.js | 114 ++++++------------ .../src/__tests__/ReactOffscreen-test.js | 48 +++++--- 5 files changed, 106 insertions(+), 226 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js index 99161bd97f487..a68e489dccc98 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 8fcdb5920756e..aea3e36ed09d2 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 c919f15222465..32b9587f26314 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 59d72380dbeeb..60359fd42d8de 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 327e72fdc9724..2915b1e769f04 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(