Use Visibility flag to schedule a hide/show effect

Instead of the Update flag, which is also used for other side-effects,
like refs.

I originally added the Visibility flag for this purpose in facebook#20043 but
it got reverted last winter when we were bisecting the effects refactor.

Author: acdlite

packages/react-reconciler/src/ReactFiberCommitWork.new.js

@@ -75,6 +75,7 @@ import {
   MutationMask,
   LayoutMask,
   PassiveMask,
+  Visibility,
 } from './ReactFiberFlags';
 import getComponentNameFromFiber from 'react-reconciler/src/getComponentNameFromFiber';
 import invariant from 'shared/invariant';
@@ -615,7 +616,7 @@ function commitLayoutEffectOnFiber(
   finishedWork: Fiber,
   committedLanes: Lanes,
 ): void {
-  if ((finishedWork.flags & (Update | Callback)) !== NoFlags) {
+  if ((finishedWork.flags & LayoutMask) !== NoFlags) {
     switch (finishedWork.tag) {
       case FunctionComponent:
       case ForwardRef:
@@ -1776,7 +1777,7 @@ function commitWork(current: Fiber | null, finishedWork: Fiber): void {
         return;
       }
       case SuspenseComponent: {
-        commitSuspenseComponent(finishedWork);
+        commitSuspenseCallback(finishedWork);
         attachSuspenseRetryListeners(finishedWork);
         return;
       }
@@ -1899,7 +1900,7 @@ function commitWork(current: Fiber | null, finishedWork: Fiber): void {
       return;
     }
     case SuspenseComponent: {
-      commitSuspenseComponent(finishedWork);
+      commitSuspenseCallback(finishedWork);
       attachSuspenseRetryListeners(finishedWork);
       return;
     }
@@ -1918,13 +1919,6 @@ function commitWork(current: Fiber | null, finishedWork: Fiber): void {
       }
       break;
     }
-    case OffscreenComponent:
-    case LegacyHiddenComponent: {
-      const newState: OffscreenState | null = finishedWork.memoizedState;
-      const isHidden = newState !== null;
-      hideOrUnhideAllChildren(finishedWork, isHidden);
-      return;
-    }
   }
   invariant(
     false,
@@ -1933,27 +1927,9 @@ function commitWork(current: Fiber | null, finishedWork: Fiber): void {
   );
 }
 
-function commitSuspenseComponent(finishedWork: Fiber) {
+function commitSuspenseCallback(finishedWork: Fiber) {
+  // TODO: Move this to passive phase
   const newState: SuspenseState | null = finishedWork.memoizedState;
-
-  if (newState !== null) {
-    markCommitTimeOfFallback();
-
-    if (supportsMutation) {
-      // Hide the Offscreen component that contains the primary children. TODO:
-      // Ideally, this effect would have been scheduled on the Offscreen fiber
-      // itself. That's how unhiding works: the Offscreen component schedules an
-      // effect on itself. However, in this case, the component didn't complete,
-      // so the fiber was never added to the effect list in the normal path. We
-      // could have appended it to the effect list in the Suspense component's
-      // second pass, but doing it this way is less complicated. This would be
-      // simpler if we got rid of the effect list and traversed the tree, like
-      // we're planning to do.
-      const primaryChildParent: Fiber = (finishedWork.child: any);
-      hideOrUnhideAllChildren(primaryChildParent, true);
-    }
-  }
-
   if (enableSuspenseCallback && newState !== null) {
     const suspenseCallback = finishedWork.memoizedProps.suspenseCallback;
     if (typeof suspenseCallback === 'function') {
@@ -2127,6 +2103,10 @@ function commitMutationEffects_complete(root: FiberRoot) {
 }
 
 function commitMutationEffectsOnFiber(finishedWork: Fiber, root: FiberRoot) {
+  // TODO: The factoring of this phase function could probably be improved. Consider
+  // switching on the type of work before checking the flags. That's what
+  // we do in all the other phases. I think this one is only different
+  // because of the shared reconcilation logic below.
   const flags = finishedWork.flags;
 
   if (flags & ContentReset) {
@@ -2147,6 +2127,37 @@ function commitMutationEffectsOnFiber(finishedWork: Fiber, root: FiberRoot) {
     }
   }
 
+  if (flags & Visibility) {
+    switch (finishedWork.tag) {
+      case SuspenseComponent: {
+        const newState: OffscreenState | null = finishedWork.memoizedState;
+        if (newState !== null) {
+          markCommitTimeOfFallback();
+          // Hide the Offscreen component that contains the primary children.
+          // TODO: Ideally, this effect would have been scheduled on the
+          // Offscreen fiber itself. That's how unhiding works: the Offscreen
+          // component schedules an effect on itself. However, in this case, the
+          // component didn't complete, so the fiber was never added to the
+          // effect list in the normal path. We could have appended it to the
+          // effect list in the Suspense component's second pass, but doing it
+          // this way is less complicated. This would be simpler if we got rid
+          // of the effect list and traversed the tree, like we're planning to
+          // do.
+          const primaryChildParent: Fiber = (finishedWork.child: any);
+          hideOrUnhideAllChildren(primaryChildParent, true);
+        }
+        break;
+      }
+      case OffscreenComponent:
+      case LegacyHiddenComponent: {
+        const newState: OffscreenState | null = finishedWork.memoizedState;
+        const isHidden = newState !== null;
+        hideOrUnhideAllChildren(finishedWork, isHidden);
+        break;
+      }
+    }
+  }
+
   // The following switch statement is only concerned about placement,
   // updates, and deletions. To avoid needing to add a case for every possible
   // bitmap value, we remove the secondary effects from the effect tag and

packages/react-reconciler/src/ReactFiberCompleteWork.new.js

@@ -9,7 +9,11 @@
 
 import type {Fiber} from './ReactInternalTypes';
 import type {Lanes, Lane} from './ReactFiberLane.new';
-import type {ReactScopeInstance, ReactContext} from 'shared/ReactTypes';
+import type {
+  ReactScopeInstance,
+  ReactContext,
+  Wakeable,
+} from 'shared/ReactTypes';
 import type {FiberRoot} from './ReactInternalTypes';
 import type {
   Instance,
@@ -60,6 +64,7 @@ import {
   Ref,
   RefStatic,
   Update,
+  Visibility,
   NoFlags,
   DidCapture,
   Snapshot,
@@ -1084,32 +1089,35 @@ function completeWork(
         }
       }
 
-      if (supportsPersistence) {
-        // TODO: Only schedule updates if not prevDidTimeout.
-        if (nextDidTimeout) {
-          // If this boundary just timed out, schedule an effect to attach a
-          // retry listener to the promise. This flag is also used to hide the
-          // primary children.
-          workInProgress.flags |= Update;
-        }
+      const wakeables: Set<Wakeable> | null = (workInProgress.updateQueue: any);
+      if (wakeables !== null) {
+        // Schedule an effect to attach a retry listener to the promise.
+        // TODO: Move to passive phase
+        workInProgress.flags |= Update;
       }
-      if (supportsMutation) {
-        // TODO: Only schedule updates if these values are non equal, i.e. it changed.
-        if (nextDidTimeout || prevDidTimeout) {
-          // If this boundary just timed out, schedule an effect to attach a
-          // retry listener to the promise. This flag is also used to hide the
-          // primary children. In mutation mode, we also need the flag to
-          // *unhide* children that were previously hidden, so check if this
-          // is currently timed out, too.
-          workInProgress.flags |= Update;
+
+      if (nextDidTimeout !== prevDidTimeout) {
+        // If the boundary was hidden or unhidden, schedule an effect to
+        // toggle its visibility.
+        if (supportsMutation) {
+          workInProgress.flags |= Visibility;
+        }
+        if (supportsPersistence) {
+          // TODO: We don't toggle the visibility (hideOrUnhideChildren) in
+          // persistent mode (Fabric) because it used to only change the
+          // visibility of the host nodes, which in persistent mode is toggled
+          // during the complete phase (appendChildrenToContainer). But now we
+          // also use it to toggle the effects. Need to implement this.
         }
       }
+
       if (
         enableSuspenseCallback &&
         workInProgress.updateQueue !== null &&
         workInProgress.memoizedProps.suspenseCallback != null
       ) {
         // Always notify the callback
+        // TODO: Move to passive phase
         workInProgress.flags |= Update;
       }
       bubbleProperties(workInProgress);
@@ -1396,7 +1404,7 @@ function completeWork(
           prevIsHidden !== nextIsHidden &&
           newProps.mode !== 'unstable-defer-without-hiding'
         ) {
-          workInProgress.flags |= Update;
+          workInProgress.flags |= Visibility;
         }
       }
 

packages/react-reconciler/src/ReactFiberFlags.js

@@ -82,7 +82,7 @@ export const MutationMask =
   Ref |
   Hydrating |
   Visibility;
-export const LayoutMask = Update | Callback | Ref;
+export const LayoutMask = Update | Callback | Ref | Visibility;
 
 // TODO: Split into PassiveMountMask and PassiveUnmountMask
 export const PassiveMask = Passive | ChildDeletion;