Do not unmount layout effects on initial Offscreen mount (#25592)

`wasHidden` is evaluted to false if `current` is null. This means Offscreen has never been shown but this code assumes it is going from 'visible' to 'hidden' and unmounts layout effects. To fix this, only unmount layout effects if `current` is not null. I'm not able to repro this problem or write unit test for it. I see this crash bug in production test. The problem with repro is that if Offscreen starts as hidden, it's render is deferred and current is no longer null.
facebook · Nov 1, 2022 · ab075a2 · ab075a2
1 parent 765805b
commit ab075a2
Show file tree

Hide file tree

Showing 3 changed files with 42 additions and 2 deletions.
diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js
@@ -2880,7 +2880,8 @@ function commitMutationEffectsOnFiber(
         }
 
         if (isHidden) {
-          if (!wasHidden) {
+          // Check if this is an update, and the tree was previously visible.
+          if (current !== null && !wasHidden) {
             if ((offscreenBoundary.mode & ConcurrentMode) !== NoMode) {
               // Disappear the layout effects of all the children
               recursivelyTraverseDisappearLayoutEffects(offscreenBoundary);

diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.old.js b/packages/react-reconciler/src/ReactFiberCommitWork.old.js
@@ -2880,7 +2880,8 @@ function commitMutationEffectsOnFiber(
         }
 
         if (isHidden) {
-          if (!wasHidden) {
+          // Check if this is an update, and the tree was previously visible.
+          if (current !== null && !wasHidden) {
             if ((offscreenBoundary.mode & ConcurrentMode) !== NoMode) {
               // Disappear the layout effects of all the children
               recursivelyTraverseDisappearLayoutEffects(offscreenBoundary);

diff --git a/packages/react-reconciler/src/__tests__/ReactOffscreen-test.js b/packages/react-reconciler/src/__tests__/ReactOffscreen-test.js
@@ -268,6 +268,44 @@ describe('ReactOffscreen', () => {
     expect(root).toMatchRenderedOutput(<span prop="Child" />);
   });
 
+  // @gate enableOffscreen
+  it('nested offscreen does not call componentWillUnmount when hidden', async () => {
+    // This is a bug that appeared during production test of <unstable_Offscreen />.
+    // It is a very specific scenario with nested Offscreens. The inner offscreen
+    // goes from visible to hidden in synchronous update.
+    class ClassComponent extends React.Component {
+      render() {
+        return <Text text="Child" />;
+      }
+
+      componentWillUnmount() {
+        Scheduler.unstable_yieldValue('componentWillUnmount');
+      }
+    }
+
+    function App() {
+      const [isVisible, setIsVisible] = React.useState(true);
+
+      if (isVisible === true) {
+        setIsVisible(false);
+      }
+
+      return (
+        <Offscreen mode="hidden">
+          <Offscreen mode={isVisible ? 'visible' : 'hidden'}>
+            <ClassComponent />
+          </Offscreen>
+        </Offscreen>
+      );
+    }
+
+    const root = ReactNoop.createRoot();
+    await act(async () => {
+      root.render(<App />);
+    });
+    expect(Scheduler).toHaveYielded(['Child']);
+  });
+
   // @gate enableOffscreen
   it('mounts/unmounts layout effects when visibility changes (starting hidden)', async () => {
     function Child({text}) {