Clean-up: Move Offscreen logic from Suspense fiber (#21925)

acdlite · web-flow · commit 9ab90de60235 · 2021-07-20T16:05:16.000-07:00
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.
diff --git 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
@@ -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
@@ -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
@@ -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
