Unify use of _pendingVisibility in attach/detach

facebook · Nov 21, 2022 · f8118ec · f8118ec
1 parent cb3fdd7
commit f8118ec
Show file tree

Hide file tree

Showing 5 changed files with 41 additions and 69 deletions.
diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.new.js b/packages/react-reconciler/src/ReactFiberBeginWork.new.js
@@ -665,6 +665,12 @@ function updateOffscreenComponent(
   const prevState: OffscreenState | null =
     current !== null ? current.memoizedState : null;
 
+  // Offscreen stores pending changes to visibility in `_pendingVisibility`. This is
+  // to support batching of `attach` and `detach` calls.
+  workInProgress.stateNode._visibility &= ~OffscreenDetached;
+  workInProgress.stateNode._visibility |=
+    workInProgress.stateNode._pendingVisibility & OffscreenDetached;
+
   markRef(current, workInProgress);
 
   if (

diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.old.js b/packages/react-reconciler/src/ReactFiberBeginWork.old.js
@@ -665,6 +665,12 @@ function updateOffscreenComponent(
   const prevState: OffscreenState | null =
     current !== null ? current.memoizedState : null;
 
+  // Offscreen stores pending changes to visibility in `_pendingVisibility`. This is
+  // to support batching of `attach` and `detach` calls.
+  workInProgress.stateNode._visibility &= ~OffscreenDetached;
+  workInProgress.stateNode._visibility |=
+    workInProgress.stateNode._pendingVisibility & OffscreenDetached;
+
   markRef(current, workInProgress);
 
   if (

diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js
@@ -153,7 +153,6 @@ import {
   clearSingleton,
   acquireSingletonInstance,
   releaseSingletonInstance,
-  scheduleMicrotask,
 } from './ReactFiberHostConfig';
 import {
   captureCommitPhaseError,
@@ -2423,26 +2422,16 @@ export function detachOffscreenInstance(instance: OffscreenInstance): void {
     );
   }
 
-  if ((instance._visibility & OffscreenDetached) !== NoFlags) {
+  if ((instance._pendingVisibility & OffscreenDetached) !== NoFlags) {
     // The instance is already detached, this is a noop.
     return;
   }
 
-  instance._pendingVisibility |= OffscreenDetached;
-
-  // Detaching needs to be postoned in case attach is called before next update.
-  scheduleMicrotask(() => {
-    if ((instance._pendingVisibility & OffscreenDetached) === NoFlags) {
-      // Attach was called. Offscreen does not need to be detached.
-      return;
-    }
-
-    instance._visibility |= OffscreenDetached;
-    const root = enqueueConcurrentRenderForLane(fiber, SyncLane);
-    if (root !== null) {
-      scheduleUpdateOnFiber(root, fiber, SyncLane, NoTimestamp);
-    }
-  });
+  const root = enqueueConcurrentRenderForLane(fiber, SyncLane);
+  if (root !== null) {
+    instance._pendingVisibility |= OffscreenDetached;
+    scheduleUpdateOnFiber(root, fiber, SyncLane, NoTimestamp);
+  }
 }
 
 export function attachOffscreenInstance(instance: OffscreenInstance): void {
@@ -2453,16 +2442,14 @@ export function attachOffscreenInstance(instance: OffscreenInstance): void {
     );
   }
 
-  instance._pendingVisibility &= ~OffscreenDetached;
-
-  if ((instance._visibility & OffscreenDetached) === NoFlags) {
+  if ((instance._pendingVisibility & OffscreenDetached) === NoFlags) {
     // The instance is already attached, this is a noop.
     return;
   }
 
   const root = enqueueConcurrentRenderForLane(fiber, SyncLane);
   if (root !== null) {
-    instance._visibility &= ~OffscreenDetached;
+    instance._pendingVisibility &= ~OffscreenDetached;
     scheduleUpdateOnFiber(root, fiber, SyncLane, NoTimestamp);
   }
 }

diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.old.js b/packages/react-reconciler/src/ReactFiberCommitWork.old.js
@@ -153,7 +153,6 @@ import {
   clearSingleton,
   acquireSingletonInstance,
   releaseSingletonInstance,
-  scheduleMicrotask,
 } from './ReactFiberHostConfig';
 import {
   captureCommitPhaseError,
@@ -2423,26 +2422,16 @@ export function detachOffscreenInstance(instance: OffscreenInstance): void {
     );
   }
 
-  if ((instance._visibility & OffscreenDetached) !== NoFlags) {
+  if ((instance._pendingVisibility & OffscreenDetached) !== NoFlags) {
     // The instance is already detached, this is a noop.
     return;
   }
 
-  instance._pendingVisibility |= OffscreenDetached;
-
-  // Detaching needs to be postoned in case attach is called before next update.
-  scheduleMicrotask(() => {
-    if ((instance._pendingVisibility & OffscreenDetached) === NoFlags) {
-      // Attach was called. Offscreen does not need to be detached.
-      return;
-    }
-
-    instance._visibility |= OffscreenDetached;
-    const root = enqueueConcurrentRenderForLane(fiber, SyncLane);
-    if (root !== null) {
-      scheduleUpdateOnFiber(root, fiber, SyncLane, NoTimestamp);
-    }
-  });
+  const root = enqueueConcurrentRenderForLane(fiber, SyncLane);
+  if (root !== null) {
+    instance._pendingVisibility |= OffscreenDetached;
+    scheduleUpdateOnFiber(root, fiber, SyncLane, NoTimestamp);
+  }
 }
 
 export function attachOffscreenInstance(instance: OffscreenInstance): void {
@@ -2453,16 +2442,14 @@ export function attachOffscreenInstance(instance: OffscreenInstance): void {
     );
   }
 
-  instance._pendingVisibility &= ~OffscreenDetached;
-
-  if ((instance._visibility & OffscreenDetached) === NoFlags) {
+  if ((instance._pendingVisibility & OffscreenDetached) === NoFlags) {
     // The instance is already attached, this is a noop.
     return;
   }
 
   const root = enqueueConcurrentRenderForLane(fiber, SyncLane);
   if (root !== null) {
-    instance._visibility &= ~OffscreenDetached;
+    instance._pendingVisibility &= ~OffscreenDetached;
     scheduleUpdateOnFiber(root, fiber, SyncLane, NoTimestamp);
   }
 }

diff --git a/packages/react-reconciler/src/__tests__/ReactOffscreen-test.js b/packages/react-reconciler/src/__tests__/ReactOffscreen-test.js
@@ -1663,43 +1663,29 @@ describe('ReactOffscreen', () => {
 
       nextRenderTriggerDetach = true;
 
-      // Offscreen is attached. State updates from offscreen are **not defered**.
-      // Offscreen is detached inside useLayoutEffect;
+      // Offscreen is attached and gets detached inside useLayoutEffect.
+      // State updates from offscreen are **defered**.
       await act(async () => {
         updateChildState(1);
         updateHighPriorityComponentState(1);
         expect(Scheduler).toFlushUntilNextPaint([
           'HighPriorityComponent 1',
           'Child 1',
           'HighPriorityComponent 2',
-          'Child 2',
         ]);
         expect(root).toMatchRenderedOutput(
           <>
             <span prop="HighPriorityComponent 2" />
-            <span prop="Child 2" />
-          </>,
-        );
-      });
-
-      // Offscreen is detached. State updates from offscreen are **defered**.
-      await act(async () => {
-        updateChildState(3);
-        updateHighPriorityComponentState(3);
-        expect(Scheduler).toFlushUntilNextPaint(['HighPriorityComponent 3']);
-        expect(root).toMatchRenderedOutput(
-          <>
-            <span prop="HighPriorityComponent 3" />
-            <span prop="Child 2" />
+            <span prop="Child 1" />
           </>,
         );
       });
 
-      expect(Scheduler).toHaveYielded(['Child 3']);
+      expect(Scheduler).toHaveYielded(['Child 2']);
       expect(root).toMatchRenderedOutput(
         <>
-          <span prop="HighPriorityComponent 3" />
-          <span prop="Child 3" />
+          <span prop="HighPriorityComponent 2" />
+          <span prop="Child 2" />
         </>,
       );
 
@@ -1708,16 +1694,16 @@ describe('ReactOffscreen', () => {
       // Offscreen is detached. State updates from offscreen are **defered**.
       // Offscreen is attached inside useLayoutEffect;
       await act(async () => {
-        updateChildState(4);
-        updateHighPriorityComponentState(4);
+        updateChildState(3);
+        updateHighPriorityComponentState(3);
         expect(Scheduler).toFlushUntilNextPaint([
-          'HighPriorityComponent 4',
-          'Child 4',
+          'HighPriorityComponent 3',
+          'Child 3',
         ]);
         expect(root).toMatchRenderedOutput(
           <>
-            <span prop="HighPriorityComponent 4" />
-            <span prop="Child 4" />
+            <span prop="HighPriorityComponent 3" />
+            <span prop="Child 3" />
           </>,
         );
       });
@@ -2043,7 +2029,7 @@ describe('ReactOffscreen', () => {
       instance.detach();
     });
 
-    expect(Scheduler).toHaveYielded(['attach child', 'detach child']);
+    expect(Scheduler).toHaveYielded([]);
   });
 
   // @gate enableOffscreen