[Fiber] Fix portal bugs

scripts/fiber/tests-passing.txt

@@ -514,6 +514,7 @@ src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js
 * should render one portal
 * should render many portals
 * should render nested portals
+* should reconcile portal children
 * should keep track of namespace across portals (simple)
 * should keep track of namespace across portals (medium)
 * should keep track of namespace across portals (complex)

src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js

@@ -367,6 +367,82 @@ describe('ReactDOMFiber', () => {
       expect(container.innerHTML).toBe('');
     });
 
+    it('should reconcile portal children', () => {
+      var portalContainer = document.createElement('div');
+
+      ReactDOM.render(
+        <div>
+          {ReactDOM.unstable_createPortal(
+            <div>portal:1</div>,
+            portalContainer
+          )}
+        </div>,
+        container
+      );
+      expect(portalContainer.innerHTML).toBe('<div>portal:1</div>');
+      expect(container.innerHTML).toBe('<div></div>');
+
+      ReactDOM.render(
+        <div>
+          {ReactDOM.unstable_createPortal(
+            <div>portal:2</div>,
+            portalContainer
+          )}
+        </div>,
+        container
+      );
+      expect(portalContainer.innerHTML).toBe('<div>portal:2</div>');
+      expect(container.innerHTML).toBe('<div></div>');
+
+      ReactDOM.render(
+        <div>
+          {ReactDOM.unstable_createPortal(
+            <p>portal:3</p>,
+            portalContainer
+          )}
+        </div>,
+        container
+      );
+      expect(portalContainer.innerHTML).toBe('<p>portal:3</p>');
+      expect(container.innerHTML).toBe('<div></div>');
+
+      ReactDOM.render(
+        <div>
+          {ReactDOM.unstable_createPortal(
+            ['Hi', 'Bye'],
+            portalContainer
+          )}
+        </div>,
+        container
+      );
+      expect(portalContainer.innerHTML).toBe('HiBye');
+      expect(container.innerHTML).toBe('<div></div>');
+
+      ReactDOM.render(
+        <div>
+          {ReactDOM.unstable_createPortal(
+            ['Bye', 'Hi'],
+            portalContainer
+          )}
+        </div>,
+        container
+      );
+      expect(portalContainer.innerHTML).toBe('ByeHi');
+      expect(container.innerHTML).toBe('<div></div>');
+
+      ReactDOM.render(
+        <div>
+          {ReactDOM.unstable_createPortal(
+            null,
+            portalContainer
+          )}
+        </div>,
+        container
+      );
+      expect(portalContainer.innerHTML).toBe('');
+      expect(container.innerHTML).toBe('<div></div>');
+    });
+
     it('should keep track of namespace across portals (simple)', () => {
       assertNamespacesMatch(
         <svg {...expectSVG}>
@@ -400,6 +476,19 @@ describe('ReactDOMFiber', () => {
     });
 
     it('should keep track of namespace across portals (medium)', () => {
+      assertNamespacesMatch(
+        <svg {...expectSVG}>
+          <image {...expectSVG} />
+          {usePortal(
+            <div {...expectHTML} />
+          )}
+          <image {...expectSVG} />
+          {usePortal(
+            <div {...expectHTML} />
+          )}
+          <image {...expectSVG} />
+        </svg>
+      );
       assertNamespacesMatch(
         <div {...expectHTML}>
           <math {...expectMath}>
@@ -613,15 +702,9 @@ describe('ReactDOMFiber', () => {
               </div>
             )}
           </ErrorBoundary>
-          {
-            /*
-            * TODO: enable. Currently this leads to stack overflow
-            * but it might be a bug in error boundaries rather than SVG or portals.
-            usePortal(
-              <div {...expectHTML} />
-            )
-            */
-          }
+          {usePortal(
+            <div {...expectHTML} />
+          )}
         </svg>
       );
     });

src/renderers/shared/fiber/ReactChildFiber.js

@@ -341,7 +341,7 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) {
     } else {
       // Update
       const existing = useFiber(current, priority);
-      existing.pendingProps = portal.children;
+      existing.pendingProps = portal;
       existing.return = returnFiber;
       return existing;
     }
@@ -976,7 +976,7 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) {
         ) {
           deleteRemainingChildren(returnFiber, child.sibling);
           const existing = useFiber(child, priority);
-          existing.pendingProps = portal.children;
+          existing.pendingProps = portal;
           existing.return = returnFiber;
           return existing;
         } else {

src/renderers/shared/fiber/ReactFiber.js

@@ -346,7 +346,7 @@ exports.createFiberFromYield = function(yieldNode : ReactYield, priorityLevel :
 
 exports.createFiberFromPortal = function(portal : ReactPortal, priorityLevel : PriorityLevel) : Fiber {
   const fiber = createFiber(HostPortal, portal.key);
-  fiber.pendingProps = portal.children;
+  fiber.pendingProps = portal;
   fiber.pendingWorkPriority = priorityLevel;
   fiber.stateNode = {
     containerInfo: portal.containerInfo,

src/renderers/shared/fiber/ReactFiberBeginWork.js

@@ -325,7 +325,7 @@ module.exports = function<T, P, I, TI, C, CX>(
 
   function updatePortalComponent(current, workInProgress) {
     const priorityLevel = workInProgress.pendingWorkPriority;
-    const nextChildren = workInProgress.pendingProps;
+    const nextChildren = workInProgress.pendingProps.children;
     if (!current) {
       // Portals are special because we don't append the children during mount
       // but at commit. Therefore we need to track insertions which the normal

src/renderers/shared/fiber/ReactFiberCommitWork.js

@@ -152,7 +152,7 @@ module.exports = function<T, P, I, TI, C, CX>(
           // If we don't have a child, try the siblings instead.
           continue siblings;
         }
-        if (!node.child) {
+        if (!node.child || node.tag === HostPortal) {
           continue siblings;
         } else {
           node.child.return = node;
@@ -265,16 +265,19 @@ module.exports = function<T, P, I, TI, C, CX>(
         // After all the children have unmounted, it is now safe to remove the
         // node from the tree.
         removeChild(parent, node.stateNode);
+        // Don't visit children because we already visited them.
       } else if (node.tag === HostPortal) {
         // When we go into a portal, it becomes the parent to remove from.
         // We will reassign it back when we pop the portal on the way up.
         parent = node.stateNode.containerInfo;
+        // Visit children because portals might contain host components.
         if (node.child) {
           node = node.child;
           continue;
         }
       } else {
         commitUnmount(node);
+        // Visit children because we may find more host components below.
         if (node.child) {
           // TODO: Coroutines need to visit the stateNode.
           node.child.return = node;
@@ -302,10 +305,22 @@ module.exports = function<T, P, I, TI, C, CX>(
   }
 
   function commitDeletion(current : Fiber) : void {
-    // Recursively delete all host nodes from the parent.
-    const parent = getHostParent(current);
-    // Detach refs and call componentWillUnmount() on the whole subtree.
-    unmountHostComponents(parent, current);
+    // Recursively delete the closest child host nodes from the closest host parent.
+    // Then detach refs and call componentWillUnmount() on the whole subtree.
+    if (current.tag === HostPortal) {
+      // When deleting a portal, there are no host nodes above it.
+      // It is a host parent itself.
+      const parent = current.stateNode.containerInfo;
+      let child = current.child;
+      while (child) {
+        unmountHostComponents(parent, child);
+        child = child.sibling;
+      }
+    } else {
+      // When deleting anything other than a portal, search for the host parent.
+      const parent = getHostParent(current);
+      unmountHostComponents(parent, current);
+    }
 
     // Cut off the return pointers to disconnect it from the tree. Ideally, we
     // should clear the child pointer of the parent alternate to let this