Fix: Hide new/updated nodes in already hidden tree

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.

Author: acdlite

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) {

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) {

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,78 @@ describe('ReactOffscreen', () => {
     });
     expect(Scheduler).toHaveYielded(['Unmount layout']);
   });
+
+  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>
+      </>,
+    );
+  });
+
+  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>);
+  });
 });