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.

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

[do not merge]Retain documentElement when appending/removing from Document #24524

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
76 changes: 76 additions & 0 deletions packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js
Expand Up @@ -576,6 +576,82 @@ describe('ReactDOMFizzServer', () => {
expect(loggedErrors).toEqual([theError]);
});

// @gate experimental
it('#22833 should not error when client rendering a fallback at the document root (html element)', async () => {
function App({isClient}) {
return (
<html>
<head>
<title>an introduction</title>
</head>
<body>
<p>hi</p>
<p>{isClient ? 'hello client' : 'hello server'}</p>
</body>
</html>
);
}

const {pipe} = ReactDOMFizzServer.renderToPipeableStream(
<App isClient={false} />,
);
pipe(writable);
await new Promise(resolve => {
setImmediate(resolve);
});

// Test Environment
const jsdom = new JSDOM(buffer, {
runScripts: 'dangerously',
});
buffer = '';
window = jsdom.window;
document = jsdom.window.document;
container = document;

// Attempt to hydrate the content.
ReactDOMClient.hydrateRoot(document, <App isClient={true} />, {
onRecoverableError(error) {
Scheduler.unstable_yieldValue(error.message);
},
});

const assertion = () => {
expect(Scheduler).toFlushAndYield([
'Text content does not match server-rendered HTML.',
'There was an error while hydrating. Because the error happened outside of a Suspense boundary, the entire root will switch to client rendering.',
]);
};

if (__DEV__) {
expect(assertion).toErrorDev(
[
'Warning: Text content did not match. Server: "hello server" Client: "hello client"\n' +
' in p (at **)\n' +
' in body (at **)\n' +
' in html (at **)',
'Warning: An error occurred during hydration. The server HTML was replaced with client content in <#document>.',
],
{withoutStack: 1},
);
} else {
assertion();
}

// We're still loading because we're waiting for the server to stream more content.
expect(getVisibleChildren(container)).toEqual(
<html>
<head>
<title>an introduction</title>
</head>
<body>
<p>hi</p>
<p>hello client</p>
</body>
</html>,
);
});

