[Fiber] Don't unhide a node if a direct parent offscreen is still hidden (#34821)

If an inner Offscreen commits an unhide, but an outer Offscreen is still hidden but they're controlling the same DOM node then we shouldn't unhide the DOM node yet. This keeps track of whether we're directly inside a hidden offscreen. It might be better to just do the tree search instead of keeping the stack state since it's a rare case. Although this hide/unhide path does trigger a lot of times even when there's no change. This was technically a bug with Suspense too but it doesn't appear because a suspended Suspense boundary never commits its partial state. If it did, it would trigger this same path. But it can happen with an outer Activity and inner Suspense.
2026-01-15 12:15:22 +00:00 · 2025-10-12 19:50:06 -04:00
parent ead92181bd
commit 1d68bce19c
3 changed files with 184 additions and 3 deletions
--- a/packages/react-reconciler/src/ReactFiberCommitWork.js
+++ b/packages/react-reconciler/src/ReactFiberCommitWork.js
@@ -292,6 +292,9 @@ import type {Flags} from './ReactFiberFlags';
 // Allows us to avoid traversing the return path to find the nearest Offscreen ancestor.
 let offscreenSubtreeIsHidden: boolean = false;
 let offscreenSubtreeWasHidden: boolean = false;
+// Track whether there's a hidden offscreen above with no HostComponent between. If so,
+// it overrides the hiddenness of the HostComponent below.
+let offscreenDirectParentIsHidden: boolean = false;

 // Used to track if a form needs to be reset at the end of the mutation phase.
 let needsFormReset = false;
