Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Float] handle resource Resource creation inside svg context #25599

Merged
merged 3 commits into from
Nov 1, 2022
Merged

[Float] handle resource Resource creation inside svg context #25599

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f83294b
do not make resources inside svg context
gnoff Nov 1, 2022
64f470a
only restrict title in svg. add warnings for almost-resoures in svg
gnoff Nov 1, 2022
6e949de
assert that foreign object allows resources
gnoff Nov 1, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion packages/react-dom-bindings/src/client/ReactDOMFloatClient.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ import {
getHostContext,
} from 'react-reconciler/src/ReactFiberHostContext';
import {getResourceFormOnly} from './validateDOMNesting';
import {getNamespace} from './ReactDOMHostConfig';
import {SVG_NAMESPACE} from '../shared/DOMNamespaces';

// The resource types we support. currently they match the form for the as argument.
// In the future this may need to change, especially when modules / scripts are supported
Expand Down Expand Up @@ -1397,10 +1399,14 @@ function insertResourceInstanceBefore(

export function isHostResourceType(type: string, props: Props): boolean {
let resourceFormOnly: boolean;
const hostContext = getHostContext();
const namespace = getNamespace(hostContext);
if (__DEV__) {
const hostContext = getHostContext();
resourceFormOnly = getResourceFormOnly(hostContext);
}
if (namespace === SVG_NAMESPACE) {
return false;
}
switch (type) {
case 'base':
case 'meta':
Expand Down
10 changes: 10 additions & 0 deletions packages/react-dom-bindings/src/client/ReactDOMHostConfig.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,16 @@ export function getPublicInstance(instance: Instance): Instance {
return instance;
}

export function getNamespace(hostContext: HostContext): string {
if (__DEV__) {
const hostContextDev: HostContextDev = (hostContext: any);
return hostContextDev.namespace;
} else {
const hostContextProd: HostContextProd = (hostContext: any);
return hostContextProd;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the other helpers that call out to external things in ReactDOMComponent read the namespace first and then pass it in as an argument to them.

I think we need to rip out getHostContext call and put those in the reconciler and then call the host config with the host context like we do for createInstance and others.

It's too fragile. I don't actually know if it's even correct to call it in the child reconciler. I think that might actually be wrong in some cases because I'm not sure that it's guaranteed to have the right context. Regardless, that code in the reconciler is what needs to provide that guarantee.

Once you pass it in to isHostResourceType, then you can get the namespace in isHostResourceType and pass it into ReactDOMFloatClient. It's free in prod since it's just the same value and hopefully the whole ReactDOMFloatClient gets inlined.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is unforutnately buried pretty deep in the reconciliation pass. I'd have to thread it through reconcileChildren. I can do this but before I go and make that happen I want to make sure that's actually viable.

An alternative is we let the reconciliation create HostComponents always and then we "upgrade" them to Resource when we beginWork and currrent === null similar to how Memo's can become SimpleMemos

Copy link
Collaborator

@sebmarkbage sebmarkbage Nov 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can start by just calling it right before calling isHostResourceType. At least then it's in the reconciler code instead of the bindings. It's a pretty cheap call too that could possibly even be reorganized by the compiler.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or it could be cheap #25609

}
}

gnoff marked this conversation as resolved.
Show resolved Hide resolved
export function prepareForCommit(containerInfo: Container): Object | null {
eventsEnabled = ReactBrowserEventEmitterIsEnabled();
selectionInformation = getSelectionInformation();
Expand Down
27 changes: 25 additions & 2 deletions packages/react-dom-bindings/src/server/ReactDOMServerFormatConfig.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1197,10 +1197,12 @@ function pushBase(
props: Object,
responseState: ResponseState,
textEmbedded: boolean,
insertionMode: InsertionMode,
noscriptTagInScope: boolean,
): ReactNodeList {
if (
enableFloat &&
insertionMode !== SVG_MODE &&
!noscriptTagInScope &&
resourcesFromElement('base', props)
) {
Expand All @@ -1222,10 +1224,12 @@ function pushMeta(
props: Object,
responseState: ResponseState,
textEmbedded: boolean,
insertionMode: InsertionMode,
noscriptTagInScope: boolean,
): ReactNodeList {
if (
enableFloat &&
insertionMode !== SVG_MODE &&
!noscriptTagInScope &&
resourcesFromElement('meta', props)
) {
Expand All @@ -1247,9 +1251,15 @@ function pushLink(
props: Object,
responseState: ResponseState,
textEmbedded: boolean,
insertionMode: InsertionMode,
noscriptTagInScope: boolean,
): ReactNodeList {
if (enableFloat && !noscriptTagInScope && resourcesFromLink(props)) {
if (
enableFloat &&
insertionMode !== SVG_MODE &&
!noscriptTagInScope &&
resourcesFromLink(props)
) {
if (textEmbedded) {
// This link follows text but we aren't writing a tag. while not as efficient as possible we need
// to be safe and assume text will follow by inserting a textSeparator
Expand Down Expand Up @@ -1371,6 +1381,7 @@ function pushTitle(
target: Array<Chunk | PrecomputedChunk>,
props: Object,
responseState: ResponseState,
insertionMode: InsertionMode,
noscriptTagInScope: boolean,
): ReactNodeList {
if (__DEV__) {
Expand Down Expand Up @@ -1415,6 +1426,7 @@ function pushTitle(

if (
enableFloat &&
insertionMode !== SVG_MODE &&
!noscriptTagInScope &&
resourcesFromElement('title', props)
) {
Expand Down Expand Up @@ -1578,9 +1590,15 @@ function pushScript(
props: Object,
responseState: ResponseState,
textEmbedded: boolean,
insertionMode: InsertionMode,
noscriptTagInScope: boolean,
): null {
if (enableFloat && !noscriptTagInScope && resourcesFromScript(props)) {
if (
enableFloat &&
insertionMode !== SVG_MODE &&
gnoff marked this conversation as resolved.
Show resolved Hide resolved
!noscriptTagInScope &&
resourcesFromScript(props)
) {
if (textEmbedded) {
// This link follows text but we aren't writing a tag. while not as efficient as possible we need
// to be safe and assume text will follow by inserting a textSeparator
Expand Down Expand Up @@ -1926,6 +1944,7 @@ export function pushStartInstance(
target,
props,
responseState,
formatContext.insertionMode,
formatContext.noscriptTagInScope,
)
: pushStartTitle(target, props, responseState);
Expand All @@ -1935,6 +1954,7 @@ export function pushStartInstance(
props,
responseState,
textEmbedded,
formatContext.insertionMode,
formatContext.noscriptTagInScope,
);
case 'script':
Expand All @@ -1944,6 +1964,7 @@ export function pushStartInstance(
props,
responseState,
textEmbedded,
formatContext.insertionMode,
formatContext.noscriptTagInScope,
)
: pushStartGenericElement(target, props, type, responseState);
Expand All @@ -1953,6 +1974,7 @@ export function pushStartInstance(
props,
responseState,
textEmbedded,
formatContext.insertionMode,
formatContext.noscriptTagInScope,
);
case 'base':
Expand All @@ -1961,6 +1983,7 @@ export function pushStartInstance(
props,
responseState,
textEmbedded,
formatContext.insertionMode,
formatContext.noscriptTagInScope,
);
// Newline eating tags
Expand Down
93 changes: 92 additions & 1 deletion packages/react-dom/src/__tests__/ReactDOMFloat-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5385,7 +5385,98 @@ describe('ReactDOMFloat', () => {
});
});

describe('noscript', () => {
describe('resource free contexts', () => {
// @gate enableFloat
it('should not turn descendants of svg into resources', async () => {
await actIntoEmptyDocument(() => {
const {pipe} = ReactDOMFizzServer.renderToPipeableStream(
<html>
<body>
<svg>
<title>foo</title>
<path>
<title>bar</title>
</path>
</svg>
</body>
</html>,
);
pipe(writable);
});
expect(getMeaningfulChildren(document)).toEqual(
<html>
<head />
<body>
<svg>
<title>foo</title>
<path>
<title>bar</title>
</path>
</svg>
</body>
</html>,
);

let root = ReactDOMClient.hydrateRoot(
document,
<html>
<head />
<body>
<svg>
<title>foo</title>
<path>
<title>bar</title>
</path>
</svg>
</body>
</html>,
);
expect(Scheduler).toFlushWithoutYielding();
expect(getMeaningfulChildren(document)).toEqual(
<html>
<head />
<body>
<svg>
<title>foo</title>
<path>
<title>bar</title>
</path>
</svg>
</body>
</html>,
);

root.unmount();
root = ReactDOMClient.createRoot(document);
root.render(
<html>
<head />
<body>
<svg>
<title>foo</title>
<path>
<title>bar</title>
</path>
</svg>
</body>
</html>,
);
expect(Scheduler).toFlushWithoutYielding();
expect(getMeaningfulChildren(document)).toEqual(
<html>
<head />
<body>
<svg>
<title>foo</title>
<path>
<title>bar</title>
</path>
</svg>
</body>
</html>,
);
});

// @gate enableFloat
it('should not turn children of noscript into resources', async () => {
function SomeResources() {
Expand Down