From 5bb7348cc25f8cc5e1b2da0bd9fc4075576715ab Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Sebastian=20Markb=C3=A5ge?= <sema@fb.com>
Date: Tue, 3 Dec 2019 13:38:02 -0800
Subject: [PATCH] Don't group Idle/Offscreen work with other work (#17456)

When we suspend we always try a lower level but we shouldn't try offscreen.
---
 .../src/ReactFiberWorkLoop.js                 |  19 ++-
 ...tSuspenseWithNoopRenderer-test.internal.js | 115 +++++++++++++++++-
 packages/shared/ReactFeatureFlags.js          |   2 +
 .../forks/ReactFeatureFlags.native-fb.js      |   1 +
 .../forks/ReactFeatureFlags.native-oss.js     |   1 +
 .../forks/ReactFeatureFlags.persistent.js     |   1 +
 .../forks/ReactFeatureFlags.test-renderer.js  |   1 +
 .../ReactFeatureFlags.test-renderer.www.js    |   1 +
 .../shared/forks/ReactFeatureFlags.www.js     |   1 +
 9 files changed, 137 insertions(+), 5 deletions(-)

diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.js b/packages/react-reconciler/src/ReactFiberWorkLoop.js
index ad6ff4037edf1..8bf99ce1139bf 100644
--- a/packages/react-reconciler/src/ReactFiberWorkLoop.js
+++ b/packages/react-reconciler/src/ReactFiberWorkLoop.js
@@ -25,6 +25,7 @@ import {
   warnAboutUnmockedScheduler,
   flushSuspenseFallbacksInTests,
   disableSchedulerTimeoutBasedOnReactExpirationTime,
+  enableTrainModelFix,
 } from 'shared/ReactFeatureFlags';
 import ReactSharedInternals from 'shared/ReactSharedInternals';
 import invariant from 'shared/invariant';
@@ -539,9 +540,19 @@ function getNextRootExpirationTimeToWorkOn(root: FiberRoot): ExpirationTime {
   // on whichever is higher priority.
   const lastPingedTime = root.lastPingedTime;
   const nextKnownPendingLevel = root.nextKnownPendingLevel;
-  return lastPingedTime > nextKnownPendingLevel
-    ? lastPingedTime
-    : nextKnownPendingLevel;
+  const nextLevel =
+    lastPingedTime > nextKnownPendingLevel
+      ? lastPingedTime
+      : nextKnownPendingLevel;
+  if (
+    enableTrainModelFix &&
+    nextLevel <= Idle &&
+    firstPendingTime !== nextLevel
+  ) {
+    // Don't work on Idle/Never priority unless everything else is committed.
+    return NoWork;
+  }
+  return nextLevel;
 }
 
 // Use this function to schedule a task for a root. There's only one task per
