From 93b9f9e514048554739b99d39abdd29941489cd3 Mon Sep 17 00:00:00 2001
From: Andrew Clark <git@andrewclark.io>
Date: Tue, 20 Jul 2021 22:37:17 -0400
Subject: [PATCH] Fix: LegacyHidden should not toggle effects
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

LegacyHidden is a transitional API that we added to replace the old
`<div hidden={true} />` API that we used to use for pre-rendering. The
plan is to replace this with the Offscreen component, once it's ready.

The idea is that LegacyHidden has identical behavior to Offscreen except
that it doesn't change the behavior of effects. (Which is basically how
`<div hidden={true} />` worked — it prerendered the hidden content in
the background, but nothing else.) That way, while we're rolling this
out, we could toggle the feature behind a feature flag either for
performance testing or as a kill switch.

It looks like we accidentally enabled the effects flag for both
Offscreen _and_ LegacyHidden. I suppose it's a good thing that nobody
has complained yet, since we eventually do want to ship this
behavior everywhere?

But I do think we should remove it from LegacyHidden, and roll it out by
gating the component type in the downstream repo. That way if there's an
issue related to the use of LegacyHidden, we can disable that without
disabling the behavior for Suspense boundaries.

In retrospect, I might have implemented this as an unstable prop on
Offscreen instead of a completely separate type — though at the time,
Offscreen didn't exist. I originally added LegacyHidden to unblock the
Lanes refactor, so I could move the deprioritization logic out of the
HostComponent implementation.

Not a big deal since we're going to remove this soon. The implementation
is almost the same regardless: before disconnecting or reconnecting
the effects, check the fiber tag. The rest of the logic is the same.
---
 .../src/ReactFiberCommitWork.new.js           | 36 +++++++-------
 .../src/ReactFiberCommitWork.old.js           | 36 +++++++-------
 .../src/ReactFiberCompleteWork.new.js         |  4 +-
 .../src/ReactFiberCompleteWork.old.js         |  4 +-
 .../src/__tests__/ReactOffscreen-test.js      | 48 +++++++++++++++++++
 5 files changed, 88 insertions(+), 40 deletions(-)

diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js
index a68e489dccc..f9d9ad39999 100644
--- a/packages/react-reconciler/src/ReactFiberCommitWork.new.js
+++ b/packages/react-reconciler/src/ReactFiberCommitWork.new.js
@@ -2131,8 +2131,7 @@ function commitMutationEffectsOnFiber(finishedWork: Fiber, root: FiberRoot) {
         }
         break;
       }
