From f8118ecb21553e74358b93d6668e4320485868cb Mon Sep 17 00:00:00 2001 From: Samuel Susla Date: Mon, 21 Nov 2022 13:30:37 +0000 Subject: [PATCH] Unify use of _pendingVisibility in attach/detach --- .../src/ReactFiberBeginWork.new.js | 6 +++ .../src/ReactFiberBeginWork.old.js | 6 +++ .../src/ReactFiberCommitWork.new.js | 29 ++++---------- .../src/ReactFiberCommitWork.old.js | 29 ++++---------- .../src/__tests__/ReactOffscreen-test.js | 40 ++++++------------- 5 files changed, 41 insertions(+), 69 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.new.js b/packages/react-reconciler/src/ReactFiberBeginWork.new.js index 27c086160ffe0..e053b839a35e4 100644 --- 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 index c3fed9d3c46a1..0347197ce0116 100644 --- 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 index 040f096abd3d3..70896ce112268 100644 --- 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 index 7cb6ff6328ac2..aa77c0f1dfd67 100644 --- 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 index c44f261337fcc..d723c44a5d308 100644 --- a/packages/react-reconciler/src/__tests__/ReactOffscreen-test.js +++ b/packages/react-reconciler/src/__tests__/ReactOffscreen-test.js @@ -1663,8 +1663,8 @@ 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); @@ -1672,34 +1672,20 @@ describe('ReactOffscreen', () => { 'HighPriorityComponent 1', 'Child 1', 'HighPriorityComponent 2', - 'Child 2', ]); expect(root).toMatchRenderedOutput( <> - - , - ); - }); - - // Offscreen is detached. State updates from offscreen are **defered**. - await act(async () => { - updateChildState(3); - updateHighPriorityComponentState(3); - expect(Scheduler).toFlushUntilNextPaint(['HighPriorityComponent 3']); - expect(root).toMatchRenderedOutput( - <> - - + , ); }); - expect(Scheduler).toHaveYielded(['Child 3']); + expect(Scheduler).toHaveYielded(['Child 2']); expect(root).toMatchRenderedOutput( <> - - + + , ); @@ -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( <> - - + + , ); }); @@ -2043,7 +2029,7 @@ describe('ReactOffscreen', () => { instance.detach(); }); - expect(Scheduler).toHaveYielded(['attach child', 'detach child']); + expect(Scheduler).toHaveYielded([]); }); // @gate enableOffscreen