Disable <div hidden /> API in old fork, too

acdlite · acdlite · commit cd78e484f6f8 · 2020-05-13T18:58:30.000-07:00
The motivation for doing this is to make it impossible for additional
uses of pre-rendering to sneak into www without going through the
LegacyHidden abstraction. Since this feature was already disabled in
the new fork, this brings the two closer to parity.

The LegacyHidden abstraction itself still needs to opt into
pre-rendering somehow, so rather than totally disabling the feature, I
updated the `hidden` prop check to be obnoxiously specific. Before, you
could set it to any truthy value; now, you must set it to the string
"unstable-do-not-use-legacy-hidden".

The node will still be hidden in the DOM, since any truthy value will
cause the browser to apply a style of `display: none`.

I will have to update the LegacyHidden component in www to use the
obnoxious string prop. This doesn't block merge, though, since the
behavior is gated by a dynamic flag. I will update the component before
I enable the flag.
diff --git a/packages/react-dom/src/__tests__/ReactUpdates-test.js b/packages/react-dom/src/__tests__/ReactUpdates-test.js
@@ -29,15 +29,19 @@ describe('ReactUpdates', () => {
   function LegacyHiddenDiv({hidden, children, ...props}) {
     if (gate(flags => flags.new)) {
       return (
-        <div hidden={hidden} {...props}>
+        <div
+          hidden={hidden ? 'unstable-do-not-use-legacy-hidden' : false}
+          {...props}>
           <React.unstable_LegacyHidden mode={hidden ? 'hidden' : 'visible'}>
             {children}
           </React.unstable_LegacyHidden>
         </div>
       );
     } else {
       return (
-        <div hidden={hidden} {...props}>
+        <div
+          hidden={hidden ? 'unstable-do-not-use-legacy-hidden' : false}
+          {...props}>
           {children}
         </div>
       );
diff --git a/packages/react-dom/src/client/ReactDOMHostConfig.js b/packages/react-dom/src/client/ReactDOMHostConfig.js
@@ -78,6 +78,7 @@ import {
   enableModernEventSystem,
   enableCreateEventHandleAPI,
   enableScopeAPI,
+  disableHiddenPropDeprioritization,
 } from 'shared/ReactFeatureFlags';
 import {HostComponent, HostText} from 'react-reconciler/src/ReactWorkTags';
 import {TOP_BEFORE_BLUR, TOP_AFTER_BLUR} from '../events/DOMTopLevelEventTypes';
@@ -372,7 +373,14 @@ export function shouldSetTextContent(type: string, props: Props): boolean {
 }
 
 export function shouldDeprioritizeSubtree(type: string, props: Props): boolean {
-  return !!props.hidden;
+  if (disableHiddenPropDeprioritization) {
+    // This is obnoxiously specific so that nobody uses it, but we can still opt
+    // in via an infra-level userspace abstraction.
+    return props.hidden === 'unstable-do-not-use-legacy-hidden';
+  } else {
+    // Legacy behavior. Any truthy value works.
+    return !!props.hidden;
+  }
 }
 
 export function createTextInstance(
diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.new.js b/packages/react-reconciler/src/ReactFiberBeginWork.new.js
@@ -75,7 +75,6 @@ import {
   warnAboutDefaultPropsOnFunctionComponents,
   enableScopeAPI,
   enableBlocksAPI,
-  warnAboutDOMHiddenAttribute,
 } from 'shared/ReactFeatureFlags';
 import invariant from 'shared/invariant';
 import shallowEqual from 'shared/shallowEqual';
@@ -1124,22 +1123,6 @@ function updateHostComponent(
   }
 
   markRef(current, workInProgress);
-
-  if (__DEV__) {
-    if (
-      warnAboutDOMHiddenAttribute &&
-      (workInProgress.mode & ConcurrentMode) !== NoMode &&
-      nextProps.hasOwnProperty('hidden')
-    ) {
-      // This warning will not be user visible. Only exists so React Core team
-      // can find existing callers and migrate them to the new API.
-      console.error(
-        'Detected use of DOM `hidden` attribute. Should migrate to new API. ' +
-          '(owner: React Core)',
-      );
-    }
-  }
-
   reconcileChildren(current, workInProgress, nextChildren, renderLanes);
   return workInProgress.child;
 }
diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.old.js b/packages/react-reconciler/src/ReactFiberBeginWork.old.js
@@ -69,7 +69,6 @@ import {
   warnAboutDefaultPropsOnFunctionComponents,
   enableScopeAPI,
   enableBlocksAPI,
-  warnAboutDOMHiddenAttribute,
 } from 'shared/ReactFeatureFlags';
 import invariant from 'shared/invariant';
 import shallowEqual from 'shared/shallowEqual';
@@ -1100,21 +1099,6 @@ function updateHostComponent(current, workInProgress, renderExpirationTime) {
 
   markRef(current, workInProgress);
 
-  if (__DEV__) {
-    if (
-      warnAboutDOMHiddenAttribute &&
-      (workInProgress.mode & ConcurrentMode) !== NoMode &&
-      nextProps.hasOwnProperty('hidden')
-    ) {
-      // This warning will not be user visible. Only exists so React Core team
-      // can find existing callers and migrate them to the new API.
-      console.error(
-        'Detected use of DOM `hidden` attribute. Should migrate to new API. ' +
-          '(owner: React Core)',
-      );
-    }
-  }
-
   // Check the host config to see if the children are offscreen/hidden.
   if (
     workInProgress.mode & ConcurrentMode &&
diff --git a/packages/react-refresh/src/__tests__/ReactFresh-test.js b/packages/react-refresh/src/__tests__/ReactFresh-test.js
@@ -79,15 +79,19 @@ describe('ReactFresh', () => {
   function LegacyHiddenDiv({hidden, children, ...props}) {
     if (gate(flags => flags.new)) {
       return (
-        <div hidden={hidden} {...props}>
+        <div
+          hidden={hidden ? 'unstable-do-not-use-legacy-hidden' : false}
+          {...props}>
           <React.unstable_LegacyHidden mode={hidden ? 'hidden' : 'visible'}>
             {children}
           </React.unstable_LegacyHidden>
         </div>
       );
     } else {
       return (
-        <div hidden={hidden} {...props}>
+        <div
+          hidden={hidden ? 'unstable-do-not-use-legacy-hidden' : false}
+          {...props}>
           {children}
         </div>
       );
diff --git a/packages/react/src/__tests__/ReactDOMTracing-test.internal.js b/packages/react/src/__tests__/ReactDOMTracing-test.internal.js
@@ -59,15 +59,19 @@ function loadModules() {
 function LegacyHiddenDiv({hidden, children, ...props}) {
   if (gate(flags => flags.new)) {
     return (
-      <div hidden={hidden} {...props}>
+      <div
+        hidden={hidden ? 'unstable-do-not-use-legacy-hidden' : false}
+        {...props}>
         <React.unstable_LegacyHidden mode={hidden ? 'hidden' : 'visible'}>
           {children}
         </React.unstable_LegacyHidden>
       </div>
     );
   } else {
     return (
-      <div hidden={hidden} {...props}>
+      <div
+        hidden={hidden ? 'unstable-do-not-use-legacy-hidden' : false}
+        {...props}>
         {children}
       </div>
     );
diff --git a/packages/shared/ReactFeatureFlags.js b/packages/shared/ReactFeatureFlags.js
@@ -140,4 +140,4 @@ export const enableLegacyFBSupport = false;
 export const deferRenderPhaseUpdateToNextBatch = true;
 
 // Flag used by www build so we can log occurrences of legacy hidden API
-export const warnAboutDOMHiddenAttribute = false;
+export const disableHiddenPropDeprioritization = true;
diff --git a/packages/shared/forks/ReactFeatureFlags.native-fb.js b/packages/shared/forks/ReactFeatureFlags.native-fb.js
@@ -48,7 +48,7 @@ export const enableFilterEmptyStringAttributesDOM = false;
 
 export const enableNewReconciler = false;
 export const deferRenderPhaseUpdateToNextBatch = true;
-export const warnAboutDOMHiddenAttribute = false;
+export const disableHiddenPropDeprioritization = true;
 
 // Flow magic to verify the exports of this file match the original version.
 // eslint-disable-next-line no-unused-vars
diff --git a/packages/shared/forks/ReactFeatureFlags.native-oss.js b/packages/shared/forks/ReactFeatureFlags.native-oss.js
@@ -47,7 +47,7 @@ export const enableFilterEmptyStringAttributesDOM = false;
 
 export const enableNewReconciler = false;
 export const deferRenderPhaseUpdateToNextBatch = true;
-export const warnAboutDOMHiddenAttribute = false;
+export const disableHiddenPropDeprioritization = true;
 
 // Flow magic to verify the exports of this file match the original version.
 // eslint-disable-next-line no-unused-vars
diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.js
@@ -47,7 +47,7 @@ export const enableFilterEmptyStringAttributesDOM = false;
 
 export const enableNewReconciler = false;
 export const deferRenderPhaseUpdateToNextBatch = true;
-export const warnAboutDOMHiddenAttribute = false;
+export const disableHiddenPropDeprioritization = true;
 
 // Flow magic to verify the exports of this file match the original version.
 // eslint-disable-next-line no-unused-vars
diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js
@@ -47,7 +47,7 @@ export const enableFilterEmptyStringAttributesDOM = false;
 
 export const enableNewReconciler = false;
 export const deferRenderPhaseUpdateToNextBatch = true;
-export const warnAboutDOMHiddenAttribute = false;
+export const disableHiddenPropDeprioritization = true;
 
 // Flow magic to verify the exports of this file match the original version.
 // eslint-disable-next-line no-unused-vars
diff --git a/packages/shared/forks/ReactFeatureFlags.testing.js b/packages/shared/forks/ReactFeatureFlags.testing.js
@@ -47,7 +47,7 @@ export const enableFilterEmptyStringAttributesDOM = false;
 
 export const enableNewReconciler = false;
 export const deferRenderPhaseUpdateToNextBatch = true;
-export const warnAboutDOMHiddenAttribute = false;
+export const disableHiddenPropDeprioritization = true;
 
 // Flow magic to verify the exports of this file match the original version.
 // eslint-disable-next-line no-unused-vars
diff --git a/packages/shared/forks/ReactFeatureFlags.testing.www.js b/packages/shared/forks/ReactFeatureFlags.testing.www.js
@@ -47,7 +47,7 @@ export const enableFilterEmptyStringAttributesDOM = false;
 
 export const enableNewReconciler = false;
 export const deferRenderPhaseUpdateToNextBatch = true;
-export const warnAboutDOMHiddenAttribute = false;
+export const disableHiddenPropDeprioritization = true;
 
 // Flow magic to verify the exports of this file match the original version.
 // eslint-disable-next-line no-unused-vars
diff --git a/packages/shared/forks/ReactFeatureFlags.www-dynamic.js b/packages/shared/forks/ReactFeatureFlags.www-dynamic.js
@@ -21,6 +21,9 @@ export const enableModernEventSystem = __VARIANT__;
 export const enableLegacyFBSupport = __VARIANT__;
 export const enableDebugTracing = !__VARIANT__;
 
+// Temporary flag, in case we need to re-enable this feature.
+export const disableHiddenPropDeprioritization = __VARIANT__;
+
 // This only has an effect in the new reconciler. But also, the new reconciler
 // is only enabled when __VARIANT__ is true. So this is set to the opposite of
 // __VARIANT__ so that it's `false` when running against the new reconciler.
@@ -36,13 +39,6 @@ export const deferRenderPhaseUpdateToNextBatch = !__VARIANT__;
 export const debugRenderPhaseSideEffectsForStrictMode = __DEV__;
 export const replayFailedUnitOfWorkWithInvokeGuardedCallback = __DEV__;
 
-// Do not add the corresponding warning to the warning filter! Only exists so we
-// can detect callers and migrate them to the new API. Should not visible to
-// anyone outside React Core team.
-//
-// Disabled in our tests, but we'll enable in www.
-export const warnAboutDOMHiddenAttribute = false;
-
 // TODO: These flags are hard-coded to the default values used in open source.
 // Update the tests so that they pass in either mode, then set these
 // to __VARIANT__.
diff --git a/packages/shared/forks/ReactFeatureFlags.www.js b/packages/shared/forks/ReactFeatureFlags.www.js
@@ -27,7 +27,7 @@ export const {
   enableLegacyFBSupport,
   enableDebugTracing,
   deferRenderPhaseUpdateToNextBatch,
-  warnAboutDOMHiddenAttribute,
+  disableHiddenPropDeprioritization,
 } = dynamicFeatureFlags;
 
 // On WWW, __EXPERIMENTAL__ is used for a new modern build.
