From 864bc2be646c9f3ab0a38e85cf422da953c448ab Mon Sep 17 00:00:00 2001
From: Samuel Susla <samuel.susla@gmail.com>
Date: Thu, 10 Nov 2022 14:49:30 +0000
Subject: [PATCH] Don't disappear layout effects unnecessarily (#25660)

Nested Offscreens can run into a case where outer Offscreen is revealed
while inner one is hidden in a single commit. This is an edge case that
was previously missed. We need to prevent call to disappear layout
effects.

When we go from state:
```jsx
<Offscreen mode={'hidden'}> // outer offscreen
  <Offscreen mode={'visible'}> // inner offscreen
    {children}
  </Offscreen>
</Offscreen>
```

To following. Notice that visibility of each offscreen flips.

```jsx
<Offscreen mode={'visible'}> // outer offscreen
  <Offscreen mode={'hidden'}> // inner offscreen
    {children}
  </Offscreen>
</Offscreen>
```

Inner offscreen must not call
`recursivelyTraverseDisappearLayoutEffects`.
Check unit tests for an example of this.
---
 .../src/ReactFiberCommitWork.new.js           |   7 +-
 .../src/ReactFiberCommitWork.old.js           |   7 +-
 .../src/__tests__/ReactOffscreen-test.js      | 102 ++++++++++++------
 3 files changed, 80 insertions(+), 36 deletions(-)

diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js
index 0b885bf134..1a9b72c8ae 100644
--- a/packages/react-reconciler/src/ReactFiberCommitWork.new.js
+++ b/packages/react-reconciler/src/ReactFiberCommitWork.new.js
@@ -2880,12 +2880,13 @@ function commitMutationEffectsOnFiber(
 
         if (isHidden) {
           const isUpdate = current !== null;
-          const isAncestorOffscreenHidden = offscreenSubtreeIsHidden;
+          const wasHiddenByAncestorOffscreen =
+            offscreenSubtreeIsHidden || offscreenSubtreeWasHidden;
           // Only trigger disapper layout effects if:
           //   - This is an update, not first mount.
           //   - This Offscreen was not hidden before.
-          //   - No ancestor Offscreen is hidden.
-          if (isUpdate && !wasHidden && !isAncestorOffscreenHidden) {
+          //   - Ancestor Offscreen was not hidden in previous commit.
+          if (isUpdate && !wasHidden && !wasHiddenByAncestorOffscreen) {
             if ((finishedWork.mode & ConcurrentMode) !== NoMode) {
               // Disappear the layout effects of all the children
               recursivelyTraverseDisappearLayoutEffects(finishedWork);
diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.old.js b/packages/react-reconciler/src/ReactFiberCommitWork.old.js
index cbbca98212..a873d23c3d 100644
--- a/packages/react-reconciler/src/ReactFiberCommitWork.old.js
+++ b/packages/react-reconciler/src/ReactFiberCommitWork.old.js
@@ -2880,12 +2880,13 @@ function commitMutationEffectsOnFiber(
 
         if (isHidden) {
           const isUpdate = current !== null;
-          const isAncestorOffscreenHidden = offscreenSubtreeIsHidden;
+          const wasHiddenByAncestorOffscreen =
+            offscreenSubtreeIsHidden || offscreenSubtreeWasHidden;
           // Only trigger disapper layout effects if:
           //   - This is an update, not first mount.
           //   - This Offscreen was not hidden before.
-          //   - No ancestor Offscreen is hidden.
-          if (isUpdate && !wasHidden && !isAncestorOffscreenHidden) {
+          //   - Ancestor Offscreen was not hidden in previous commit.
+          if (isUpdate && !wasHidden && !wasHiddenByAncestorOffscreen) {
             if ((finishedWork.mode & ConcurrentMode) !== NoMode) {
               // Disappear the layout effects of all the children
               recursivelyTraverseDisappearLayoutEffects(finishedWork);
diff --git a/packages/react-reconciler/src/__tests__/ReactOffscreen-test.js b/packages/react-reconciler/src/__tests__/ReactOffscreen-test.js
index 417f851ad4..d830265610 100644
--- a/packages/react-reconciler/src/__tests__/ReactOffscreen-test.js
+++ b/packages/react-reconciler/src/__tests__/ReactOffscreen-test.js
@@ -275,7 +275,7 @@ describe('ReactOffscreen', () => {
     // goes from visible to hidden in synchronous update.
     class ClassComponent extends React.Component {
       render() {
-        return <Text text="Child" />;
+        return <Text text="child" />;
       }
 
       componentWillUnmount() {
@@ -287,47 +287,89 @@ describe('ReactOffscreen', () => {
       }
     }
 
-    let _setIsVisible;
-    let isFirstRender = true;
-
-    function App() {
-      const [isVisible, setIsVisible] = React.useState(true);
-      _setIsVisible = setIsVisible;
-      if (isFirstRender === true) {
-        setIsVisible(false);
-        isFirstRender = false;
-      }
-
-      return (
-        <Offscreen mode="hidden">
-          <Offscreen mode={isVisible ? 'visible' : 'hidden'}>
-            <ClassComponent />
-          </Offscreen>
-        </Offscreen>
-      );
-    }
-
     const root = ReactNoop.createRoot();
     await act(async () => {
-      root.render(<App />);
+      // Outer and inner offscreen are hidden.
+      root.render(
+        <Offscreen mode={'hidden'}>
+          <Offscreen mode={'hidden'}>
+            <ClassComponent />
+          </Offscreen>
+        </Offscreen>,
+      );
     });
 
-    expect(Scheduler).toHaveYielded(['Child']);
-    expect(root).toMatchRenderedOutput(<span hidden={true} prop="Child" />);
+    expect(Scheduler).toHaveYielded(['child']);
+    expect(root).toMatchRenderedOutput(<span hidden={true} prop="child" />);
 
     await act(async () => {
-      _setIsVisible(true);
+      // Inner offscreen is visible.
+      root.render(
+        <Offscreen mode={'hidden'}>
+          <Offscreen mode={'visible'}>
+            <ClassComponent />
+          </Offscreen>
+        </Offscreen>,
+      );
     });
 
-    expect(Scheduler).toHaveYielded(['Child']);
-    expect(root).toMatchRenderedOutput(<span hidden={true} prop="Child" />);
+    expect(Scheduler).toHaveYielded(['child']);
+    expect(root).toMatchRenderedOutput(<span hidden={true} prop="child" />);
 
     await act(async () => {
-      _setIsVisible(false);
+      // Inner offscreen is hidden.
+      root.render(
+        <Offscreen mode={'hidden'}>
+          <Offscreen mode={'hidden'}>
+            <ClassComponent />
+          </Offscreen>
+        </Offscreen>,
+      );
     });
 
-    expect(Scheduler).toHaveYielded(['Child']);
-    expect(root).toMatchRenderedOutput(<span hidden={true} prop="Child" />);
+    expect(Scheduler).toHaveYielded(['child']);
+    expect(root).toMatchRenderedOutput(<span hidden={true} prop="child" />);
+
+    await act(async () => {
+      // Inner offscreen is visible.
+      root.render(
+        <Offscreen mode={'hidden'}>
+          <Offscreen mode={'visible'}>
+            <ClassComponent />
+          </Offscreen>
+        </Offscreen>,
+      );
+    });
+
+    Scheduler.unstable_clearYields();
+
+    await act(async () => {
+      // Outer offscreen is visible.
+      // Inner offscreen is hidden.
+      root.render(
+        <Offscreen mode={'visible'}>
+          <Offscreen mode={'hidden'}>
+            <ClassComponent />
+          </Offscreen>
+        </Offscreen>,
+      );
+    });
+
+    expect(Scheduler).toHaveYielded(['child']);
+
+    await act(async () => {
+      // Outer offscreen is hidden.
+      // Inner offscreen is visible.
+      root.render(
+        <Offscreen mode={'hidden'}>
+          <Offscreen mode={'visible'}>
+            <ClassComponent />
+          </Offscreen>
+        </Offscreen>,
+      );
+    });
+
+    expect(Scheduler).toHaveYielded(['child']);
   });
 
   // @gate enableOffscreen