From 8a9f82ed58c2fa76583a041fa34ad80f5f94a3d1 Mon Sep 17 00:00:00 2001 From: Josh Story Date: Mon, 6 Mar 2023 15:00:54 -0800 Subject: [PATCH] [Float][Fizz][Fiber] - Do not hoist elements with `itemProp` & hydrate more tolerantly in hoist contexts (#26256) ## Do not hoist elements with `itemProp` In HTML `itemprop` signifies a property of an `itemscope` with respect to the Microdata spec (https://html.spec.whatwg.org/multipage/microdata.html#microdata) additionally `itemprop` is valid on any tag and can even make some tags that are otherwise invalid in the `` valid there (`` for instance). Originally I tried an approach where if you rendered something otherwise hoistable inside an `itemscope` it would not hoist if it had an `itemprop`. This meant that some components with `itemprop` could hoist (if they were not scoped, which is generally invalid microdata implementation). However the problem is things that do hoist, hoist into the head and body and these tags can have an `itemscope`. This creates a ton of ambiguity when trying to hydrate in these hoist scopes because we can't know for certain whether a DOM node we find there was hoisted or not even if it has an `itemprop` attribute. There are other scenarios too that have abiguous semantics like rendering a hoistable with `itemProp` outside of ``. Is it fair to embed that hoistable inside that itemScope even though it was defined outside? To simplify the situation and disambiguate I dropped the `itemscope` portion from the implementation and now any host component that could normally be hoisted will not hoist if it has an `itemProp` prop. In addition to the changes made for `itemProp` this PR also modifies part of the hydration implementation to be more tolerant of tags injected by 3rd parties. This was opportunistically done when we needed to have context information like `inItemScope` but with the most recent implementation that has been removed. I have however left the hydration changes in place as it is a goal to make React handle hydrating the entire Document even when we cannot control whether 3rd parties are going to inject tags that React will not render but are also not hoistables ------- ##### Original Description when we considered tracking itemScope >One recent decision was to make elements using the `itemProp` prop not hoistable if they were inside and itemScope. This better fits with Microdata spec which allows for meta tags and other tag types usually reserved for the `` to be used in the `` when using itemScope. > >To implement this a number of small changes were necessary > >1. HostContext in prod needed to expand beyond just tracking the element namespace for new element creation. It now tracks whether we are in an itemScope. To keep this efficient it is modeled as a bitmask. >2. To disambiguate what is and is not a potential instance in the DOM for hoistables the hydration algo was updated to skip past non-matching instances while attempting to claim the instance rather than ahead of time (getNextHydratable). >3. React will not consider an itemScope on ``, ``, or `` as a valid scope for the hoisting opt-out. This is important as an invariant so we can make assumptions about certain tags in these scopes. This should not be a functional breaking change because if any of these tags have an `itemScope` then it can just be moved into the first node inside the `` > >Since we were already updating the logic for hydration to better support `itemScope` opt-out I also changed the hydration behavior for suspected 3rd party nodes in `` and ``. Now if you are hydrating in either of those contexts hydration will skip past any non-matching nodes until it finds a match. This allows 3rd party scripts and extensions to inject nodes in either context that React does not expect and still avoid a hydration mismatch. > >This new algorithm isn't perfect and it is possible for a mismatch to occur. The most glaring case may be if a 3rd party script prepends a `
` into `` and you render a `
` in `` in your app. there is nothing to signal to React that this div was 3rd party so it will claim is as the hydrated instance and hydration will almost certainly fail immediately afterwards. > >The expectation is that this is rare and that if falling back to client rendering is transparent to the user then there is not problem here. We will continue to evaluate this and may change the hydration matching algorithm further to match user and developer expectations --- .../src/client/ReactDOMComponent.js | 183 ++++----- .../src/client/ReactDOMFloatClient.js | 77 ++-- .../src/client/ReactDOMHostConfig.js | 366 ++++++++++-------- .../src/server/ReactDOMServerFormatConfig.js | 15 +- .../src/shared/DOMNamespaces.js | 2 +- .../src/__tests__/ReactDOMFloat-test.js | 337 +++++++++++++++- ...DOMServerPartialHydration-test.internal.js | 17 +- .../src/ReactFiberBeginWork.js | 6 +- .../ReactFiberHostConfigWithNoHydration.js | 12 +- .../src/ReactFiberHostContext.js | 2 +- .../src/ReactFiberHydrationContext.js | 347 +++++++++++++---- .../src/forks/ReactFiberHostConfig.custom.js | 17 +- 12 files changed, 983 insertions(+), 398 deletions(-) diff --git a/packages/react-dom-bindings/src/client/ReactDOMComponent.js b/packages/react-dom-bindings/src/client/ReactDOMComponent.js index 620dc314485e1..e736f2ffb53a8 100644 --- a/packages/react-dom-bindings/src/client/ReactDOMComponent.js +++ b/packages/react-dom-bindings/src/client/ReactDOMComponent.js @@ -55,7 +55,12 @@ import { setValueForStyles, validateShorthandPropertyCollisionInDev, } from './CSSPropertyOperations'; -import {HTML_NAMESPACE, getIntrinsicNamespace} from '../shared/DOMNamespaces'; +import { + HTML_NAMESPACE, + MATH_NAMESPACE, + SVG_NAMESPACE, + getIntrinsicNamespace, +} from '../shared/DOMNamespaces'; import { getPropertyInfo, shouldIgnoreAttribute, @@ -375,112 +380,112 @@ function updateDOMProperties( } } -export function createElement( +// Creates elements in the HTML namesapce +export function createHTMLElement( type: string, props: Object, - rootContainerElement: Element | Document | DocumentFragment, - parentNamespace: string, + ownerDocument: Document, ): Element { let isCustomComponentTag; - // We create tags in the namespace of their parent container, except HTML - // tags get no namespace. - const ownerDocument: Document = - getOwnerDocumentFromRootContainer(rootContainerElement); let domElement: Element; - let namespaceURI = parentNamespace; - if (namespaceURI === HTML_NAMESPACE) { - namespaceURI = getIntrinsicNamespace(type); + if (__DEV__) { + isCustomComponentTag = isCustomComponent(type, props); + // Should this check be gated by parent namespace? Not sure we want to + // allow or . + if (!isCustomComponentTag && type !== type.toLowerCase()) { + console.error( + '<%s /> is using incorrect casing. ' + + 'Use PascalCase for React components, ' + + 'or lowercase for HTML elements.', + type, + ); + } } - if (namespaceURI === HTML_NAMESPACE) { + + if (type === 'script') { + // Create the script via .innerHTML so its "parser-inserted" flag is + // set to true and it does not execute + const div = ownerDocument.createElement('div'); if (__DEV__) { - isCustomComponentTag = isCustomComponent(type, props); - // Should this check be gated by parent namespace? Not sure we want to - // allow or . - if (!isCustomComponentTag && type !== type.toLowerCase()) { + if (enableTrustedTypesIntegration && !didWarnScriptTags) { console.error( - '<%s /> is using incorrect casing. ' + - 'Use PascalCase for React components, ' + - 'or lowercase for HTML elements.', - type, + 'Encountered a script tag while rendering React component. ' + + 'Scripts inside React components are never executed when rendering ' + + 'on the client. Consider using template tag instead ' + + '(https://developer.mozilla.org/en-US/docs/Web/HTML/Element/template).', ); + didWarnScriptTags = true; } } - - if (type === 'script') { - // Create the script via .innerHTML so its "parser-inserted" flag is - // set to true and it does not execute - const div = ownerDocument.createElement('div'); - if (__DEV__) { - if (enableTrustedTypesIntegration && !didWarnScriptTags) { - console.error( - 'Encountered a script tag while rendering React component. ' + - 'Scripts inside React components are never executed when rendering ' + - 'on the client. Consider using template tag instead ' + - '(https://developer.mozilla.org/en-US/docs/Web/HTML/Element/template).', - ); - didWarnScriptTags = true; - } - } - div.innerHTML = ' + // but it seems reasonable and conservative to reject this as a hydration error as well + return false; + } else if ( + instance.nodeName.toLowerCase() !== type.toLowerCase() || + isMarkedResource(instance) + ) { + // We are either about to + return true; + } else { + // We have an Element with the right type. + const element: Element = (instance: any); + const anyProps = (props: any); + + // We are going to try to exclude it if we can definitely identify it as a hoisted Node or if + // we can guess that the node is likely hoisted or was inserted by a 3rd party script or browser extension + // 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) { + // 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': { + // The only way to opt out of hoisting meta tags is to give it an itemprop attribute. We assume there will be + // not 3rd party meta tags that are prepended, accepting the cases where this isn't true because meta tags + // are usually only functional for SSR so even in a rare case where we did bind to an injected tag the runtime + // implications are minimal + if (!element.hasAttribute('itemprop')) { + // This is a Hoistable + return true; + } + break; + } + case 'link': { + // Links come in many forms and we do expect 3rd parties to inject them into / . We exclude known resources + // and then use high-entroy attributes like href which are almost always used and almost always unique to filter out unlikely + // matches. + const rel = element.getAttribute('rel'); + if (rel === 'stylesheet' && element.hasAttribute('data-precedence')) { + // This is a stylesheet resource + return true; + } else if ( + 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) + ) { + // 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 + return true; + } + break; + } + case 'style': { + // Styles are hard to match correctly. We can exclude known resources but otherwise we accept the fact that a non-hoisted style tags + // in or are likely never going to be unmounted given their position in the document and the fact they likely hold global styles + if (element.hasAttribute('data-precedence')) { + // This is a style resource + return true; + } + break; + } + case 'script': { + // Scripts are a little tricky, we exclude known resources and then similar to links try to use high-entropy attributes + // to reject poor matches. One challenge with scripts are inline scripts. We don't attempt to check text content which could + // in theory lead to a hydration error later if a 3rd party injected an inline script before the React rendered nodes. + // Falling back to client rendering if this happens should be seemless though so we will try this hueristic and revisit later + // if we learn it is problematic + const srcAttr = element.getAttribute('src'); + if ( + srcAttr && + element.hasAttribute('async') && + !element.hasAttribute('itemprop') + ) { + // This is an async script resource + return true; + } else if ( + 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 + return true; + } + break; + } + } + // 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 false; + } +} + +export function shouldSkipHydratableForTextInstance( + instance: HydratableInstance, +): boolean { + return instance.nodeType === ELEMENT_NODE; +} + +export function shouldSkipHydratableForSuspenseInstance( + instance: HydratableInstance, +): boolean { + return instance.nodeType === ELEMENT_NODE; +} export function canHydrateInstance( instance: HydratableInstance, @@ -852,19 +989,21 @@ export function canHydrateInstance( ): null | Instance { if ( instance.nodeType !== ELEMENT_NODE || - type.toLowerCase() !== instance.nodeName.toLowerCase() + instance.nodeName.toLowerCase() !== type.toLowerCase() ) { return null; + } else { + return ((instance: any): Instance); } - // This has now been refined to an element node. - return ((instance: any): Instance); } export function canHydrateTextInstance( instance: HydratableInstance, text: string, ): null | TextInstance { - if (text === '' || instance.nodeType !== TEXT_NODE) { + if (text === '') return null; + + if (instance.nodeType !== TEXT_NODE) { // Empty strings are not parsed by HTML so there won't be a correct match here. return null; } @@ -876,7 +1015,6 @@ export function canHydrateSuspenseInstance( instance: HydratableInstance, ): null | SuspenseInstance { if (instance.nodeType !== COMMENT_NODE) { - // Empty strings are not parsed by HTML so there won't be a correct match here. return null; } // This has now been refined to a suspense node. @@ -931,114 +1069,8 @@ function getNextHydratable(node: ?Node) { // Skip non-hydratable nodes. for (; node != null; node = ((node: any): Node).nextSibling) { const nodeType = node.nodeType; - if (enableFloat && enableHostSingletons) { - if (nodeType === ELEMENT_NODE) { - const element: Element = (node: any); - switch (element.tagName) { - // This is subtle. in SVG scope the title tag is case sensitive. we don't want to skip - // titles in svg but we do want to skip them outside of svg. there is an edge case where - // you could do `React.createElement('TITLE', ...)` inside an svg scope but the SSR serializer - // will still emit lowercase. Practically speaking the only time the DOM will have a non-uppercased - // title tagName is if it is inside an svg. - // Other Resource types like META, BASE, LINK, and SCRIPT should be treated as resources even inside - // svg scope because they are invalid otherwise. We still don't need to handle the lowercase variant - // because if they are present in the DOM already they would have been hoisted outside the SVG scope - // as Resources. So while it would be correct to skip a inside and this algorithm won't - // skip that link because the tagName will not be uppercased it functionally is irrelevant. If one - // tries to render incompatible types such as a non-resource stylesheet inside an svg the server will - // emit that invalid html and hydration will fail. In Dev this will present warnings guiding the - // developer on how to fix. - case 'TITLE': - case 'META': - case 'HTML': - case 'HEAD': - case 'BODY': { - continue; - } - case 'LINK': { - const linkEl: HTMLLinkElement = (element: any); - // All links that are server rendered are resources except - // stylesheets that do not have a precedence - if ( - linkEl.rel === 'stylesheet' && - !linkEl.hasAttribute('data-precedence') - ) { - break; - } - continue; - } - case 'STYLE': { - const styleEl: HTMLStyleElement = (element: any); - if (styleEl.hasAttribute('data-precedence')) { - continue; - } - break; - } - case 'SCRIPT': { - const scriptEl: HTMLScriptElement = (element: any); - if (scriptEl.hasAttribute('async')) { - continue; - } - break; - } - } - break; - } else if (nodeType === TEXT_NODE) { - break; - } - } else if (enableFloat) { - if (nodeType === ELEMENT_NODE) { - const element: Element = (node: any); - switch (element.tagName) { - case 'TITLE': - case 'META': { - continue; - } - case 'LINK': { - const linkEl: HTMLLinkElement = (element: any); - // All links that are server rendered are resources except - // stylesheets that do not have a precedence - if ( - linkEl.rel === 'stylesheet' && - !linkEl.hasAttribute('data-precedence') - ) { - break; - } - continue; - } - case 'STYLE': { - const styleEl: HTMLStyleElement = (element: any); - if (styleEl.hasAttribute('data-precedence')) { - continue; - } - break; - } - case 'SCRIPT': { - const scriptEl: HTMLScriptElement = (element: any); - if (scriptEl.hasAttribute('async')) { - continue; - } - break; - } - } - break; - } else if (nodeType === TEXT_NODE) { - break; - } - } else if (enableHostSingletons) { - if (nodeType === ELEMENT_NODE) { - const tag: string = (node: any).tagName; - if (tag === 'HTML' || tag === 'HEAD' || tag === 'BODY') { - continue; - } - break; - } else if (nodeType === TEXT_NODE) { - break; - } - } else { - if (nodeType === ELEMENT_NODE || nodeType === TEXT_NODE) { - break; - } + if (nodeType === ELEMENT_NODE || nodeType === TEXT_NODE) { + break; } if (nodeType === COMMENT_NODE) { const nodeData = (node: any).data; @@ -1093,26 +1125,28 @@ export function hydrateInstance( // TODO: Possibly defer this until the commit phase where all the events // get attached. updateFiberProps(instance, props); - let parentNamespace: string; - if (__DEV__) { - const hostContextDev = ((hostContext: any): HostContextDev); - parentNamespace = hostContextDev.namespace; - } else { - parentNamespace = ((hostContext: any): HostContextProd); - } // TODO: Temporary hack to check if we're in a concurrent root. We can delete // when the legacy root API is removed. const isConcurrentMode = ((internalInstanceHandle: Fiber).mode & ConcurrentMode) !== NoMode; + let parentNamespace; + if (__DEV__) { + const hostContextDev = ((hostContext: any): HostContextDev); + parentNamespace = hostContextDev.namespace; + } else { + const hostContextProd = ((hostContext: any): HostContextProd); + parentNamespace = hostContextProd; + } + return diffHydratedProperties( instance, type, props, - parentNamespace, isConcurrentMode, shouldWarnDev, + parentNamespace, ); } @@ -1584,7 +1618,7 @@ export function isHostHoistableType( hostContext: HostContext, ): boolean { let outsideHostContainerContext: boolean; - let namespace: string; + let namespace: HostContextProd; if (__DEV__) { const hostContextDev: HostContextDev = (hostContext: any); // We can only render resources when we are not within the host container context @@ -1595,17 +1629,41 @@ export function isHostHoistableType( const hostContextProd: HostContextProd = (hostContext: any); namespace = hostContextProd; } + + // Global opt out of hoisting for anything in SVG Namespace or anything with an itemProp inside an itemScope + if (namespace === SVG_NAMESPACE || props.itemProp != null) { + if (__DEV__) { + if ( + outsideHostContainerContext && + props.itemProp != null && + (type === 'meta' || + type === 'title' || + type === 'style' || + type === 'link' || + type === 'script') + ) { + console.error( + 'Cannot render a <%s> outside the main document if it has an `itemProp` prop. `itemProp` suggests the tag belongs to an' + + ' `itemScope` which can appear anywhere in the DOM. If you were intending for React to hoist this <%s> remove the `itemProp` prop.' + + ' Otherwise, try moving this tag into the or of the Document.', + type, + type, + ); + } + } + return false; + } + switch (type) { case 'meta': case 'title': { - return namespace !== SVG_NAMESPACE; + return true; } case 'style': { if ( typeof props.precedence !== 'string' || typeof props.href !== 'string' || - props.href === '' || - namespace === SVG_NAMESPACE + props.href === '' ) { if (__DEV__) { if (outsideHostContainerContext) { @@ -1629,8 +1687,7 @@ export function isHostHoistableType( typeof props.href !== 'string' || props.href === '' || props.onLoad || - props.onError || - namespace === SVG_NAMESPACE + props.onError ) { if (__DEV__) { if ( @@ -1686,8 +1743,7 @@ export function isHostHoistableType( props.onLoad || props.onError || typeof props.src !== 'string' || - !props.src || - namespace === SVG_NAMESPACE + !props.src ) { if (__DEV__) { if (outsideHostContainerContext) { @@ -1771,8 +1827,8 @@ export function resolveSingletonInstance( validateDOMNestingDev: boolean, ): Instance { if (__DEV__) { + const hostContextDev = ((hostContext: any): HostContextDev); if (validateDOMNestingDev) { - const hostContextDev = ((hostContext: any): HostContextDev); validateDOMNesting(type, null, hostContextDev.ancestorInfo); } } diff --git a/packages/react-dom-bindings/src/server/ReactDOMServerFormatConfig.js b/packages/react-dom-bindings/src/server/ReactDOMServerFormatConfig.js index f3a234305b4df..77a7067e5d413 100644 --- a/packages/react-dom-bindings/src/server/ReactDOMServerFormatConfig.js +++ b/packages/react-dom-bindings/src/server/ReactDOMServerFormatConfig.js @@ -1257,7 +1257,11 @@ function pushMeta( noscriptTagInScope: boolean, ): null { if (enableFloat) { - if (insertionMode === SVG_MODE || noscriptTagInScope) { + if ( + insertionMode === SVG_MODE || + noscriptTagInScope || + props.itemProp != null + ) { return pushSelfClosing(target, props, 'meta'); } else { if (textEmbedded) { @@ -1293,6 +1297,7 @@ function pushLink( if ( insertionMode === SVG_MODE || noscriptTagInScope || + props.itemProp != null || typeof rel !== 'string' || typeof href !== 'string' || href === '' @@ -1573,6 +1578,7 @@ function pushStyle( if ( insertionMode === SVG_MODE || noscriptTagInScope || + props.itemProp != null || typeof precedence !== 'string' || typeof href !== 'string' || href === '' @@ -1843,7 +1849,11 @@ function pushTitle( } if (enableFloat) { - if (insertionMode !== SVG_MODE && !noscriptTagInScope) { + if ( + insertionMode !== SVG_MODE && + !noscriptTagInScope && + props.itemProp == null + ) { pushTitleImpl(responseState.hoistableChunks, props); return null; } else { @@ -2034,6 +2044,7 @@ function pushScript( if ( insertionMode === SVG_MODE || noscriptTagInScope || + props.itemProp != null || typeof props.src !== 'string' || !props.src ) { diff --git a/packages/react-dom-bindings/src/shared/DOMNamespaces.js b/packages/react-dom-bindings/src/shared/DOMNamespaces.js index 43fdf00865155..6c6c887ffafa5 100644 --- a/packages/react-dom-bindings/src/shared/DOMNamespaces.js +++ b/packages/react-dom-bindings/src/shared/DOMNamespaces.js @@ -4,7 +4,7 @@ * This source code is licensed under the MIT license found in the * LICENSE file in the root directory of this source tree. * - * @noflow + * @flow */ export const HTML_NAMESPACE = 'http://www.w3.org/1999/xhtml'; diff --git a/packages/react-dom/src/__tests__/ReactDOMFloat-test.js b/packages/react-dom/src/__tests__/ReactDOMFloat-test.js index ca5222a9b56b4..0d540b9541ffc 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFloat-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFloat-test.js @@ -369,7 +369,6 @@ describe('ReactDOMFloat', () => { foo -