Skip to content

Commit

Permalink
[Float][Fizz][Fiber] - Do not hoist elements with itemProp & hydrat…
Browse files Browse the repository at this point in the history 
…e 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 `<body>` valid there (`<meta>` 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 `<html itemScope={true>`. 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 `<head>` to be used in the `<body>` 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 `<html>`, `<head>`, or
`<body>` 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 `<body>`
>
>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 `<head>` and `<body>`. 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
`<div>` into `<body>` and you render a `<div>` in `<body>` 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
  • Loading branch information
@gnoff
gnoff authored Mar 6, 2023
1 parent 3cad3a5 commit 8a9f82e
Show file tree
Hide file tree
Showing 12 changed files with 983 additions and 398 deletions.
183 changes: 94 additions & 89 deletions packages/react-dom-bindings/src/client/ReactDOMComponent.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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 <SVG> or <mATH>.
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 <SVG> or <mATH>.
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 = '<script><' + '/script>'; // eslint-disable-line
// This is guaranteed to yield a script element.
const firstChild = ((div.firstChild: any): HTMLScriptElement);
domElement = div.removeChild(firstChild);
} else if (typeof props.is === 'string') {
domElement = ownerDocument.createElement(type, {is: props.is});
} else {
// Separate else branch instead of using `props.is || undefined` above because of a Firefox bug.
// See discussion in https://github.com/facebook/react/pull/6896
// and discussion in https://bugzilla.mozilla.org/show_bug.cgi?id=1276240
domElement = ownerDocument.createElement(type);
// Normally attributes are assigned in `setInitialDOMProperties`, however the `multiple` and `size`
// attributes on `select`s needs to be added before `option`s are inserted.
// This prevents:
// - a bug where the `select` does not scroll to the correct option because singular
// `select` elements automatically pick the first item #13222
// - a bug where the `select` set the first item as selected despite the `size` attribute #14239
// See https://github.com/facebook/react/issues/13222
// and https://github.com/facebook/react/issues/14239
if (type === 'select') {
const node = ((domElement: any): HTMLSelectElement);
if (props.multiple) {
node.multiple = true;
} else if (props.size) {
// Setting a size greater than 1 causes a select to behave like `multiple=true`, where
// it is possible that no option is selected.
//
// This is only necessary when a select in "single selection mode".
node.size = props.size;
}
div.innerHTML = '<script><' + '/script>'; // eslint-disable-line
// This is guaranteed to yield a script element.
const firstChild = ((div.firstChild: any): HTMLScriptElement);
domElement = div.removeChild(firstChild);
} else if (typeof props.is === 'string') {
domElement = ownerDocument.createElement(type, {is: props.is});
} else {
// Separate else branch instead of using `props.is || undefined` above because of a Firefox bug.
// See discussion in https://github.com/facebook/react/pull/6896
// and discussion in https://bugzilla.mozilla.org/show_bug.cgi?id=1276240
domElement = ownerDocument.createElement(type);
// Normally attributes are assigned in `setInitialDOMProperties`, however the `multiple` and `size`
// attributes on `select`s needs to be added before `option`s are inserted.
// This prevents:
// - a bug where the `select` does not scroll to the correct option because singular
// `select` elements automatically pick the first item #13222
// - a bug where the `select` set the first item as selected despite the `size` attribute #14239
// See https://github.com/facebook/react/issues/13222
// and https://github.com/facebook/react/issues/14239
if (type === 'select') {
const node = ((domElement: any): HTMLSelectElement);
if (props.multiple) {
node.multiple = true;
} else if (props.size) {
// Setting a size greater than 1 causes a select to behave like `multiple=true`, where
// it is possible that no option is selected.
//
// This is only necessary when a select in "single selection mode".
node.size = props.size;
}
}
} else {
domElement = ownerDocument.createElementNS(namespaceURI, type);
}

if (__DEV__) {
if (namespaceURI === HTML_NAMESPACE) {
if (
!isCustomComponentTag &&
// $FlowFixMe[method-unbinding]
Object.prototype.toString.call(domElement) ===
'[object HTMLUnknownElement]' &&
!hasOwnProperty.call(warnedUnknownTags, type)
) {
warnedUnknownTags[type] = true;
console.error(
'The tag <%s> is unrecognized in this browser. ' +
'If you meant to render a React component, start its name with ' +
'an uppercase letter.',
type,
);
}
if (
!isCustomComponentTag &&
// $FlowFixMe[method-unbinding]
Object.prototype.toString.call(domElement) ===
'[object HTMLUnknownElement]' &&
!hasOwnProperty.call(warnedUnknownTags, type)
) {
warnedUnknownTags[type] = true;
console.error(
'The tag <%s> is unrecognized in this browser. ' +
'If you meant to render a React component, start its name with ' +
'an uppercase letter.',
type,
);
}
}

return domElement;
}

export function createSVGElement(
type: string,
ownerDocument: Document,
): Element {
return ownerDocument.createElementNS(SVG_NAMESPACE, type);
}

export function createMathElement(
type: string,
ownerDocument: Document,
): Element {
return ownerDocument.createElementNS(MATH_NAMESPACE, type);
}

export function createTextNode(
text: string,
rootContainerElement: Element | Document | DocumentFragment,
Expand Down Expand Up @@ -864,9 +869,9 @@ export function diffHydratedProperties(
domElement: Element,
tag: string,
rawProps: Object,
parentNamespace: string,
isConcurrentMode: boolean,
shouldWarnDev: boolean,
parentNamespaceDev: string,
): null | Array<mixed> {
let isCustomComponentTag;
let extraAttributeNames: Set<string>;
Expand Down Expand Up @@ -1109,11 +1114,11 @@ export function diffHydratedProperties(
propertyInfo,
);
} else {
let ownNamespace = parentNamespace;
if (ownNamespace === HTML_NAMESPACE) {
ownNamespace = getIntrinsicNamespace(tag);
let ownNamespaceDev = parentNamespaceDev;
if (ownNamespaceDev === HTML_NAMESPACE) {
ownNamespaceDev = getIntrinsicNamespace(tag);
}
if (ownNamespace === HTML_NAMESPACE) {
if (ownNamespaceDev === HTML_NAMESPACE) {
// $FlowFixMe - Should be inferred as not undefined.
extraAttributeNames.delete(propKey.toLowerCase());
} else {
Expand Down
Loading

0 comments on commit 8a9f82e

Please sign in to comment.