From 487535a22a45cd7b87ce78853daa473c58b3a442 Mon Sep 17 00:00:00 2001 From: Josh Story Date: Fri, 3 Mar 2023 16:45:26 -0800 Subject: [PATCH] remove the concept of itemScope and categorically treat itemProp as an opt out --- .../src/client/ReactDOMHostConfig.js | 191 ++++++++++++------ .../src/server/ReactDOMServerFormatConfig.js | 55 +---- .../ReactDOMServerLegacyFormatConfig.js | 1 - .../src/__tests__/ReactDOMFloat-test.js | 140 ++++++------- .../ReactFiberHostConfigWithNoHydration.js | 2 +- .../src/ReactFiberHydrationContext.js | 7 +- .../src/forks/ReactFiberHostConfig.custom.js | 4 +- 7 files changed, 213 insertions(+), 187 deletions(-) diff --git a/packages/react-dom-bindings/src/client/ReactDOMHostConfig.js b/packages/react-dom-bindings/src/client/ReactDOMHostConfig.js index 972c444cacd46..01c7a3d70a825 100644 --- a/packages/react-dom-bindings/src/client/ReactDOMHostConfig.js +++ b/packages/react-dom-bindings/src/client/ReactDOMHostConfig.js @@ -18,6 +18,7 @@ import type { import type {ReactScopeInstance} from 'shared/ReactTypes'; import type {AncestorInfoDev} from './validateDOMNesting'; +import {checkAttributeStringCoercion} from 'shared/CheckStringCoercion'; import { precacheFiberNode, updateFiberProps, @@ -111,7 +112,6 @@ export type Props = { left?: null | number, right?: null | number, top?: null | number, - itemScope?: null | boolean, ... }; type RawProps = { @@ -241,17 +241,15 @@ export function getChildHostContext( } } -const NoContext /* */ = 0b000000; +const NoContext /* */ = 0b0000; // Element namespace contexts -const HTMLNamespaceContext /* */ = 0b000001; -const SVGNamespaceContext /* */ = 0b000010; -const MathNamespaceContext /* */ = 0b000100; -const AnyNamespaceContext /* */ = 0b000111; +const HTMLNamespaceContext /* */ = 0b0001; +const SVGNamespaceContext /* */ = 0b0010; +const MathNamespaceContext /* */ = 0b0100; +const AnyNamespaceContext /* */ = 0b0111; // Ancestor tag/attribute contexts -const HoistContext /* */ = 0b001000; // When we are directly inside the HostRoot or inside certain tags like , , and -const ItemInScope /* */ = 0b010000; // When we are inside an itemScope deeper than -const HeadOrBodyInScope /* */ = 0b100000; // When we are inside a or +const HoistContext /* */ = 0b1000; // When we are directly inside the HostRoot or inside certain tags like , , and function getNamespaceContext(namespaceURI: string) { switch (namespaceURI) { @@ -272,13 +270,7 @@ function getChildHostContextImpl( props: null | Props, ) { // We assume any child context is not a HoistContext. It will get re-added later if certain circumstances warrant it - let context = parentContext & ~HoistContext; - // We need to determine if we are now in an itemScope. - if (props && props.itemScope === true && parentContext & HeadOrBodyInScope) { - // We only allow itemscopes deeper than or . This is helpful to disambiguate - // resources that need to hoist vs those that do not - context |= ItemInScope; - } + const context = parentContext & ~HoistContext; const namespaceContext = context & AnyNamespaceContext; const otherContext = context & ~AnyNamespaceContext; @@ -293,12 +285,9 @@ function getChildHostContextImpl( case 'math': return MathNamespaceContext | otherContext; case 'html': - return HTMLNamespaceContext | otherContext | HoistContext; case 'head': case 'body': - return ( - HTMLNamespaceContext | otherContext | HoistContext | HeadOrBodyInScope - ); + return HTMLNamespaceContext | otherContext | HoistContext; default: return context; } @@ -934,14 +923,12 @@ export function isHydratable(type: string, props: Props): boolean { // In certain contexts, namely and , we want to skip past Nodes that are in theory // hydratable but do not match the current Fiber being hydrated. We track the hydratable node we // are currently attempting using this module global. If the hydration is unsuccessful Fiber will -// call getNextHydratableAfterFailedAttempt which uses this cursor to return the expected next +// call getLastAttemptedHydratable which uses this cursor to return the expected next // hydratable. let hydratableNode: null | HydratableInstance = null; -export function getNextHydratableAfterFailedAttempt(): null | HydratableInstance { - return hydratableNode === null - ? null - : getNextHydratableSibling(hydratableNode); +export function getLastAttemptedHydratable(): null | HydratableInstance { + return hydratableNode; } export function getNextMatchingHydratableInstance( @@ -950,6 +937,7 @@ export function getNextMatchingHydratableInstance( props: Props, hostContext: HostContext, ): null | Instance { + const anyProps = (props: any); // We set this first because it must always be set on every invocation hydratableNode = instance; @@ -980,7 +968,6 @@ export function getNextMatchingHydratableInstance( ) { // This is either text or a tag type that differs from the tag we are trying to hydrate // or a Node we already bound to a hoistable. We skip past it. - hydratableNode = getNextHydratableSibling(node); continue; } else { // We have an Element with the right type. @@ -991,68 +978,135 @@ export function getNextMatchingHydratableInstance( // using high entropy attributes for certain types. This technique will fail for strange insertions like // extension prepending
in the but that already breaks before and that is an edge case. switch (type) { - // We make a leap and assume that no titles or metas will ever hydrate as components in the or - // This is because the only way they would opt out of hoisting semantics is to be in svg, which is not possible - // in these scopes, or to have an itemProp while being in an itemScope. We define , , and as not - // supporting itemScope to make this a strict guarantee. Because we can never be in this position we elide the check here. - // case 'title': - // case 'meta': { - // continue; - // } + // case 'title': We assume all titles are matchable. You should only have one in the Document, at least in a hoistable scope + // and if you are a HostComponent with type title we must either be in an context or this title must have an `itemProp` prop. + case 'meta': { + if (__DEV__) { + checkAttributeStringCoercion(anyProps.content, 'content'); + } + if ( + element.getAttribute('content') !== + (anyProps.content == null ? null : '' + anyProps.content) + ) { + // Content is coerced to a string because it is similar to value. If something with toString is provided + // we want to get that string value before doing the comparison. The rest are less likely to be used in this fashion + // and we just check identity + continue; + } + if ( + element.getAttribute('name') !== + (anyProps.name == null ? null : anyProps.name) || + element.getAttribute('property') !== + (anyProps.property == null ? null : anyProps.property) || + element.getAttribute('itemprop') !== + (anyProps.itemProp == null ? null : anyProps.itemProp) || + element.getAttribute('http-equiv') !== + (anyProps.httpEquiv == null ? null : anyProps.httpEquiv) || + element.getAttribute('charset') !== + (anyProps.charSet == null ? null : anyProps.charSet) + ) { + continue; + } + return element; + } case 'link': { const rel = element.getAttribute('rel'); if ( rel === 'stylesheet' && element.hasAttribute('data-precedence') ) { - // This is a stylesheet resource and necessarily not a target for hydration of a component + // This is a stylesheet resource continue; } else if ( - rel !== (props: any).rel || - element.getAttribute('href') !== (props: any).href + rel !== anyProps.rel || + element.getAttribute('href') !== + (anyProps.href == null ? null : anyProps.href) || + element.getAttribute('crossorigin') !== + (anyProps.crossOrigin == null ? null : anyProps.crossOrigin) || + element.getAttribute('title') !== + (anyProps.title == null ? null : anyProps.title) ) { - // This link Node is definitely not a match for this rendered link + // rel + href should usually be enough to uniquely identify a link however crossOrigin can vary for rel preconnect + // and title could vary for rel alternate continue; } - break; + return element; } case 'style': { if (element.hasAttribute('data-precedence')) { - // This i a style resource necessarily not a target for hydration of a component - continue; - } else if ( - typeof props.children === 'string' && - element.textContent !== props.children - ) { - // The contents of this style Node do not match the contents of this rendered style + // This is a style resource continue; } + // We need to fall through to hit the textContent comparison case below. This switch is structured so that you either + // continue, or return, unless you need to do this child comparison. break; } case 'script': { - if (element.hasAttribute('async')) { - // This is an async script resource and necessarily not a target for hydration of a compoennt + const srcAttr = element.getAttribute('src'); + if ( + srcAttr && + element.hasAttribute('async') && + !element.hasAttribute('itemprop') + ) { + // This is an async script resource continue; } else if ( - typeof (props: any).src === 'string' && - element.getAttribute('src') !== (props: any).src + srcAttr !== (anyProps.src == null ? null : anyProps.src) || + element.getAttribute('type') !== + (anyProps.type == null ? null : anyProps.type) || + element.getAttribute('crossorigin') !== + (anyProps.crossOrigin == null ? null : anyProps.crossOrigin) ) { // This script is for a different src continue; } else if ( - typeof props.children === 'string' && - element.textContent !== props.children + typeof anyProps.children === 'string' && + element.textContent !== anyProps.children ) { // This is an inline script with different text content continue; } + // We need to fall through to hit the textContent comparison case below. This switch is structured so that you either + // continue, or return, unless you need to do this child comparison. break; } + default: { + // We have excluded the most likely cases of mismatch between hoistable tags, 3rd party script inserted tags, + // and browser extension inserted tags. While it is possible this is not the right match it is a decent hueristic + // that should work in the vast majority of cases. + return element; + } } - // We have excluded the most likely cases of mismatch between hoistable tags, 3rd party script inserted tags, - // and browser extension inserted tags. While it is possible this is not the right match it is a decent hueristic - // that should work in the vast majority of cases. + // The only branches that should fall through to here are those that need to check textContent against single value children + // in particular, or of the Document.', + type, + type, + ); + } + } return false; } @@ -1999,12 +2072,6 @@ export function resolveSingletonInstance( if (validateDOMNestingDev) { validateDOMNesting(type, null, hostContextDev.ancestorInfo); } - if (hostContextDev.context & ItemInScope && props.itemScope === true) { - console.error( - 'React expected the <%s> element to not have an `itemScope` prop but found this prop instead. React does not support itemScopes on , , or because it interferes with hoisting certain tags. Try moving the itemScope to an element just inside the instead.', - type, - ); - } } const ownerDocument = getOwnerDocumentFromRootContainer( rootContainerInstance, diff --git a/packages/react-dom-bindings/src/server/ReactDOMServerFormatConfig.js b/packages/react-dom-bindings/src/server/ReactDOMServerFormatConfig.js index 4e3cf36a31e2e..77a7067e5d413 100644 --- a/packages/react-dom-bindings/src/server/ReactDOMServerFormatConfig.js +++ b/packages/react-dom-bindings/src/server/ReactDOMServerFormatConfig.js @@ -333,20 +333,17 @@ export type FormatContext = { insertionMode: InsertionMode, // root/svg/html/mathml/table selectedValue: null | string | Array, // the selected value(s) inside a noscriptTagInScope: boolean, - itemInScope: boolean, }; function createFormatContext( insertionMode: InsertionMode, - selectedValue: null | string | Array, + selectedValue: null | string, noscriptTagInScope: boolean, - itemInScope: boolean, ): FormatContext { return { insertionMode, selectedValue, noscriptTagInScope, - itemInScope, }; } @@ -357,7 +354,7 @@ export function createRootFormatContext(namespaceURI?: string): FormatContext { : namespaceURI === 'http://www.w3.org/1998/Math/MathML' ? MATHML_MODE : ROOT_HTML_MODE; - return createFormatContext(insertionMode, null, false, false); + return createFormatContext(insertionMode, null, false); } export function getChildFormatContext( @@ -365,39 +362,32 @@ export function getChildFormatContext( type: string, props: Object, ): FormatContext { - const itemScoped = - parentContext.itemInScope || - (props.itemScope === true && parentContext.insertionMode > HTML_HTML_MODE); switch (type) { case 'noscript': - return createFormatContext(HTML_MODE, null, true, itemScoped); + return createFormatContext(HTML_MODE, null, true); case 'select': return createFormatContext( HTML_MODE, props.value != null ? props.value : props.defaultValue, parentContext.noscriptTagInScope, - itemScoped, ); case 'svg': return createFormatContext( SVG_MODE, null, parentContext.noscriptTagInScope, - itemScoped, ); case 'math': return createFormatContext( MATHML_MODE, null, parentContext.noscriptTagInScope, - itemScoped, ); case 'foreignObject': return createFormatContext( HTML_MODE, null, parentContext.noscriptTagInScope, - itemScoped, ); // Table parents are special in that their children can only be created at all if they're // wrapped in a table parent. So we need to encode that we're entering this mode. @@ -406,7 +396,6 @@ export function getChildFormatContext( HTML_TABLE_MODE, null, parentContext.noscriptTagInScope, - itemScoped, ); case 'thead': case 'tbody': @@ -415,21 +404,18 @@ export function getChildFormatContext( HTML_TABLE_BODY_MODE, null, parentContext.noscriptTagInScope, - itemScoped, ); case 'colgroup': return createFormatContext( HTML_COLGROUP_MODE, null, parentContext.noscriptTagInScope, - itemScoped, ); case 'tr': return createFormatContext( HTML_TABLE_ROW_MODE, null, parentContext.noscriptTagInScope, - itemScoped, ); } if (parentContext.insertionMode >= HTML_TABLE_MODE) { @@ -439,28 +425,19 @@ export function getChildFormatContext( HTML_MODE, null, parentContext.noscriptTagInScope, - itemScoped, ); } if (parentContext.insertionMode === ROOT_HTML_MODE) { if (type === 'html') { // We've emitted the root and is now in mode. - return createFormatContext(HTML_HTML_MODE, null, false, itemScoped); + return createFormatContext(HTML_HTML_MODE, null, false); } else { // We've emitted the root and is now in plain HTML mode. - return createFormatContext(HTML_MODE, null, false, itemScoped); + return createFormatContext(HTML_MODE, null, false); } } else if (parentContext.insertionMode === HTML_HTML_MODE) { // We've emitted the document element and is now in plain HTML mode. - return createFormatContext(HTML_MODE, null, false, itemScoped); - } - if (itemScoped !== parentContext.itemInScope) { - return createFormatContext( - parentContext.insertionMode, - parentContext.selectedValue, - parentContext.noscriptTagInScope, - itemScoped, - ); + return createFormatContext(HTML_MODE, null, false); } return parentContext; } @@ -1278,13 +1255,12 @@ function pushMeta( textEmbedded: boolean, insertionMode: InsertionMode, noscriptTagInScope: boolean, - itemInScope: boolean, ): null { if (enableFloat) { if ( insertionMode === SVG_MODE || noscriptTagInScope || - (itemInScope && props.itemProp != null) + props.itemProp != null ) { return pushSelfClosing(target, props, 'meta'); } else { @@ -1313,7 +1289,6 @@ function pushLink( textEmbedded: boolean, insertionMode: InsertionMode, noscriptTagInScope: boolean, - itemInScope: boolean, ): null { if (enableFloat) { const rel = props.rel; @@ -1322,7 +1297,7 @@ function pushLink( if ( insertionMode === SVG_MODE || noscriptTagInScope || - (itemInScope && props.itemProp != null) || + props.itemProp != null || typeof rel !== 'string' || typeof href !== 'string' || href === '' @@ -1566,7 +1541,6 @@ function pushStyle( textEmbedded: boolean, insertionMode: InsertionMode, noscriptTagInScope: boolean, - itemInScope: boolean, ): ReactNodeList { if (__DEV__) { if (hasOwnProperty.call(props, 'children')) { @@ -1604,7 +1578,7 @@ function pushStyle( if ( insertionMode === SVG_MODE || noscriptTagInScope || - (itemInScope && props.itemProp != null) || + props.itemProp != null || typeof precedence !== 'string' || typeof href !== 'string' || href === '' @@ -1825,7 +1799,6 @@ function pushTitle( responseState: ResponseState, insertionMode: InsertionMode, noscriptTagInScope: boolean, - itemInScope: boolean, ): ReactNodeList { if (__DEV__) { if (hasOwnProperty.call(props, 'children')) { @@ -1879,7 +1852,7 @@ function pushTitle( if ( insertionMode !== SVG_MODE && !noscriptTagInScope && - (!itemInScope || props.itemProp == null) + props.itemProp == null ) { pushTitleImpl(responseState.hoistableChunks, props); return null; @@ -2066,13 +2039,12 @@ function pushScript( textEmbedded: boolean, insertionMode: InsertionMode, noscriptTagInScope: boolean, - itemInScope: boolean, ): null { if (enableFloat) { if ( insertionMode === SVG_MODE || noscriptTagInScope || - (itemInScope && props.itemProp != null) || + props.itemProp != null || typeof props.src !== 'string' || !props.src ) { @@ -2531,7 +2503,6 @@ export function pushStartInstance( responseState, formatContext.insertionMode, formatContext.noscriptTagInScope, - formatContext.itemInScope, ) : pushStartTitle(target, props); case 'link': @@ -2543,7 +2514,6 @@ export function pushStartInstance( textEmbedded, formatContext.insertionMode, formatContext.noscriptTagInScope, - formatContext.itemInScope, ); case 'script': return enableFloat @@ -2554,7 +2524,6 @@ export function pushStartInstance( textEmbedded, formatContext.insertionMode, formatContext.noscriptTagInScope, - formatContext.itemInScope, ) : pushStartGenericElement(target, props, type); case 'style': @@ -2565,7 +2534,6 @@ export function pushStartInstance( textEmbedded, formatContext.insertionMode, formatContext.noscriptTagInScope, - formatContext.itemInScope, ); case 'meta': return pushMeta( @@ -2575,7 +2543,6 @@ export function pushStartInstance( textEmbedded, formatContext.insertionMode, formatContext.noscriptTagInScope, - formatContext.itemInScope, ); // Newline eating tags case 'listing': diff --git a/packages/react-dom-bindings/src/server/ReactDOMServerLegacyFormatConfig.js b/packages/react-dom-bindings/src/server/ReactDOMServerLegacyFormatConfig.js index 14b0d09e1c233..5ca51fbcbbac0 100644 --- a/packages/react-dom-bindings/src/server/ReactDOMServerLegacyFormatConfig.js +++ b/packages/react-dom-bindings/src/server/ReactDOMServerLegacyFormatConfig.js @@ -101,7 +101,6 @@ export function createRootFormatContext(): FormatContext { insertionMode: HTML_MODE, // We skip the root mode because we don't want to emit the DOCTYPE in legacy mode. selectedValue: null, noscriptTagInScope: false, - itemInScope: false, }; } diff --git a/packages/react-dom/src/__tests__/ReactDOMFloat-test.js b/packages/react-dom/src/__tests__/ReactDOMFloat-test.js index 10721ef598cdb..b53864bbfab4d 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFloat-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFloat-test.js @@ -2293,23 +2293,25 @@ body { ); }); - it('does not hoist anything with an itemprop prop that is inside an itemscope', async () => { - await actIntoEmptyDocument(() => { - renderToPipeableStream( + it('does not hoist anything with an itemprop prop', async () => { + function App() { + return ( - - - title - - -