Retain documentElement when appending/removing from Document

gnoff · gnoff · commit 1818f2946f31 · 2022-05-06T09:59:25.000-07:00
creating a root or hydrating into a root with a Document container could error in surprising ways because the document root element was not removed when clearing the container and then a new html element was attempting to be appended to the document. Browsers do not allow more than one root element and this errored leading to a cascading failure where all content was unmounted from the tree breaking the application.

This change changes the behavior of clearContainer, appendChildToContainer, and removeChildFromContainer

clearContainer on a Document container will now remove all children of the documentElement (usually &lt;html&gt;)

appendChildToContainer on a Document container will now 'adopt' the existing documentElement, transferring all stateNode properties and children over to it rather than dirctly appending the child

removeChildFromContainer on a Document container will now remove all children of the documentElement but leave the documentElement in place
diff --git a/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js b/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js
@@ -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 () => {
diff --git a/packages/react-dom/src/__tests__/ReactRenderDocument-test.js b/packages/react-dom/src/__tests__/ReactRenderDocument-test.js
@@ -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 (
diff --git a/packages/react-dom/src/client/ReactDOMComponentTree.js b/packages/react-dom/src/client/ReactDOMComponentTree.js
@@ -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.
diff --git a/packages/react-dom/src/client/ReactDOMHostConfig.js b/packages/react-dom/src/client/ReactDOMHostConfig.js
@@ -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,
@@ -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));
+      }
+    }
   }
 }
 
@@ -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;
+    }
   }
 }
 
@@ -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 = '';
   }
 }
 
