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

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.
facebook · May 14, 2020 · cd78e48 · cd78e48
1 parent 9e5b2c9
commit cd78e48
Show file tree

Hide file tree

Showing 15 changed files with 38 additions and 55 deletions.
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.