Do not unmount layout effects if ancestor Offscreen is hidden (#25628)

This is a follow up on facebook/react#25592 There is another condition Offscreen calls `recursivelyTraverseDisappearLayoutEffects` when it shouldn't. Offscreen may be nested. When nested Offscreen is hidden, it should only unmount layout effects if it meets following conditions: 1. This is an update, not first mount. 2. This Offscreen was hidden before. 3. No ancestor Offscreen is hidden. Previously, we were not accounting for the third condition.
jerrydev0927 · Dec 3, 2022 · c5d42f0 · c5d42f0
1 parent 271a127
commit c5d42f0
Show file tree

Hide file tree

Showing 3 changed files with 46 additions and 14 deletions.
diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js
@@ -2869,7 +2869,6 @@ function commitMutationEffectsOnFiber(
 
       if (flags & Visibility) {
         const offscreenInstance: OffscreenInstance = finishedWork.stateNode;
-        const offscreenBoundary: Fiber = finishedWork;
 
         // Track the current state on the Offscreen instance so we can
         // read it during an event
@@ -2880,11 +2879,16 @@ function commitMutationEffectsOnFiber(
         }
 
         if (isHidden) {
-          // Check if this is an update, and the tree was previously visible.
-          if (current !== null && !wasHidden) {
-            if ((offscreenBoundary.mode & ConcurrentMode) !== NoMode) {
+          const isUpdate = current !== null;
+          const isAncestorOffscreenHidden = offscreenSubtreeIsHidden;
+          // 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) {
+            if ((finishedWork.mode & ConcurrentMode) !== NoMode) {
               // Disappear the layout effects of all the children
-              recursivelyTraverseDisappearLayoutEffects(offscreenBoundary);
+              recursivelyTraverseDisappearLayoutEffects(finishedWork);
             }
           }
         } else {
@@ -2897,7 +2901,7 @@ function commitMutationEffectsOnFiber(
         if (supportsMutation && !isOffscreenManual(finishedWork)) {
           // TODO: This needs to run whenever there's an insertion or update
           // inside a hidden Offscreen tree.
-          hideOrUnhideAllChildren(offscreenBoundary, isHidden);
+          hideOrUnhideAllChildren(finishedWork, isHidden);
         }
       }
 

diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.old.js b/packages/react-reconciler/src/ReactFiberCommitWork.old.js
@@ -2869,7 +2869,6 @@ function commitMutationEffectsOnFiber(
 
       if (flags & Visibility) {
         const offscreenInstance: OffscreenInstance = finishedWork.stateNode;
-        const offscreenBoundary: Fiber = finishedWork;
 
         // Track the current state on the Offscreen instance so we can
         // read it during an event
@@ -2880,11 +2879,16 @@ function commitMutationEffectsOnFiber(
         }
 
         if (isHidden) {
-          // Check if this is an update, and the tree was previously visible.
-          if (current !== null && !wasHidden) {
-            if ((offscreenBoundary.mode & ConcurrentMode) !== NoMode) {
+          const isUpdate = current !== null;
+          const isAncestorOffscreenHidden = offscreenSubtreeIsHidden;
+          // 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) {
+            if ((finishedWork.mode & ConcurrentMode) !== NoMode) {
               // Disappear the layout effects of all the children
-              recursivelyTraverseDisappearLayoutEffects(offscreenBoundary);
+              recursivelyTraverseDisappearLayoutEffects(finishedWork);
             }
           }
         } else {
@@ -2897,7 +2901,7 @@ function commitMutationEffectsOnFiber(
         if (supportsMutation && !isOffscreenManual(finishedWork)) {
           // TODO: This needs to run whenever there's an insertion or update
           // inside a hidden Offscreen tree.
-          hideOrUnhideAllChildren(offscreenBoundary, isHidden);
+          hideOrUnhideAllChildren(finishedWork, isHidden);
         }
       }
 

diff --git a/packages/react-reconciler/src/__tests__/ReactOffscreen-test.js b/packages/react-reconciler/src/__tests__/ReactOffscreen-test.js
@@ -281,13 +281,21 @@ describe('ReactOffscreen', () => {
       componentWillUnmount() {
         Scheduler.unstable_yieldValue('componentWillUnmount');
       }
+
+      componentDidMount() {
+        Scheduler.unstable_yieldValue('componentDidMount');
+      }
     }
 
+    let _setIsVisible;
+    let isFirstRender = true;
+
     function App() {
       const [isVisible, setIsVisible] = React.useState(true);
-
-      if (isVisible === true) {
+      _setIsVisible = setIsVisible;
+      if (isFirstRender === true) {
         setIsVisible(false);
+        isFirstRender = false;
       }
 
       return (
@@ -303,7 +311,23 @@ describe('ReactOffscreen', () => {
     await act(async () => {
       root.render(<App />);
     });
+
+    expect(Scheduler).toHaveYielded(['Child']);
+    expect(root).toMatchRenderedOutput(<span hidden={true} prop="Child" />);
+
+    await act(async () => {
+      _setIsVisible(true);
+    });
+
+    expect(Scheduler).toHaveYielded(['Child']);
+    expect(root).toMatchRenderedOutput(<span hidden={true} prop="Child" />);
+
+    await act(async () => {
+      _setIsVisible(false);
+    });
+
     expect(Scheduler).toHaveYielded(['Child']);
+    expect(root).toMatchRenderedOutput(<span hidden={true} prop="Child" />);
   });
 
   // @gate enableOffscreen