Repository: facebook/react
Pull request: Defer hidden setState callbacks until component is visible

A class component `setState` callback should not fire if a component is inside a hidden Offscreen tree. Instead, it should wait until the next time the component is made visible.

Changed files:
filename: packages/react-reconciler/src/ReactFiberClassUpdateQueue.new.js
patch: @@ -102,7 +102,12 @@ import {
   enterDisallowedContextReadInDEV,
   exitDisallowedContextReadInDEV,
 } from './ReactFiberNewContext.new';
-import {Callback, ShouldCapture, DidCapture} from './ReactFiberFlags';
+import {
+  Callback,
+  Visibility,
+  ShouldCapture,
+  DidCapture,
+} from './ReactFiberFlags';
 
 import {debugRenderPhaseSideEffectsForStrictMode} from 'shared/ReactFeatureFlags';
 
@@ -136,14 +141,15 @@ export type Update<State> = {|
 export type SharedQueue<State> = {|
   pending: Update<State> | null,
   lanes: Lanes,
+  hiddenCallbacks: Array<() => mixed> | null,
 |};
 
 export type UpdateQueue<State> = {|
   baseState: State,
   firstBaseUpdate: Update<State> | null,
   lastBaseUpdate: Update<State> | null,
   shared: SharedQueue<State>,
-  effects: Array<Update<State>> | null,
+  callbacks: Array<() => mixed> | null,
 |};
 
 export const UpdateState = 0;
@@ -175,8 +181,9 @@ export function initializeUpdateQueue<State>(fiber: Fiber): void {
     shared: {
       pending: null,
       lanes: NoLanes,
+      hiddenCallbacks: null,
     },
-    effects: null,
+    callbacks: null,
   };
   fiber.updateQueue = queue;
 }
@@ -194,7 +201,7 @@ export function cloneUpdateQueue<State>(
       firstBaseUpdate: currentQueue.firstBaseUpdate,
       lastBaseUpdate: currentQueue.lastBaseUpdate,
       shared: currentQueue.shared,
-      effects: currentQueue.effects,
+      callbacks: null,
     };
     workInProgress.updateQueue = clone;
   }
@@ -326,7 +333,9 @@ export function enqueueCapturedUpdate<State>(
 
             tag: update.tag,
             payload: update.payload,
-            callback: update.callback,
+            // When this update is rebased, we should not fire its
+            // callback again.
+            callback: null,
 
             next: null,
           };
@@ -355,7 +364,7 @@ export function enqueueCapturedUpdate<State>(
         firstBaseUpdate: newFirst,
         lastBaseUpdate: newLast,
         shared: currentQueue.shared,
-        effects: currentQueue.effects,
+        callbacks: currentQueue.callbacks,
       };
       workInProgress.updateQueue = queue;
       return;
@@ -577,7 +586,10 @@ export function processUpdateQueue<State>(
 
             tag: update.tag,
             payload: update.payload,
-            callback: update.callback,
+
+            // When this update is rebased, we should not fire its
+            // callback again.
+            callback: null,
 
             next: null,
           };
@@ -594,18 +606,16 @@ export function processUpdateQueue<State>(
           instance,
         );
         const callback = update.callback;