-      case OffscreenComponent:
-      case LegacyHiddenComponent: {
+      case OffscreenComponent: {
         const newState: OffscreenState | null = finishedWork.memoizedState;
         const isHidden = newState !== null;
         const current = finishedWork.alternate;
@@ -2145,27 +2144,26 @@ function commitMutationEffectsOnFiber(finishedWork: Fiber, root: FiberRoot) {
           hideOrUnhideAllChildren(offscreenBoundary, isHidden);
         }
 
-        if (isHidden) {
-          if (!wasHidden) {
-            if (
-              enableSuspenseLayoutEffectSemantics &&
-              (offscreenBoundary.mode & ConcurrentMode) !== NoMode
-            ) {
-              nextEffect = offscreenBoundary;
-              let offscreenChild = offscreenBoundary.child;
-              while (offscreenChild !== null) {
-                nextEffect = offscreenChild;
-                disappearLayoutEffects_begin(offscreenChild);
-                offscreenChild = offscreenChild.sibling;
+        if (enableSuspenseLayoutEffectSemantics) {
+          if (isHidden) {
+            if (!wasHidden) {
+              if ((offscreenBoundary.mode & ConcurrentMode) !== NoMode) {
+                nextEffect = offscreenBoundary;
+                let offscreenChild = offscreenBoundary.child;
+                while (offscreenChild !== null) {
+                  nextEffect = offscreenChild;
+                  disappearLayoutEffects_begin(offscreenChild);
+                  offscreenChild = offscreenChild.sibling;
+                }
               }
             }
+          } else {
+            if (wasHidden) {
+              // TODO: Move re-appear call here for symmetry?
+            }
           }
-        } else {
-          if (wasHidden) {
-            // TODO: Move re-appear call here for symmetry?
-          }
+          break;
         }
-        break;
       }
     }
   }
diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.old.js b/packages/react-reconciler/src/ReactFiberCommitWork.old.js
index aea3e36ed09..60c230cd251 100644
--- a/packages/react-reconciler/src/ReactFiberCommitWork.old.js
+++ b/packages/react-reconciler/src/ReactFiberCommitWork.old.js
@@ -2131,8 +2131,7 @@ function commitMutationEffectsOnFiber(finishedWork: Fiber, root: FiberRoot) {
         }
         break;
       }
-      case OffscreenComponent:
-      case LegacyHiddenComponent: {
+      case OffscreenComponent: {
         const newState: OffscreenState | null = finishedWork.memoizedState;
         const isHidden = newState !== null;
         const current = finishedWork.alternate;
@@ -2145,27 +2144,26 @@ function commitMutationEffectsOnFiber(finishedWork: Fiber, root: FiberRoot) {
           hideOrUnhideAllChildren(offscreenBoundary, isHidden);
         }
 
-        if (isHidden) {
-          if (!wasHidden) {
-            if (
-              enableSuspenseLayoutEffectSemantics &&
-              (offscreenBoundary.mode & ConcurrentMode) !== NoMode
-            ) {
-              nextEffect = offscreenBoundary;
-              let offscreenChild = offscreenBoundary.child;
-              while (offscreenChild !== null) {
-                nextEffect = offscreenChild;
-                disappearLayoutEffects_begin(offscreenChild);
-                offscreenChild = offscreenChild.sibling;
+        if (enableSuspenseLayoutEffectSemantics) {
+          if (isHidden) {
+            if (!wasHidden) {
+              if ((offscreenBoundary.mode & ConcurrentMode) !== NoMode) {
+                nextEffect = offscreenBoundary;
+                let offscreenChild = offscreenBoundary.child;
+                while (offscreenChild !== null) {
+                  nextEffect = offscreenChild;
+                  disappearLayoutEffects_begin(offscreenChild);
+                  offscreenChild = offscreenChild.sibling;
+                }
               }
             }
+          } else {
+            if (wasHidden) {
+              // TODO: Move re-appear call here for symmetry?
+            }
           }
-        } else {
-          if (wasHidden) {
-            // TODO: Move re-appear call here for symmetry?
-          }
+          break;
         }
-        break;
       }
     }
   }
diff --git a/packages/react-reconciler/src/ReactFiberCompleteWork.new.js b/packages/react-reconciler/src/ReactFiberCompleteWork.new.js
index 32b9587f263..b6feee098b4 100644
--- a/packages/react-reconciler/src/ReactFiberCompleteWork.new.js
+++ b/packages/react-reconciler/src/ReactFiberCompleteWork.new.js
@@ -1361,7 +1361,9 @@ function completeWork(
         const prevIsHidden = prevState !== null;
         if (
           prevIsHidden !== nextIsHidden &&
-          newProps.mode !== 'unstable-defer-without-hiding'
+          newProps.mode !== 'unstable-defer-without-hiding' &&
+          // LegacyHidden doesn't do any hiding — it only pre-renders.
+          workInProgress.tag !== LegacyHiddenComponent
         ) {
           workInProgress.flags |= Visibility;
         }
diff --git a/packages/react-reconciler/src/ReactFiberCompleteWork.old.js b/packages/react-reconciler/src/ReactFiberCompleteWork.old.js
index 60359fd42d8..612ad2db52c 100644
--- a/packages/react-reconciler/src/ReactFiberCompleteWork.old.js
+++ b/packages/react-reconciler/src/ReactFiberCompleteWork.old.js
@@ -1361,7 +1361,9 @@ function completeWork(
         const prevIsHidden = prevState !== null;
         if (
           prevIsHidden !== nextIsHidden &&
-          newProps.mode !== 'unstable-defer-without-hiding'
+          newProps.mode !== 'unstable-defer-without-hiding' &&
+          // LegacyHidden doesn't do any hiding — it only pre-renders.
+          workInProgress.tag !== LegacyHiddenComponent
         ) {
           workInProgress.flags |= Visibility;
         }
diff --git a/packages/react-reconciler/src/__tests__/ReactOffscreen-test.js b/packages/react-reconciler/src/__tests__/ReactOffscreen-test.js
index 2915b1e769f..9f9225fea3b 100644
--- a/packages/react-reconciler/src/__tests__/ReactOffscreen-test.js
+++ b/packages/react-reconciler/src/__tests__/ReactOffscreen-test.js
@@ -337,4 +337,52 @@ describe('ReactOffscreen', () => {
       expect(root).toMatchRenderedOutput(<span prop="Child" />);
     }
   });
+
+  // @gate experimental || www
+  it('does not toggle effects for LegacyHidden component', async () => {
+    // LegacyHidden is meant to be the same as offscreen except it doesn't
+    // do anything to effects. Only used by www, as a temporary migration step.
+    function Child({text}) {
+      useLayoutEffect(() => {
+        Scheduler.unstable_yieldValue('Mount layout');
+        return () => {
+          Scheduler.unstable_yieldValue('Unmount layout');
+        };
+      }, []);
+      return <Text text="Child" />;
+    }
+
+    const root = ReactNoop.createRoot();
+    await act(async () => {
+      root.render(
+        <LegacyHidden mode="visible">
+          <Child />
+        </LegacyHidden>,
+      );
+    });
+    expect(Scheduler).toHaveYielded(['Child', 'Mount layout']);
+
+    await act(async () => {
+      root.render(
+        <LegacyHidden mode="hidden">
+          <Child />
+        </LegacyHidden>,
+      );
+    });
+    expect(Scheduler).toHaveYielded(['Child']);
+
+    await act(async () => {
+      root.render(
+        <LegacyHidden mode="visible">
+          <Child />
+        </LegacyHidden>,
+      );
+    });
+    expect(Scheduler).toHaveYielded(['Child']);
+
+    await act(async () => {
+      root.render(null);
+    });
+    expect(Scheduler).toHaveYielded(['Unmount layout']);
+  });
 });