// @gate experimental
it('should asynchronously load the suspense boundary', async () => {
await act(async () => {
Expand Down
5 changes: 4 additions & 1 deletion packages/react-dom/src/__tests__/ReactRenderDocument-test.js
Expand Up @@ -62,7 +62,10 @@ describe('rendering React components at document', () => {
expect(body === testDocument.body).toBe(true);
});

it('should not be able to unmount component from document node', () => {
// @TODO This test should now fail since we leave the documentElement in place even when we unmount
// from a Document container. It probably just amkes senese to reframe this test to reflect that you
// are left with an empty html element rather than no children at all
xit('should not be able to unmount component from document node', () => {
class Root extends React.Component {
render() {
return (
Expand Down
36 changes: 36 additions & 0 deletions packages/react-dom/src/client/ReactDOMComponentTree.js
Expand Up @@ -153,6 +153,42 @@ export function getClosestInstanceFromNode(targetNode: Node): null | Fiber {
return null;
}

/**
* Given an original DOM node attached to a Fiber, replace it with the
* replacement DOM node, detatching the original in the process
*
* This function throws if the original node is not attached to a Fiber
*/
export function replaceNode(original: Node, replacement: Node): void {
const fiber = original[internalInstanceKey];
if (fiber == null) {
throw new Error(
'replaceNode expected the original DOM node to have a fiber reference but one was not found. this is a bug in React, please file an issue.',
);
}
fiber.stateNode = replacement;
replacement[internalInstanceKey] = original[internalInstanceKey];
delete original[internalInstanceKey];
if (internalPropsKey in original) {
replacement[internalPropsKey] = original[internalPropsKey];
delete original[internalPropsKey];
}
if (internalEventHandlersKey in original) {
replacement[internalEventHandlersKey] = original[internalEventHandlersKey];
delete original[internalEventHandlersKey];
}
if (internalEventHandlerListenersKey in original) {
replacement[internalEventHandlerListenersKey] =
original[internalEventHandlerListenersKey];
delete original[internalEventHandlerListenersKey];
}
if (internalEventHandlesSetKey in original) {
replacement[internalEventHandlesSetKey] =
original[internalEventHandlesSetKey];
delete original[internalEventHandlesSetKey];
}
}

/**
* Given a DOM node, return the ReactDOMComponent or ReactDOMTextComponent
* instance, or null if the node was not rendered by this React.
Expand Down
93 changes: 61 additions & 32 deletions packages/react-dom/src/client/ReactDOMHostConfig.js
Expand Up @@ -23,8 +23,10 @@ import {
getFiberFromScopeInstance,
getInstanceFromNode as getInstanceFromNodeDOMTree,
isContainerMarkedAsRoot,
replaceNode,
detachDeletedInstance,
} from './ReactDOMComponentTree';
export {detachDeletedInstance} from './ReactDOMComponentTree';
export {detachDeletedInstance};
import {hasRole} from './DOMAccessibilityRoles';
import {
createElement,
Expand Down Expand Up @@ -484,29 +486,45 @@ export function appendChildToContainer(
container: Container,
child: Instance | TextInstance,
): void {
let parentNode;
if (container.nodeType === COMMENT_NODE) {
parentNode = (container.parentNode: any);
parentNode.insertBefore(child, container);
} else {
parentNode = container;
parentNode.appendChild(child);
}
// This container might be used for a portal.
// If something inside a portal is clicked, that click should bubble
// through the React tree. However, on Mobile Safari the click would
// never bubble through the *DOM* tree unless an ancestor with onclick
// event exists. So we wouldn't see it and dispatch it.
// This is why we ensure that non React root containers have inline onclick
// defined.
// https://github.com/facebook/react/issues/11918
const reactRootContainer = container._reactRootContainer;
if (
(reactRootContainer === null || reactRootContainer === undefined) &&
parentNode.onclick === null
) {
// TODO: This cast may not be sound for SVG, MathML or custom elements.
trapClickOnNonInteractiveElement(((parentNode: any): HTMLElement));
switch (container.nodeType) {
case DOCUMENT_NODE: {
const documentElement = container.documentElement;
// We cannot append an html element to the document when another one is present
// We also cannot remove the html element without breaking some browsers. Instead
// we are going to swap the current node with the document root element and append
// the head and children to this container. It should have already been cleared
// during an earlier step of the commit phase.

replaceNode(child, documentElement);
const childrenToAppend = child.childNodes;
while (childrenToAppend.length) {
documentElement.appendChild(childrenToAppend[0]);
}
return;
}
case COMMENT_NODE: {
container.parentNode.insertBefore(child, container);
return;
}
default: {
container.appendChild(child);
// This container might be used for a portal.
// If something inside a portal is clicked, that click should bubble
// through the React tree. However, on Mobile Safari the click would
// never bubble through the *DOM* tree unless an ancestor with onclick
// event exists. So we wouldn't see it and dispatch it.
// This is why we ensure that non React root containers have inline onclick
// defined.
// https://github.com/facebook/react/issues/11918
const reactRootContainer = container._reactRootContainer;
if (
(reactRootContainer === null || reactRootContainer === undefined) &&
container.onclick === null
) {
// TODO: This cast may not be sound for SVG, MathML or custom elements.
trapClickOnNonInteractiveElement(((container: any): HTMLElement));
}
}
}
}

Expand Down Expand Up @@ -573,10 +591,24 @@ export function removeChildFromContainer(
container: Container,
child: Instance | TextInstance | SuspenseInstance,
): void {
if (container.nodeType === COMMENT_NODE) {
(container.parentNode: any).removeChild(child);
} else {
container.removeChild(child);
switch (container.nodeType) {
case DOCUMENT_NODE: {
detachDeletedInstance(child);
let childOfChild = child.firstChild;
while (childOfChild) {
child.removeChild(childOfChild);
childOfChild = child.firstChild;
}
return;
}
case COMMENT_NODE: {
(container.parentNode: any).removeChild(child);
return;
}
default: {
container.removeChild(child);
return;
}
}
}

Expand Down Expand Up @@ -672,10 +704,7 @@ export function clearContainer(container: Container): void {
if (container.nodeType === ELEMENT_NODE) {
((container: any): Element).textContent = '';
} else if (container.nodeType === DOCUMENT_NODE) {
const body = ((container: any): Document).body;
if (body != null) {
body.textContent = '';
}
((container: any): Document).documentElement.textContent = '';
}
}

Expand Down