From 4bd245e9ee22458bcd5b68524c47eaaab2cf2058 Mon Sep 17 00:00:00 2001
From: Samuel Susla <samuel.susla@gmail.com>
Date: Fri, 4 Nov 2022 13:31:07 +0000
Subject: [PATCH] Do not unmount layout effects if ancestor Offscreen is hidden
 (#25628)

This is a follow up on https://github.com/facebook/react/pull/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.
---
 .../src/ReactFiberCommitWork.new.js           | 16 +++++++----
 .../src/ReactFiberCommitWork.old.js           | 16 +++++++----
 .../src/__tests__/ReactOffscreen-test.js      | 28 +++++++++++++++++--
 3 files changed, 46 insertions(+), 14 deletions(-)

diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js
index f3fa8fbb21552..0b885bf13488b 100644
--- 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
index ea1ff57f3d8db..cbbca982127f0 100644
--- 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
index f7ca087088a70..417f851ad45b7 100644
--- 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