From bce1e9e886b5f05350b4d82a75531048b3ec2b62 Mon Sep 17 00:00:00 2001
From: Andrew Clark <git@andrewclark.io>
Date: Mon, 26 Jul 2021 11:18:55 -0400
Subject: [PATCH] Fix: Hide new/updated nodes in already hidden tree (#21929)

When a new node is added to an already hidden Offscreen tree, we need
to schedule a visibility effect to hide it. Previously we would only
hide when the boundary initially switches from visible to hidden, which
meant that newly inserted nodes would be visible.

We need to do the same thing for nodes that are updated, because the
update might affect the DOM node's `style.display` property.

The implementation is to check the `subtreeFlags` for an Insertion or
Update effect.

This only affects Offscreen, not Suspense, because Suspense boundaries
cannot be updated while in their fallback (hidden) state.

And it only affects mutation mode, because in persistent mode we
implement hiding by cloning the host tree during the complete phase,
which already happens on every update.
---
 .../src/ReactFiberCompleteWork.new.js         |  26 +++-
 .../src/ReactFiberCompleteWork.old.js         |  26 +++-
 .../src/__tests__/ReactOffscreen-test.js      | 112 +++++++++++++-----
 3 files changed, 120 insertions(+), 44 deletions(-)

diff --git a/packages/react-reconciler/src/ReactFiberCompleteWork.new.js b/packages/react-reconciler/src/ReactFiberCompleteWork.new.js
index b6feee098b4f4..9416ac45dd4c5 100644
--- a/packages/react-reconciler/src/ReactFiberCompleteWork.new.js
+++ b/packages/react-reconciler/src/ReactFiberCompleteWork.new.js
@@ -63,6 +63,7 @@ import {NoMode, ConcurrentMode, ProfileMode} from './ReactTypeOfMode';
 import {
   Ref,
   RefStatic,
+  Placement,
   Update,
   Visibility,
   NoFlags,
@@ -1369,13 +1370,26 @@ function completeWork(
         }
       }
 
-      // Don't bubble properties for hidden children.
-      if (
-        !nextIsHidden ||
-        includesSomeLane(subtreeRenderLanes, (OffscreenLane: Lane)) ||
-        (workInProgress.mode & ConcurrentMode) === NoMode
-      ) {
+      if (!nextIsHidden || (workInProgress.mode & ConcurrentMode) === NoMode) {
         bubbleProperties(workInProgress);
+      } else {
+        // Don't bubble properties for hidden children unless we're rendering
+        // at offscreen priority.
+        if (includesSomeLane(subtreeRenderLanes, (OffscreenLane: Lane))) {
+          bubbleProperties(workInProgress);
+          if (supportsMutation) {
+            // Check if there was an insertion or update in the hidden subtree.
+            // If so, we need to hide those nodes in the commit phase, so
+            // schedule a visibility effect.
+            if (
+              workInProgress.tag !== LegacyHiddenComponent &&
+              workInProgress.subtreeFlags & (Placement | Update) &&
+              newProps.mode !== 'unstable-defer-without-hiding'
+            ) {
+              workInProgress.flags |= Visibility;
+            }
+          }
+        }
       }
 
       if (enableCache) {
diff --git a/packages/react-reconciler/src/ReactFiberCompleteWork.old.js b/packages/react-reconciler/src/ReactFiberCompleteWork.old.js
index 612ad2db52c9f..35d17f871ab93 100644
--- a/packages/react-reconciler/src/ReactFiberCompleteWork.old.js
+++ b/packages/react-reconciler/src/ReactFiberCompleteWork.old.js
@@ -63,6 +63,7 @@ import {NoMode, ConcurrentMode, ProfileMode} from './ReactTypeOfMode';
 import {
   Ref,
   RefStatic,
+  Placement,
   Update,
   Visibility,
   NoFlags,
@@ -1369,13 +1370,26 @@ function completeWork(
         }
       }
 
-      // Don't bubble properties for hidden children.
-      if (
-        !nextIsHidden ||
-        includesSomeLane(subtreeRenderLanes, (OffscreenLane: Lane)) ||
-        (workInProgress.mode & ConcurrentMode) === NoMode
-      ) {
+      if (!nextIsHidden || (workInProgress.mode & ConcurrentMode) === NoMode) {
         bubbleProperties(workInProgress);
+      } else {
+        // Don't bubble properties for hidden children unless we're rendering
+        // at offscreen priority.
+        if (includesSomeLane(subtreeRenderLanes, (OffscreenLane: Lane))) {
+          bubbleProperties(workInProgress);
+          if (supportsMutation) {
+            // Check if there was an insertion or update in the hidden subtree.
+            // If so, we need to hide those nodes in the commit phase, so
+            // schedule a visibility effect.
+            if (
+              workInProgress.tag !== LegacyHiddenComponent &&
+              workInProgress.subtreeFlags & (Placement | Update) &&
+              newProps.mode !== 'unstable-defer-without-hiding'
+            ) {
+              workInProgress.flags |= Visibility;
+            }
+          }
+        }
       }
 
       if (enableCache) {
diff --git a/packages/react-reconciler/src/__tests__/ReactOffscreen-test.js b/packages/react-reconciler/src/__tests__/ReactOffscreen-test.js
index 9f9225fea3bad..4dfd6459a33a3 100644
--- a/packages/react-reconciler/src/__tests__/ReactOffscreen-test.js
+++ b/packages/react-reconciler/src/__tests__/ReactOffscreen-test.js
@@ -201,14 +201,7 @@ describe('ReactOffscreen', () => {
     });
     // No layout effect.
     expect(Scheduler).toHaveYielded(['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" />);
-    }
+    expect(root).toMatchRenderedOutput(<span hidden={true} prop="Child" />);
 
     // Unhide the tree. The layout effect is mounted.
     await act(async () => {
@@ -255,14 +248,7 @@ describe('ReactOffscreen', () => {
       );
     });
     expect(Scheduler).toHaveYielded(['Unmount layout', '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" />);
-    }
+    expect(root).toMatchRenderedOutput(<span hidden={true} prop="Child" />);
 
     // Unhide the tree. The layout effect is re-mounted.
     await act(async () => {
@@ -299,14 +285,7 @@ describe('ReactOffscreen', () => {
       );
     });
     expect(Scheduler).toHaveYielded(['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" />);
-    }
+    expect(root).toMatchRenderedOutput(<span hidden={true} prop="Child" />);
 
     // Show the tree. The layout effect is mounted.
     await act(async () => {
@@ -328,14 +307,7 @@ describe('ReactOffscreen', () => {
       );
     });
     expect(Scheduler).toHaveYielded(['Unmount layout', '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" />);
-    }
+    expect(root).toMatchRenderedOutput(<span hidden={true} prop="Child" />);
   });
 
   // @gate experimental || www
@@ -385,4 +357,80 @@ describe('ReactOffscreen', () => {
     });
     expect(Scheduler).toHaveYielded(['Unmount layout']);
   });
+
+  // @gate experimental || www
+  it('hides new insertions into an already hidden tree', async () => {
+    const root = ReactNoop.createRoot();
+    await act(async () => {
+      root.render(
+        <Offscreen mode="hidden">
+          <span>Hi</span>
+        </Offscreen>,
+      );
+    });
+    expect(root).toMatchRenderedOutput(<span hidden={true}>Hi</span>);
+
+    // Insert a new node into the hidden tree
+    await act(async () => {
+      root.render(
+        <Offscreen mode="hidden">
+          <span>Hi</span>
+          <span>Something new</span>
+        </Offscreen>,
+      );
+    });
+    expect(root).toMatchRenderedOutput(
+      <>
+        <span hidden={true}>Hi</span>
+        {/* This new node should also be hidden */}
+        <span hidden={true}>Something new</span>
+      </>,
+    );
+  });
+
+  // @gate experimental || www
+  it('hides updated nodes inside an already hidden tree', async () => {
+    const root = ReactNoop.createRoot();
+    await act(async () => {
+      root.render(
+        <Offscreen mode="hidden">
+          <span>Hi</span>
+        </Offscreen>,
+      );
+    });
+    expect(root).toMatchRenderedOutput(<span hidden={true}>Hi</span>);
+
+    // Set the `hidden` prop to on an already hidden node
+    await act(async () => {
+      root.render(
+        <Offscreen mode="hidden">
+          <span hidden={false}>Hi</span>
+        </Offscreen>,
+      );
+    });
+    // It should still be hidden, because the Offscreen container overrides it
+    expect(root).toMatchRenderedOutput(<span hidden={true}>Hi</span>);
+
+    // Unhide the boundary
+    await act(async () => {
+      root.render(
+        <Offscreen mode="visible">
+          <span hidden={true}>Hi</span>
+        </Offscreen>,
+      );
+    });
+    // It should still be hidden, because of the prop
+    expect(root).toMatchRenderedOutput(<span hidden={true}>Hi</span>);
+
+    // Remove the `hidden` prop
+    await act(async () => {
+      root.render(
+        <Offscreen mode="visible">
+          <span>Hi</span>
+        </Offscreen>,
+      );
+    });
+    // Now it's visible
+    expect(root).toMatchRenderedOutput(<span>Hi</span>);
+  });
 });