@@ -2141,8 +2144,14 @@ function commitMutationEffectsOnFiber(
      // Fall through
    }
    case HostComponent: {
+      // We've hit a host component, so it's no longer a direct parent.
+      const prevOffscreenDirectParentIsHidden = offscreenDirectParentIsHidden;
+      offscreenDirectParentIsHidden = false;
+
      recursivelyTraverseMutationEffects(root, finishedWork, lanes);

+      offscreenDirectParentIsHidden = prevOffscreenDirectParentIsHidden;
+
      commitReconciliationEffects(finishedWork, lanes);

      if (flags & Ref) {
@@ -2422,10 +2431,14 @@ function commitMutationEffectsOnFiber(
        // effects again.
        const prevOffscreenSubtreeIsHidden = offscreenSubtreeIsHidden;
        const prevOffscreenSubtreeWasHidden = offscreenSubtreeWasHidden;
+        const prevOffscreenDirectParentIsHidden = offscreenDirectParentIsHidden;
        offscreenSubtreeIsHidden = prevOffscreenSubtreeIsHidden || isHidden;
+        offscreenDirectParentIsHidden =
+          prevOffscreenDirectParentIsHidden || isHidden;
        offscreenSubtreeWasHidden = prevOffscreenSubtreeWasHidden || wasHidden;
        recursivelyTraverseMutationEffects(root, finishedWork, lanes);
        offscreenSubtreeWasHidden = prevOffscreenSubtreeWasHidden;
+        offscreenDirectParentIsHidden = prevOffscreenDirectParentIsHidden;
        offscreenSubtreeIsHidden = prevOffscreenSubtreeIsHidden;

        if (
@@ -2504,9 +2517,10 @@ function commitMutationEffectsOnFiber(
        }

        if (supportsMutation) {
-          // TODO: This needs to run whenever there's an insertion or update
-          // inside a hidden Offscreen tree.
-          hideOrUnhideAllChildren(finishedWork, isHidden);
+          // If it's trying to unhide but the parent is still hidden, then we should not unhide.
+          if (isHidden || !offscreenDirectParentIsHidden) {
+            hideOrUnhideAllChildren(finishedWork, isHidden);
+          }
        }
      }

--- a/packages/react-reconciler/src/tests/Activity-test.js
+++ b/packages/react-reconciler/src/tests/Activity-test.js
@@ -14,6 +14,7 @@ let waitForPaint;
 let waitFor;
 let assertLog;
 let assertConsoleErrorDev;
+let Suspense;

 describe('Activity', () => {
  beforeEach(() => {
@@ -25,6 +26,7 @@ describe('Activity', () => {
    act = require('internal-test-utils').act;
    LegacyHidden = React.unstable_LegacyHidden;
    Activity = React.Activity;
+    Suspense = React.Suspense;
    useState = React.useState;
    useInsertionEffect = React.useInsertionEffect;
    useLayoutEffect = React.useLayoutEffect;
@@ -1424,6 +1426,72 @@ describe('Activity', () => {
    );
  });

+  // @gate enableActivity
+  it('reveal an inner Activity boundary without revealing an outer one on the same host child', async () => {
+    // This ensures that no update is scheduled, which would cover up the bug if the parent
+    // then re-hides the child on the way up.
+    const memoizedElement = <div />;
+    function App({showOuter, showInner}) {
+      return (
+        <Activity mode={showOuter ? 'visible' : 'hidden'} name="Outer">
+          <Activity mode={showInner ? 'visible' : 'hidden'} name="Inner">
+            {memoizedElement}
+          </Activity>
+        </Activity>
+      );
+    }
+
+    const root = ReactNoop.createRoot();
+
+    // Prerender the whole tree.
+    await act(() => {
+      root.render(<App showOuter={false} showInner={false} />);
+    });
+    expect(root).toMatchRenderedOutput(<div hidden={true} />);
+
+    await act(() => {
+      root.render(<App showOuter={false} showInner={true} />);
+    });
+    expect(root).toMatchRenderedOutput(<div hidden={true} />);
+  });
+
+  // @gate enableActivity
+  it('reveal an inner Suspense boundary without revealing an outer Activity on the same host child', async () => {
+    // This ensures that no update is scheduled, which would cover up the bug if the parent
+    // then re-hides the child on the way up.
+    const memoizedElement = <div />;
+    const promise = new Promise(() => {});
+    function App({showOuter, showInner}) {
+      return (
+        <Activity mode={showOuter ? 'visible' : 'hidden'} name="Outer">
+          <Suspense name="Inner">
+            {memoizedElement}
+            {showInner ? null : promise}
+          </Suspense>
+        </Activity>
+      );
+    }
+
+    const root = ReactNoop.createRoot();
+
+    // Prerender the whole tree.
+    await act(() => {
+      root.render(<App showOuter={false} showInner={true} />);
+    });
+    expect(root).toMatchRenderedOutput(<div hidden={true} />);
+
+    // Resuspend the inner.
+    await act(() => {
+      root.render(<App showOuter={false} showInner={false} />);
+    });
+    expect(root).toMatchRenderedOutput(<div hidden={true} />);
+
+    await act(() => {
+      root.render(<App showOuter={false} showInner={true} />);
+    });
+    expect(root).toMatchRenderedOutput(<div hidden={true} />);
+  });
+
  // @gate enableActivity
  it('insertion effects are not disconnected when the visibility changes', async () => {
    function Child({step}) {
--- a/packages/react-reconciler/src/tests/ReactSuspenseEffectsSemantics-test.js
+++ b/packages/react-reconciler/src/tests/ReactSuspenseEffectsSemantics-test.js
@@ -1401,6 +1401,105 @@ describe('ReactSuspenseEffectsSemantics', () => {
      );
    });

+    // @gate enableLegacyCache
+    it('should wait to reveal an inner child when inner one reveals first', async () => {
+      function App({outerChildren, innerChildren}) {
+        return (
+          <Suspense fallback={<Text text="OuterFallback" />} name="Outer">
+            <Suspense fallback={<Text text="InnerFallback" />} name="Inner">
+              <div>{innerChildren}</div>
+            </Suspense>
+            {outerChildren}
+          </Suspense>
+        );
+      }
+
+      // Mount
+      await act(() => {
+        ReactNoop.render(<App />);
+      });
+      assertLog([]);
+      expect(ReactNoop).toMatchRenderedOutput(<div />);
+
+      // Resuspend inner boundary
+      await act(() => {
+        ReactNoop.render(
+          <App
+            outerChildren={null}
+            innerChildren={<AsyncText text="InnerAsync" />}
+          />,
+        );
+      });
+      assertLog([
+        'Suspend:InnerAsync',
+        'Text:InnerFallback render',
+        'Text:InnerFallback create insertion',
+        'Text:InnerFallback create layout',
+        'Text:InnerFallback create passive',
+        'Suspend:InnerAsync',
+      ]);
+      expect(ReactNoop).toMatchRenderedOutput(
+        <>
+          <div hidden={true} />
+          <span prop="InnerFallback" />
+        </>,
+      );
+
+      // Resuspend both boundaries
+      await act(() => {
+        ReactNoop.render(
+          <App
+            outerChildren={<AsyncText text="OuterAsync" />}
+            innerChildren={<AsyncText text="InnerAsync" />}
+          />,
+        );
+      });
+      assertLog([
+        'Suspend:InnerAsync',
+        'Text:InnerFallback render',
+        'Suspend:OuterAsync',
+        'Text:OuterFallback render',
+        'Text:InnerFallback destroy layout',
+        'Text:OuterFallback create insertion',
+        'Text:OuterFallback create layout',
+        'Text:OuterFallback create passive',
+        'Suspend:InnerAsync',
+        'Text:InnerFallback render',
+        'Suspend:OuterAsync',
+      ]);
+      expect(ReactNoop).toMatchRenderedOutput(
+        <>
+          <div hidden={true} />
+          <span prop="InnerFallback" hidden={true} />
+          <span prop="OuterFallback" />
+        </>,
+      );
+
+      // Unsuspend the inner Suspense subtree only
+      // Interestingly, this never commits because the tree is left suspended.
+      // If it did commit, it would potentially cause the div to incorrectly reappear.
+      await act(() => {
+        ReactNoop.render(
+          <App
+            outerChildren={<AsyncText text="OuterAsync" />}
+            innerChildren={null}
+          />,
+        );
+      });
+      assertLog([
+        'Suspend:OuterAsync',
+        'Text:OuterFallback render',
+        'Suspend:OuterAsync',
+      ]);
+      expect(ReactNoop).toMatchRenderedOutput(
+        <>
+          <div hidden={true} />
+          <span prop="InnerFallback" hidden={true} />
+          <span prop="OuterFallback" />
+        </>,
+      );
+    });
+
    // @gate enableLegacyCache
    it('should show nested host nodes if multiple boundaries resolve at the same time', async () => {
      function App({innerChildren = null, outerChildren = null}) {