Repository: facebook/react
Pull request: Clean-up: Move visibility toggling logic from Suspense fiber to Offscreen fiber

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 implementation 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.

Changed files:
filename: packages/react-reconciler/src/ReactFiberCommitWork.new.js
patch: @@ -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;

filename: packages/react-reconciler/src/ReactFiberCommitWork.old.js
patch: @@ -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;

filename: packages/react-reconciler/src/ReactFiberCompleteWork.new.js
patch: @@ -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 &&

filename: packages/react-reconciler/src/ReactFiberCompleteWork.old.js
patch: @@ -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 &&

filename: packages/react-reconciler/src/__tests__/ReactOffscreen-test.js
patch: @@ -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(<span prop="Child" />);
+    if (gate(flags => flags.persistent)) {
+      expect(root).toMatchRenderedOutput(<span hidden={true} prop="Child" />);
+    } else {
+      // TODO: Offscreen does not yet hide/unhide children correctly in mutation
+      // mode. Until we do, it should only be used inside a host component
+      // wrapper whose visibility is toggled simultaneously.
+      expect(root).toMatchRenderedOutput(<span prop="Child" />);
+    }
 
     // Unhide the tree. The layout effect is mounted.
     await act(async () => {
@@ -251,10 +255,14 @@ describe('ReactOffscreen', () => {
       );
     });
     expect(Scheduler).toHaveYielded(['Unmount layout', '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(<span prop="Child" />);
+    if (gate(flags => flags.persistent)) {
+      expect(root).toMatchRenderedOutput(<span hidden={true} prop="Child" />);
+    } else {
+      // TODO: Offscreen does not yet hide/unhide children correctly in mutation
+      // mode. Until we do, it should only be used inside a host component
+      // wrapper whose visibility is toggled simultaneously.
+      expect(root).toMatchRenderedOutput(<span prop="Child" />);
+    }
 
     // Unhide the tree. The layout effect is re-mounted.
     await act(async () => {
@@ -291,10 +299,14 @@ describe('ReactOffscreen', () => {
       );
     });
     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(<span prop="Child" />);
+    if (gate(flags => flags.persistent)) {
+      expect(root).toMatchRenderedOutput(<span hidden={true} prop="Child" />);
+    } else {
+      // TODO: Offscreen does not yet hide/unhide children correctly in mutation
+      // mode. Until we do, it should only be used inside a host component
+      // wrapper whose visibility is toggled simultaneously.
+      expect(root).toMatchRenderedOutput(<span prop="Child" />);
+    }
 
     // Show the tree. The layout effect is mounted.
     await act(async () => {
@@ -316,9 +328,13 @@ describe('ReactOffscreen', () => {
       );
     });
     expect(Scheduler).toHaveYielded(['Unmount layout', '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(<span prop="Child" />);
+    if (gate(flags => flags.persistent)) {
+      expect(root).toMatchRenderedOutput(<span hidden={true} prop="Child" />);
+    } else {
+      // TODO: Offscreen does not yet hide/unhide children correctly in mutation
+      // mode. Until we do, it should only be used inside a host component
+      // wrapper whose visibility is toggled simultaneously.
+      expect(root).toMatchRenderedOutput(<span prop="Child" />);
+    }
   });
 });