Idle updates should not be blocked by hidden work (#16871)

* Idle updates should not be blocked by hidden work

Use the special `Idle` expiration time for updates that are triggered at
Scheduler's `IdlePriority`, instead of `Never`.

The key difference between Idle and Never¹ is that Never work can be
committed in an inconsistent state without tearing the UI. The main
example is offscreen content, like a hidden subtree.

¹ "Never" isn't the best name. I originally called it that because it
"never" expires, but neither does Idle. Since it's mostly used for
offscreen subtrees, we could call it "Offscreen." However, it's also
used for dehydrated Suspense boundaries, which are inconsistent in the
sense that they haven't finished yet, but aren't visibly inconsistent
because the server rendered HTML matches what the hydrated tree would
look like.

* Reset as early as possible using local variable

* Updates in a hidden effect should be Idle

I had made them Never to avoid an extra render when a hidden effect
updates the hidden component -- if they are Idle, we have to render once
at Idle, which bails out on the hidden subtree, then again at Never to
actually process the update -- but the problem of needing an extra
render pass to bail out hidden updates already exists and we should fix
that properly instead of adding yet another special case.

packages/react-reconciler/src/ReactFiberExpirationTime.js

@@ -21,9 +21,16 @@ import {
 export type ExpirationTime = number;
 
 export const NoWork = 0;
-// TODO: Think of a better name for Never.
+// TODO: Think of a better name for Never. The key difference with Idle is that
+// Never work can be committed in an inconsistent state without tearing the UI.
+// The main example is offscreen content, like a hidden subtree. So one possible
+// name is Offscreen. However, it also includes dehydrated Suspense boundaries,
+// which are inconsistent in the sense that they haven't finished yet, but
+// aren't visibly inconsistent because the server rendered HTML matches what the
+// hydrated tree would look like.
 export const Never = 1;
-// TODO: Use the Idle expiration time for idle state updates
+// Idle is slightly higher priority than Never. It must completely finish in
+// order to be consistent.
 export const Idle = 2;
 export const Sync = MAX_SIGNED_31_BIT_INT;
 export const Batched = Sync - 1;
@@ -115,7 +122,7 @@ export function inferPriorityFromExpirationTime(
   if (expirationTime === Sync) {
     return ImmediatePriority;
   }
-  if (expirationTime === Never) {
+  if (expirationTime === Never || expirationTime === Idle) {
     return IdlePriority;
   }
   const msUntil =

packages/react-reconciler/src/ReactFiberWorkLoop.js

@@ -321,6 +321,7 @@ export function computeExpirationForFiber(
 
   if ((executionContext & RenderContext) !== NoContext) {
     // Use whatever time we're already rendering
+    // TODO: Should there be a way to opt out, like with `runWithPriority`?
     return renderExpirationTime;
   }
 
@@ -347,7 +348,7 @@ export function computeExpirationForFiber(
         expirationTime = computeAsyncExpiration(currentTime);
         break;
       case IdlePriority:
-        expirationTime = Never;
+        expirationTime = Idle;
         break;
       default:
         invariant(false, 'Expected a valid priority level');
@@ -1406,14 +1407,14 @@ export function markRenderEventTimeAndConfig(
 ): void {
   if (
     expirationTime < workInProgressRootLatestProcessedExpirationTime &&
-    expirationTime > Never
+    expirationTime > Idle
   ) {
     workInProgressRootLatestProcessedExpirationTime = expirationTime;
   }
   if (suspenseConfig !== null) {
     if (
       expirationTime < workInProgressRootLatestSuspenseTimeout &&
-      expirationTime > Never
+      expirationTime > Idle
     ) {
       workInProgressRootLatestSuspenseTimeout = expirationTime;
       // Most of the time we only have one config and getting wrong is not bad.
@@ -2203,24 +2204,25 @@ function commitLayoutEffects(
 }
 
 export function flushPassiveEffects() {
+  if (pendingPassiveEffectsRenderPriority !== NoPriority) {
+    const priorityLevel =
+      pendingPassiveEffectsRenderPriority > NormalPriority
+        ? NormalPriority
+        : pendingPassiveEffectsRenderPriority;
+    pendingPassiveEffectsRenderPriority = NoPriority;
+    return runWithPriority(priorityLevel, flushPassiveEffectsImpl);
+  }
+}
+
+function flushPassiveEffectsImpl() {
   if (rootWithPendingPassiveEffects === null) {
     return false;
   }
   const root = rootWithPendingPassiveEffects;
   const expirationTime = pendingPassiveEffectsExpirationTime;
-  const renderPriorityLevel = pendingPassiveEffectsRenderPriority;
   rootWithPendingPassiveEffects = null;
   pendingPassiveEffectsExpirationTime = NoWork;
-  pendingPassiveEffectsRenderPriority = NoPriority;
-  const priorityLevel =
-    renderPriorityLevel > NormalPriority ? NormalPriority : renderPriorityLevel;
-  return runWithPriority(
-    priorityLevel,
-    flushPassiveEffectsImpl.bind(null, root, expirationTime),
-  );
-}
 
-function flushPassiveEffectsImpl(root, expirationTime) {
   invariant(
     (executionContext & (RenderContext | CommitContext)) === NoContext,
     'Cannot flush passive effects while already rendering.',
@@ -2263,6 +2265,7 @@ function flushPassiveEffectsImpl(root, expirationTime) {
   }
 
   executionContext = prevExecutionContext;
+
   flushSyncCallbackQueue();
 
   // If additional passive effects were scheduled, increment a counter. If this

packages/react-reconciler/src/__tests__/ReactSchedulerIntegration-test.internal.js

@@ -352,4 +352,60 @@ describe('ReactSchedulerIntegration', () => {
     Scheduler.unstable_flushUntilNextPaint();
     expect(Scheduler).toHaveYielded(['A', 'B', 'C']);
   });
+
+  it('idle updates are not blocked by offscreen work', async () => {
+    function Text({text}) {
+      Scheduler.unstable_yieldValue(text);
+      return text;
+    }
+
+    function App({label}) {
+      return (
+        <>
+          <Text text={`Visible: ` + label} />
+          <div hidden={true}>
+            <Text text={`Hidden: ` + label} />
+          </div>
+        </>
+      );
+    }
+
+    const root = ReactNoop.createRoot();
+    await ReactNoop.act(async () => {
+      root.render(<App label="A" />);
+
+      // Commit the visible content
+      expect(Scheduler).toFlushUntilNextPaint(['Visible: A']);
+      expect(root).toMatchRenderedOutput(
+        <>
+          Visible: A
+          <div hidden={true} />
+        </>,
+      );
+
+      // Before the hidden content has a chance to render, schedule an
+      // idle update
+      runWithPriority(IdlePriority, () => {
+        root.render(<App label="B" />);
+      });
+
+      // The next commit should only include the visible content
+      expect(Scheduler).toFlushUntilNextPaint(['Visible: B']);
+      expect(root).toMatchRenderedOutput(
+        <>
+          Visible: B
+          <div hidden={true} />
+        </>,
+      );
+    });
+
+    // The hidden content commits later
+    expect(Scheduler).toHaveYielded(['Hidden: B']);
+    expect(root).toMatchRenderedOutput(
+      <>
+        Visible: B
+        <div hidden={true}>Hidden: B</div>
+      </>,
+    );
+  });
 });

packages/react/src/__tests__/ReactDOMTracing-test.internal.js

@@ -134,7 +134,12 @@ describe('ReactDOMTracing', () => {
         expect(
           onInteractionScheduledWorkCompleted,
         ).toHaveBeenLastNotifiedOfInteraction(interaction);
-        expect(onRender).toHaveBeenCalledTimes(3);
+        // TODO: This is 4 instead of 3 because this update was scheduled at
+        // idle priority, and idle updates are slightly higher priority than
+        // offscreen work. So it takes two render passes to finish it. Profiler
+        // calls `onRender` for the first render even though everything
+        // bails out.
+        expect(onRender).toHaveBeenCalledTimes(4);
         expect(onRender).toHaveLastRenderedWithInteractions(
           new Set([interaction]),
         );
@@ -281,7 +286,12 @@ describe('ReactDOMTracing', () => {
         expect(
           onInteractionScheduledWorkCompleted,
         ).toHaveBeenLastNotifiedOfInteraction(interaction);
-        expect(onRender).toHaveBeenCalledTimes(3);
+        // TODO: This is 4 instead of 3 because this update was scheduled at
+        // idle priority, and idle updates are slightly higher priority than
+        // offscreen work. So it takes two render passes to finish it. Profiler
+        // calls `onRender` for the first render even though everything
+        // bails out.
+        expect(onRender).toHaveBeenCalledTimes(4);
         expect(onRender).toHaveLastRenderedWithInteractions(
           new Set([interaction]),
         );

scripts/jest/matchers/schedulerTestMatchers.js

@@ -44,6 +44,15 @@ function toFlushAndYieldThrough(Scheduler, expectedYields) {
   });
 }
 
+function toFlushUntilNextPaint(Scheduler, expectedYields) {
+  assertYieldsWereCleared(Scheduler);
+  Scheduler.unstable_flushUntilNextPaint();
+  const actualYields = Scheduler.unstable_clearYields();
+  return captureAssertion(() => {
+    expect(actualYields).toEqual(expectedYields);
+  });
+}
+
 function toFlushWithoutYielding(Scheduler) {
   return toFlushAndYield(Scheduler, []);
 }
@@ -76,6 +85,7 @@ function toFlushAndThrow(Scheduler, ...rest) {
 module.exports = {
   toFlushAndYield,
   toFlushAndYieldThrough,
+  toFlushUntilNextPaint,
   toFlushWithoutYielding,
   toFlushExpired,
   toHaveYielded,