@@ -2362,7 +2373,7 @@ export function pingSuspendedRoot(
   // Mark the time at which this ping was scheduled.
   root.lastPingedTime = suspendedTime;
 
-  if (root.finishedExpirationTime === suspendedTime) {
+  if (!enableTrainModelFix && root.finishedExpirationTime === suspendedTime) {
     // If there's a pending fallback waiting to commit, throw it away.
     root.finishedExpirationTime = NoWork;
     root.finishedWork = null;
diff --git a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js
index 86fb78244188d..97f247895d553 100644
--- a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js
+++ b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js
@@ -2222,7 +2222,8 @@ describe('ReactSuspenseWithNoopRenderer', () => {
     Scheduler.unstable_runWithPriority(Scheduler.unstable_IdlePriority, () =>
       ReactNoop.render(<Foo renderContent={2} />),
     );
-    expect(Scheduler).toFlushAndYield(['Suspend! [A]', 'Loading A...']);
+    // We won't even work on Idle priority.
+    expect(Scheduler).toFlushAndYield([]);
 
     // We're still suspended.
     expect(ReactNoop.getChildren()).toEqual([]);
@@ -2789,4 +2790,116 @@ describe('ReactSuspenseWithNoopRenderer', () => {
 
     expect(root).toMatchRenderedOutput(<span prop="Foo" />);
   });
+
+  it('should not render hidden content while suspended on higher pri', async () => {
+    function Offscreen() {
+      Scheduler.unstable_yieldValue('Offscreen');
+      return 'Offscreen';
+    }
+    function App({showContent}) {
+      React.useLayoutEffect(() => {
+        Scheduler.unstable_yieldValue('Commit');
+      });
+      return (
+        <>
+          <div hidden={true}>
+            <Offscreen />
+          </div>
+          <Suspense fallback={<Text text="Loading..." />}>
+            {showContent ? <AsyncText text="A" ms={2000} /> : null}
+          </Suspense>
+        </>
+      );
+    }
+
+    // Initial render.
+    ReactNoop.render(<App showContent={false} />);
+    expect(Scheduler).toFlushAndYieldThrough(['Commit']);
+    expect(ReactNoop).toMatchRenderedOutput(<div hidden={true} />);
+
+    // Start transition.
+    React.unstable_withSuspenseConfig(
+      () => {
+        ReactNoop.render(<App showContent={true} />);
+      },
+      {timeoutMs: 2000},
+    );
+
+    expect(Scheduler).toFlushAndYield(['Suspend! [A]', 'Loading...']);
+    Scheduler.unstable_advanceTime(2000);
+    await advanceTimers(2000);
+    expect(Scheduler).toHaveYielded(['Promise resolved [A]']);
+    expect(Scheduler).toFlushAndYieldThrough(['A', 'Commit']);
+    expect(ReactNoop).toMatchRenderedOutput(
+      <>
+        <div hidden={true} />
+        <span prop="A" />
+      </>,
+    );
+    expect(Scheduler).toFlushAndYield(['Offscreen']);
+    expect(ReactNoop).toMatchRenderedOutput(
+      <>
+        <div hidden={true}>Offscreen</div>
+        <span prop="A" />
+      </>,
+    );
+  });
+
+  it('should be able to unblock higher pri content before suspended hidden', async () => {
+    function Offscreen() {
+      Scheduler.unstable_yieldValue('Offscreen');
+      return 'Offscreen';
+    }
+    function App({showContent}) {
+      React.useLayoutEffect(() => {
+        Scheduler.unstable_yieldValue('Commit');
+      });
+      return (
+        <Suspense fallback={<Text text="Loading..." />}>
+          <div hidden={true}>
+            <AsyncText text="A" ms={2000} />
+            <Offscreen />
+          </div>
+          {showContent ? <AsyncText text="A" ms={2000} /> : null}
+        </Suspense>
+      );
+    }
+
+    // Initial render.
+    ReactNoop.render(<App showContent={false} />);
+    expect(Scheduler).toFlushAndYieldThrough(['Commit']);
+    expect(ReactNoop).toMatchRenderedOutput(<div hidden={true} />);
+
+    // Partially render through the hidden content.
+    expect(Scheduler).toFlushAndYieldThrough(['Suspend! [A]']);
+
+    // Start transition.
+    React.unstable_withSuspenseConfig(
+      () => {
+        ReactNoop.render(<App showContent={true} />);
+      },
+      {timeoutMs: 5000},
+    );
+
+    expect(Scheduler).toFlushAndYield(['Suspend! [A]', 'Loading...']);
+    Scheduler.unstable_advanceTime(2000);
+    await advanceTimers(2000);
+    expect(Scheduler).toHaveYielded(['Promise resolved [A]']);
+    expect(Scheduler).toFlushAndYieldThrough(['A', 'Commit']);
+    expect(ReactNoop).toMatchRenderedOutput(
+      <>
+        <div hidden={true} />
+        <span prop="A" />
+      </>,
+    );
+    expect(Scheduler).toFlushAndYield(['A', 'Offscreen']);
+    expect(ReactNoop).toMatchRenderedOutput(
+      <>
+        <div hidden={true}>
+          <span prop="A" />Offscreen
+        </div>
+        <span prop="A" />
+      </>,
+    );
+  });
 });
diff --git a/packages/shared/ReactFeatureFlags.js b/packages/shared/ReactFeatureFlags.js
index 3ac13c638ef00..a465b60035dc6 100644
--- a/packages/shared/ReactFeatureFlags.js
+++ b/packages/shared/ReactFeatureFlags.js
@@ -91,6 +91,8 @@ export const disableLegacyContext = false;
 
 export const disableSchedulerTimeoutBasedOnReactExpirationTime = false;
 
+export const enableTrainModelFix = __EXPERIMENTAL__;
+
 export const enableTrustedTypesIntegration = false;
 
 // Flag to turn event.target and event.currentTarget in ReactNative from a reactTag to a component instance
diff --git a/packages/shared/forks/ReactFeatureFlags.native-fb.js b/packages/shared/forks/ReactFeatureFlags.native-fb.js
index c65fd57fc9be3..acda11ca0a9c4 100644
--- a/packages/shared/forks/ReactFeatureFlags.native-fb.js
+++ b/packages/shared/forks/ReactFeatureFlags.native-fb.js
@@ -43,6 +43,7 @@ export const warnAboutDefaultPropsOnFunctionComponents = false;
 export const warnAboutStringRefs = false;
 export const disableLegacyContext = false;
 export const disableSchedulerTimeoutBasedOnReactExpirationTime = false;
+export const enableTrainModelFix = false;
 export const enableTrustedTypesIntegration = false;
 
 // Only used in www builds.
diff --git a/packages/shared/forks/ReactFeatureFlags.native-oss.js b/packages/shared/forks/ReactFeatureFlags.native-oss.js
index 086f657e06fbc..d1497f14fee1c 100644
--- a/packages/shared/forks/ReactFeatureFlags.native-oss.js
+++ b/packages/shared/forks/ReactFeatureFlags.native-oss.js
@@ -37,6 +37,7 @@ export const warnAboutDefaultPropsOnFunctionComponents = false;
 export const warnAboutStringRefs = false;
 export const disableLegacyContext = false;
 export const disableSchedulerTimeoutBasedOnReactExpirationTime = false;
+export const enableTrainModelFix = false;
 export const enableTrustedTypesIntegration = false;
 export const enableNativeTargetAsInstance = false;
 
diff --git a/packages/shared/forks/ReactFeatureFlags.persistent.js b/packages/shared/forks/ReactFeatureFlags.persistent.js
index c76f57f235a14..ed111e930d2ca 100644
--- a/packages/shared/forks/ReactFeatureFlags.persistent.js
+++ b/packages/shared/forks/ReactFeatureFlags.persistent.js
@@ -37,6 +37,7 @@ export const warnAboutDefaultPropsOnFunctionComponents = false;
 export const warnAboutStringRefs = false;
 export const disableLegacyContext = false;
 export const disableSchedulerTimeoutBasedOnReactExpirationTime = false;
+export const enableTrainModelFix = false;
 export const enableTrustedTypesIntegration = false;
 export const enableNativeTargetAsInstance = false;
 
diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.js
index 0fc30e1c14fc1..8896e48dc49a8 100644
--- a/packages/shared/forks/ReactFeatureFlags.test-renderer.js
+++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.js
@@ -37,6 +37,7 @@ export const warnAboutDefaultPropsOnFunctionComponents = false;
 export const warnAboutStringRefs = false;
 export const disableLegacyContext = false;
 export const disableSchedulerTimeoutBasedOnReactExpirationTime = false;
+export const enableTrainModelFix = false;
 export const enableTrustedTypesIntegration = false;
 export const enableNativeTargetAsInstance = false;
 
diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js
index 10e1b25245595..bf65b7c67f5a0 100644
--- a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js
+++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js
@@ -35,6 +35,7 @@ export const warnAboutDefaultPropsOnFunctionComponents = false;
 export const warnAboutStringRefs = false;
 export const disableLegacyContext = false;
 export const disableSchedulerTimeoutBasedOnReactExpirationTime = false;
+export const enableTrainModelFix = false;
 export const enableTrustedTypesIntegration = false;
 export const enableNativeTargetAsInstance = false;
 
diff --git a/packages/shared/forks/ReactFeatureFlags.www.js b/packages/shared/forks/ReactFeatureFlags.www.js
index bd49552fbc9a4..09684b7b3a106 100644
--- a/packages/shared/forks/ReactFeatureFlags.www.js
+++ b/packages/shared/forks/ReactFeatureFlags.www.js
@@ -16,6 +16,7 @@ export const {
   disableInputAttributeSyncing,
   enableTrustedTypesIntegration,
   enableSelectiveHydration,
+  enableTrainModelFix,
 } = require('ReactFeatureFlags');
 
 // In www, we have experimental support for gathering data