-        if (
-          callback !== null &&
-          // If the update was already committed, we should not queue its
-          // callback again.
-          update.lane !== NoLane
-        ) {
+        if (callback !== null) {
           workInProgress.flags |= Callback;
-          const effects = queue.effects;
-          if (effects === null) {
-            queue.effects = [update];
+          if (isHiddenUpdate) {
+            workInProgress.flags |= Visibility;
+          }
+          const callbacks = queue.callbacks;
+          if (callbacks === null) {
+            queue.callbacks = [callback];
           } else {
-            effects.push(update);
+            callbacks.push(callback);
           }
         }
       }
@@ -679,22 +689,51 @@ export function checkHasForceUpdateAfterProcessing(): boolean {
   return hasForceUpdate;
 }
 
-export function commitUpdateQueue<State>(
-  finishedWork: Fiber,
-  finishedQueue: UpdateQueue<State>,
-  instance: any,
+export function deferHiddenCallbacks<State>(
+  updateQueue: UpdateQueue<State>,
 ): void {
-  // Commit the effects
-  const effects = finishedQueue.effects;
-  finishedQueue.effects = null;
-  if (effects !== null) {
-    for (let i = 0; i < effects.length; i++) {
-      const effect = effects[i];
-      const callback = effect.callback;
-      if (callback !== null) {
-        effect.callback = null;
-        callCallback(callback, instance);
-      }
+  // When an update finishes on a hidden component, its callback should not
+  // be fired until/unless the component is made visible again. Stash the
+  // callback on the shared queue object so it can be fired later.
+  const newHiddenCallbacks = updateQueue.callbacks;
+  if (newHiddenCallbacks !== null) {
+    const existingHiddenCallbacks = updateQueue.shared.hiddenCallbacks;
+    if (existingHiddenCallbacks === null) {
+      updateQueue.shared.hiddenCallbacks = newHiddenCallbacks;
+    } else {
+      updateQueue.shared.hiddenCallbacks = existingHiddenCallbacks.concat(
+        newHiddenCallbacks,
+      );
+    }
+  }
+}
+
+export function commitHiddenCallbacks<State>(
+  updateQueue: UpdateQueue<State>,
+  context: any,
+): void {
+  // This component is switching from hidden -> visible. Commit any callbacks
+  // that were previously deferred.
+  const hiddenCallbacks = updateQueue.shared.hiddenCallbacks;
+  if (hiddenCallbacks !== null) {
+    updateQueue.shared.hiddenCallbacks = null;
+    for (let i = 0; i < hiddenCallbacks.length; i++) {
+      const callback = hiddenCallbacks[i];
+      callCallback(callback, context);
+    }
+  }
+}
+
+export function commitCallbacks<State>(
+  updateQueue: UpdateQueue<State>,
+  context: any,
+): void {
+  const callbacks = updateQueue.callbacks;
+  if (callbacks !== null) {
+    updateQueue.callbacks = null;
+    for (let i = 0; i < callbacks.length; i++) {
+      const callback = callbacks[i];
+      callCallback(callback, context);
     }
   }
 }

filename: packages/react-reconciler/src/ReactFiberClassUpdateQueue.old.js
patch: @@ -102,7 +102,12 @@ import {
   enterDisallowedContextReadInDEV,
   exitDisallowedContextReadInDEV,
 } from './ReactFiberNewContext.old';
-import {Callback, ShouldCapture, DidCapture} from './ReactFiberFlags';
+import {
+  Callback,
+  Visibility,
+  ShouldCapture,
+  DidCapture,
+} from './ReactFiberFlags';
 
 import {debugRenderPhaseSideEffectsForStrictMode} from 'shared/ReactFeatureFlags';
 
@@ -136,14 +141,15 @@ export type Update<State> = {|
 export type SharedQueue<State> = {|
   pending: Update<State> | null,
   lanes: Lanes,
+  hiddenCallbacks: Array<() => mixed> | null,
 |};
 
 export type UpdateQueue<State> = {|
   baseState: State,
   firstBaseUpdate: Update<State> | null,
   lastBaseUpdate: Update<State> | null,
   shared: SharedQueue<State>,
-  effects: Array<Update<State>> | null,
+  callbacks: Array<() => mixed> | null,
 |};
 
 export const UpdateState = 0;
@@ -175,8 +181,9 @@ export function initializeUpdateQueue<State>(fiber: Fiber): void {
     shared: {
       pending: null,
       lanes: NoLanes,
+      hiddenCallbacks: null,
     },
-    effects: null,
+    callbacks: null,
   };
   fiber.updateQueue = queue;
 }
@@ -194,7 +201,7 @@ export function cloneUpdateQueue<State>(
       firstBaseUpdate: currentQueue.firstBaseUpdate,
       lastBaseUpdate: currentQueue.lastBaseUpdate,
       shared: currentQueue.shared,
-      effects: currentQueue.effects,
+      callbacks: null,
     };
     workInProgress.updateQueue = clone;
   }
@@ -326,7 +333,9 @@ export function enqueueCapturedUpdate<State>(
 
             tag: update.tag,
             payload: update.payload,
-            callback: update.callback,
+            // When this update is rebased, we should not fire its
+            // callback again.
+            callback: null,
 
             next: null,
           };
@@ -355,7 +364,7 @@ export function enqueueCapturedUpdate<State>(
         firstBaseUpdate: newFirst,
         lastBaseUpdate: newLast,
         shared: currentQueue.shared,
-        effects: currentQueue.effects,
+        callbacks: currentQueue.callbacks,
       };
       workInProgress.updateQueue = queue;
       return;
@@ -577,7 +586,10 @@ export function processUpdateQueue<State>(
 
             tag: update.tag,
             payload: update.payload,
-            callback: update.callback,
+
+            // When this update is rebased, we should not fire its
+            // callback again.
+            callback: null,
 
             next: null,
           };
@@ -594,18 +606,16 @@ export function processUpdateQueue<State>(
           instance,
         );
         const callback = update.callback;
-        if (
-          callback !== null &&
-          // If the update was already committed, we should not queue its
-          // callback again.
-          update.lane !== NoLane
-        ) {
+        if (callback !== null) {
           workInProgress.flags |= Callback;
-          const effects = queue.effects;
-          if (effects === null) {
-            queue.effects = [update];
+          if (isHiddenUpdate) {
+            workInProgress.flags |= Visibility;
+          }
+          const callbacks = queue.callbacks;
+          if (callbacks === null) {
+            queue.callbacks = [callback];
           } else {
-            effects.push(update);
+            callbacks.push(callback);
           }
         }
       }
@@ -679,22 +689,51 @@ export function checkHasForceUpdateAfterProcessing(): boolean {
   return hasForceUpdate;
 }
 
-export function commitUpdateQueue<State>(
-  finishedWork: Fiber,
-  finishedQueue: UpdateQueue<State>,
-  instance: any,
+export function deferHiddenCallbacks<State>(
+  updateQueue: UpdateQueue<State>,
 ): void {
-  // Commit the effects
-  const effects = finishedQueue.effects;
-  finishedQueue.effects = null;
-  if (effects !== null) {
-    for (let i = 0; i < effects.length; i++) {
-      const effect = effects[i];
-      const callback = effect.callback;
-      if (callback !== null) {
-        effect.callback = null;
-        callCallback(callback, instance);
-      }
+  // When an update finishes on a hidden component, its callback should not
+  // be fired until/unless the component is made visible again. Stash the
+  // callback on the shared queue object so it can be fired later.
+  const newHiddenCallbacks = updateQueue.callbacks;
+  if (newHiddenCallbacks !== null) {
+    const existingHiddenCallbacks = updateQueue.shared.hiddenCallbacks;
+    if (existingHiddenCallbacks === null) {
+      updateQueue.shared.hiddenCallbacks = newHiddenCallbacks;
+    } else {
+      updateQueue.shared.hiddenCallbacks = existingHiddenCallbacks.concat(
+        newHiddenCallbacks,
+      );
+    }
+  }
+}
+
+export function commitHiddenCallbacks<State>(
+  updateQueue: UpdateQueue<State>,
+  context: any,
+): void {
+  // This component is switching from hidden -> visible. Commit any callbacks
+  // that were previously deferred.
+  const hiddenCallbacks = updateQueue.shared.hiddenCallbacks;
+  if (hiddenCallbacks !== null) {
+    updateQueue.shared.hiddenCallbacks = null;
+    for (let i = 0; i < hiddenCallbacks.length; i++) {
+      const callback = hiddenCallbacks[i];
+      callCallback(callback, context);
+    }
+  }
+}
+
+export function commitCallbacks<State>(
+  updateQueue: UpdateQueue<State>,
+  context: any,
+): void {
+  const callbacks = updateQueue.callbacks;
+  if (callbacks !== null) {
+    updateQueue.callbacks = null;
+    for (let i = 0; i < callbacks.length; i++) {
+      const callback = callbacks[i];
+      callCallback(callback, context);
     }
   }
 }

filename: packages/react-reconciler/src/ReactFiberCommitWork.new.js
patch: @@ -75,6 +75,7 @@ import {
   ChildDeletion,
   Snapshot,
   Update,
+  Callback,
   Ref,
   Hydrating,
   Passive,
@@ -100,7 +101,11 @@ import {
   startPassiveEffectTimer,
 } from './ReactProfilerTimer.new';
 import {ConcurrentMode, NoMode, ProfileMode} from './ReactTypeOfMode';
-import {commitUpdateQueue} from './ReactFiberClassUpdateQueue.new';
+import {
+  deferHiddenCallbacks,
+  commitHiddenCallbacks,
+  commitCallbacks,
+} from './ReactFiberClassUpdateQueue.new';
 import {
   getPublicInstance,
   supportsMutation,
@@ -854,7 +859,7 @@ function commitLayoutEffectOnFiber(
         const updateQueue: UpdateQueue<
           *,
         > | null = (finishedWork.updateQueue: any);
-        if (updateQueue !== null) {
+        if (finishedWork.flags & Callback && updateQueue !== null) {
           if (__DEV__) {
             if (
               finishedWork.type === finishedWork.elementType &&
@@ -885,7 +890,7 @@ function commitLayoutEffectOnFiber(
           // We could update instance props and state here,
           // but instead we rely on them being set during last render.
           // TODO: revisit this when we implement resuming.
-          commitUpdateQueue(finishedWork, updateQueue, instance);
+          commitCallbacks(updateQueue, instance);
         }
         break;
       }
@@ -895,7 +900,7 @@ function commitLayoutEffectOnFiber(
         const updateQueue: UpdateQueue<
           *,
         > | null = (finishedWork.updateQueue: any);
-        if (updateQueue !== null) {
+        if (finishedWork.flags & Callback && updateQueue !== null) {
           let instance = null;
           if (finishedWork.child !== null) {
             switch (finishedWork.child.tag) {
@@ -907,7 +912,7 @@ function commitLayoutEffectOnFiber(
                 break;
             }
           }
-          commitUpdateQueue(finishedWork, updateQueue, instance);
+          commitCallbacks(updateQueue, instance);
         }
         break;
       }
@@ -1059,6 +1064,10 @@ function reappearLayoutEffectsOnFiber(node: Fiber) {
         safelyCallComponentDidMount(node, node.return, instance);
       }
       safelyAttachRef(node, node.return);
+      const updateQueue: UpdateQueue<*> | null = (node.updateQueue: any);
+      if (updateQueue !== null) {
+        commitHiddenCallbacks(updateQueue, instance);
+      }
       break;
     }
     case HostComponent: {
@@ -2155,6 +2164,15 @@ function commitMutationEffectsOnFiber(
           safelyDetachRef(current, current.return);
         }
       }
+
+      if (flags & Callback && offscreenSubtreeIsHidden) {
+        const updateQueue: UpdateQueue<
+          *,
+        > | null = (finishedWork.updateQueue: any);
+        if (updateQueue !== null) {
+          deferHiddenCallbacks(updateQueue);
+        }
+      }
       return;
     }
     case HostComponent: {
@@ -2341,16 +2359,21 @@ function commitMutationEffectsOnFiber(
       return;
     }
     case OffscreenComponent: {
+      const newState: OffscreenState | null = finishedWork.memoizedState;
+      const isHidden = newState !== null;
       const wasHidden = current !== null && current.memoizedState !== null;
 
       if (finishedWork.mode & ConcurrentMode) {
         // Before committing the children, track on the stack whether this
         // offscreen subtree was already hidden, so that we don't unmount the
         // effects again.
+        const prevOffscreenSubtreeIsHidden = offscreenSubtreeIsHidden;
         const prevOffscreenSubtreeWasHidden = offscreenSubtreeWasHidden;
+        offscreenSubtreeIsHidden = prevOffscreenSubtreeIsHidden || isHidden;
         offscreenSubtreeWasHidden = prevOffscreenSubtreeWasHidden || wasHidden;
         recursivelyTraverseMutationEffects(root, finishedWork, lanes);
         offscreenSubtreeWasHidden = prevOffscreenSubtreeWasHidden;
+        offscreenSubtreeIsHidden = prevOffscreenSubtreeIsHidden;
       } else {
         recursivelyTraverseMutationEffects(root, finishedWork, lanes);
       }
@@ -2359,8 +2382,6 @@ function commitMutationEffectsOnFiber(
 
       if (flags & Visibility) {
         const offscreenInstance: OffscreenInstance = finishedWork.stateNode;
-        const newState: OffscreenState | null = finishedWork.memoizedState;
-        const isHidden = newState !== null;
         const offscreenBoundary: Fiber = finishedWork;
 
         // Track the current state on the Offscreen instance so we can

filename: packages/react-reconciler/src/ReactFiberCommitWork.old.js
patch: @@ -75,6 +75,7 @@ import {
   ChildDeletion,
   Snapshot,
   Update,
+  Callback,
   Ref,
   Hydrating,
   Passive,
@@ -100,7 +101,11 @@ import {
   startPassiveEffectTimer,
 } from './ReactProfilerTimer.old';
 import {ConcurrentMode, NoMode, ProfileMode} from './ReactTypeOfMode';
-import {commitUpdateQueue} from './ReactFiberClassUpdateQueue.old';
+import {
+  deferHiddenCallbacks,
+  commitHiddenCallbacks,
+  commitCallbacks,
+} from './ReactFiberClassUpdateQueue.old';
 import {
   getPublicInstance,
   supportsMutation,
@@ -854,7 +859,7 @@ function commitLayoutEffectOnFiber(
         const updateQueue: UpdateQueue<
           *,
         > | null = (finishedWork.updateQueue: any);
-        if (updateQueue !== null) {
+        if (finishedWork.flags & Callback && updateQueue !== null) {
           if (__DEV__) {
             if (
               finishedWork.type === finishedWork.elementType &&
@@ -885,7 +890,7 @@ function commitLayoutEffectOnFiber(
           // We could update instance props and state here,
           // but instead we rely on them being set during last render.
           // TODO: revisit this when we implement resuming.
-          commitUpdateQueue(finishedWork, updateQueue, instance);
+          commitCallbacks(updateQueue, instance);
         }
         break;
       }
@@ -895,7 +900,7 @@ function commitLayoutEffectOnFiber(
         const updateQueue: UpdateQueue<
           *,
         > | null = (finishedWork.updateQueue: any);
-        if (updateQueue !== null) {
+        if (finishedWork.flags & Callback && updateQueue !== null) {
           let instance = null;
           if (finishedWork.child !== null) {
             switch (finishedWork.child.tag) {
@@ -907,7 +912,7 @@ function commitLayoutEffectOnFiber(
                 break;
             }
           }
-          commitUpdateQueue(finishedWork, updateQueue, instance);
+          commitCallbacks(updateQueue, instance);
         }
         break;
       }
@@ -1059,6 +1064,10 @@ function reappearLayoutEffectsOnFiber(node: Fiber) {
         safelyCallComponentDidMount(node, node.return, instance);
       }
       safelyAttachRef(node, node.return);
+      const updateQueue: UpdateQueue<*> | null = (node.updateQueue: any);
+      if (updateQueue !== null) {
+        commitHiddenCallbacks(updateQueue, instance);
+      }
       break;
     }
     case HostComponent: {
@@ -2130,6 +2139,15 @@ function commitMutationEffectsOnFiber(
           safelyDetachRef(current, current.return);
         }
       }
+
+      if (flags & Callback && offscreenSubtreeIsHidden) {
+        const updateQueue: UpdateQueue<
+          *,
+        > | null = (finishedWork.updateQueue: any);
+        if (updateQueue !== null) {
+          deferHiddenCallbacks(updateQueue);
+        }
+      }
       return;
     }
     case HostComponent: {
@@ -2312,16 +2330,21 @@ function commitMutationEffectsOnFiber(
       return;
     }
     case OffscreenComponent: {
+      const newState: OffscreenState | null = finishedWork.memoizedState;
+      const isHidden = newState !== null;
       const wasHidden = current !== null && current.memoizedState !== null;
 
       if (finishedWork.mode & ConcurrentMode) {
         // Before committing the children, track on the stack whether this
         // offscreen subtree was already hidden, so that we don't unmount the
         // effects again.
+        const prevOffscreenSubtreeIsHidden = offscreenSubtreeIsHidden;
         const prevOffscreenSubtreeWasHidden = offscreenSubtreeWasHidden;
+        offscreenSubtreeIsHidden = prevOffscreenSubtreeIsHidden || isHidden;
         offscreenSubtreeWasHidden = prevOffscreenSubtreeWasHidden || wasHidden;
         recursivelyTraverseMutationEffects(root, finishedWork, lanes);
         offscreenSubtreeWasHidden = prevOffscreenSubtreeWasHidden;
+        offscreenSubtreeIsHidden = prevOffscreenSubtreeIsHidden;
       } else {
         recursivelyTraverseMutationEffects(root, finishedWork, lanes);
       }
@@ -2330,8 +2353,6 @@ function commitMutationEffectsOnFiber(
 
       if (flags & Visibility) {
         const offscreenInstance: OffscreenInstance = finishedWork.stateNode;
-        const newState: OffscreenState | null = finishedWork.memoizedState;
-        const isHidden = newState !== null;
         const offscreenBoundary: Fiber = finishedWork;
 
         // Track the current state on the Offscreen instance so we can

filename: packages/react-reconciler/src/__tests__/ReactOffscreen-test.js
patch: @@ -640,4 +640,60 @@ describe('ReactOffscreen', () => {
     });
     expect(root).toMatchRenderedOutput(null);
   });
+
+  // @gate enableOffscreen
+  it('class component setState callbacks do not fire until tree is visible', async () => {
+    const root = ReactNoop.createRoot();
+
+    let child;
+    class Child extends React.Component {
+      state = {text: 'A'};
+      render() {
+        child = this;
+        return <Text text={this.state.text} />;
+      }
+    }
+
+    // Initial render
+    await act(async () => {
+      root.render(
+        <Offscreen mode="hidden">
+          <Child />
+        </Offscreen>,
+      );
+    });
+    expect(Scheduler).toHaveYielded(['A']);
+    expect(root).toMatchRenderedOutput(<span hidden={true} prop="A" />);
+
+    // Schedule an update to a hidden class component. The update will finish
+    // rendering in the background, but the callback shouldn't fire yet, because
+    // the component isn't visible.
+    await act(async () => {
+      child.setState({text: 'B'}, () => {
+        Scheduler.unstable_yieldValue('B update finished');
+      });
+    });
+    expect(Scheduler).toHaveYielded(['B']);
+    expect(root).toMatchRenderedOutput(<span hidden={true} prop="B" />);
+
+    // Now reveal the hidden component. Simultaneously, schedule another
+    // update with a callback to the same component. When the component is
+    // revealed, both the B callback and C callback should fire, in that order.
+    await act(async () => {
+      root.render(
+        <Offscreen mode="visible">
+          <Child />
+        </Offscreen>,
+      );
+      child.setState({text: 'C'}, () => {
+        Scheduler.unstable_yieldValue('C update finished');
+      });
+    });
+    expect(Scheduler).toHaveYielded([
+      'C',
+      'B update finished',
+      'C update finished',
+    ]);
+    expect(root).toMatchRenderedOutput(<span prop="C" />);
+  });
 });