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.
facebook · Nov 10, 2022 · d1e35c7 · d1e35c7
1 parent 1e3e30d
commit d1e35c7
Show file tree

Hide file tree

Showing 3 changed files with 75 additions and 31 deletions.
diff --git 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
@@ -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
@@ -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;
+    const root = ReactNoop.createRoot();
+    await act(async () => {
+      // Outer and inner offscreen are hidden.
+      root.render(
+        <Offscreen mode={'hidden'}>
+          <Offscreen mode={'hidden'}>
+            <ClassComponent />
+          </Offscreen>
+        </Offscreen>,
+      );
+    });
 
-    function App() {
-      const [isVisible, setIsVisible] = React.useState(true);
-      _setIsVisible = setIsVisible;
-      if (isFirstRender === true) {
-        setIsVisible(false);
-        isFirstRender = false;
-      }
+    expect(Scheduler).toHaveYielded(['child']);
+    expect(root).toMatchRenderedOutput(<span hidden={true} prop="child" />);
 
-      return (
-        <Offscreen mode="hidden">
-          <Offscreen mode={isVisible ? 'visible' : 'hidden'}>
+    await act(async () => {
+      // Inner offscreen is visible.
+      root.render(
+        <Offscreen mode={'hidden'}>
+          <Offscreen mode={'visible'}>
             <ClassComponent />
           </Offscreen>
-        </Offscreen>
+        </Offscreen>,
       );
-    }
+    });
+
+    expect(Scheduler).toHaveYielded(['child']);
+    expect(root).toMatchRenderedOutput(<span hidden={true} prop="child" />);
 
-    const root = ReactNoop.createRoot();
     await act(async () => {
-      root.render(<App />);
+      // 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 () => {
-      _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" />);
+    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 () => {
-      _setIsVisible(false);
+      // Outer offscreen is hidden.
+      // 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']);
   });
 
   // @gate enableOffscreen