Repository: facebook/react
Pull request: Fix: Activity should hide portal contents

Fixes https://github.com/facebook/react/issues/35000

This PR updates the behavior of Activity so that when it is hidden, it hides the contents of any portals contained within it.

Previously we had intentionally chosen not to implement this behavior, because it was thought that this concern should be left to the userspace code that manages the portal, e.g. by adding or removing the portal container from the DOM. Depending on the use case for the portal, this is often desirable anyway because the portal container itself is not controlled by React.

However, React does own the _contents_ of the portal, and we can hide those elements regardless of what the user chooses to do with the container. This makes the hiding/unhiding behavior of portals with Activity automatic in the majority of cases, and also benefits from aligning the DOM mutations with the rest of the React's commit phase lifecycle.

The reason we have to special case this at all is because usually we only hide the direct DOM children of the Activity boundary. There's no reason to go deeper than that, because hiding a parent DOM element effectively hides everything inside of it. Portals are the exception, because they don't exist in the normal DOM hierarchy; we can't assume that just because a portal has a parent in the React tree that it will also have that parent in the actual DOM.

So, whenever an Activity boundary is hidden, we must search for and hide _any_ portal that is contained within it, and recursively hide its direct children, too.

To optimize this search, we use a new subtree flag, PortalStatic, that is set only on fiber paths that contain a HostPortal. This lets us skip over any subtree that does not contain a portal.

Changed files:
filename: packages/react-dom/src/__tests__/ReactDOMActivity-test.js
patch: @@ -0,0 +1,134 @@
+/**
+ * Copyright (c) Meta Platforms, Inc. and affiliates.
+ *
+ * This source code is licensed under the MIT license found in the
+ * LICENSE file in the root directory of this source tree.
+ *
+ * @emails react-core
+ */
+
+'use strict';
+
+let React;
+let Activity;
+let useState;
+let ReactDOM;
+let ReactDOMClient;
+let act;
+
+describe('ReactDOMActivity', () => {
+  let container;
+
+  beforeEach(() => {
+    jest.resetModules();
+    React = require('react');
+    Activity = React.Activity;
+    useState = React.useState;
+    ReactDOM = require('react-dom');
+    ReactDOMClient = require('react-dom/client');
+    act = require('internal-test-utils').act;
+    container = document.createElement('div');
+    document.body.appendChild(container);
+  });
+
+  afterEach(() => {
+    document.body.removeChild(container);
+  });
+
+  // @gate enableActivity
+  it(
+    'hiding an Activity boundary also hides the direct children of any ' +
+      'portals it contains, regardless of how deeply nested they are',
+    async () => {
+      const portalContainer = document.createElement('div');
+
+      let setShow;
+      function Accordion({children}) {
+        const [shouldShow, _setShow] = useState(true);
+        setShow = _setShow;
+        return (
+          <Activity mode={shouldShow ? 'visible' : 'hidden'}>
+            {children}
+          </Activity>
+        );
+      }
+
+      function App({portalContents}) {
+        return (
+          <Accordion>
+            <div>
+              {ReactDOM.createPortal(
+                <div>Portal contents</div>,
+                portalContainer,
+              )}
+            </div>
+          </Accordion>
+        );
+      }
+
+      const root = ReactDOMClient.createRoot(container);
+      await act(() => root.render(<App />));
+      expect(container.innerHTML).toBe('<div></div>');
+      expect(portalContainer.innerHTML).toBe('<div>Portal contents</div>');
+
+      // Hide the Activity boundary. Not only are the nearest DOM elements hidden,
+      // but also the children of the nested portal contained within it.
+      await act(() => setShow(false));
+      expect(container.innerHTML).toBe('<div style="display: none;"></div>');
+      expect(portalContainer.innerHTML).toBe(
+        '<div style="display: none;">Portal contents</div>',
+      );
+    },
+  );
+
+  // @gate enableActivity
+  it(
+    'revealing an Activity boundary inside a portal does not reveal the ' +
+      'portal contents if has a hidden Activity parent',
+    async () => {
+      const portalContainer = document.createElement('div');
+
+      let setShow;
+      function Accordion({children}) {
+        const [shouldShow, _setShow] = useState(false);
+        setShow = _setShow;
+        return (
+          <Activity mode={shouldShow ? 'visible' : 'hidden'}>
+            {children}
+          </Activity>
+        );
+      }
+
+      function App({portalContents}) {
+        return (
+          <Activity mode="hidden">
+            <div>
+              {ReactDOM.createPortal(
+                <Accordion>
+                  <div>Portal contents</div>
+                </Accordion>,
+                portalContainer,
+              )}
+            </div>
+          </Activity>
+        );
+      }
+
+      // Start with both boundaries hidden.
+      const root = ReactDOMClient.createRoot(container);
+      await act(() => root.render(<App />));
+      expect(container.innerHTML).toBe('<div style="display: none;"></div>');
+      expect(portalContainer.innerHTML).toBe(
+        '<div style="display: none;">Portal contents</div>',
+      );
+
+      // Reveal the inner Activity boundary. It should not reveal its children,
+      // because there's a parent Activity boundary that is still hidden.
+      await act(() => setShow(true));
+      expect(container.innerHTML).toBe('<div style="display: none;"></div>');
+      expect(portalContainer.innerHTML).toBe(
+        '<div style="display: none;">Portal contents</div>',
+      );
+    },
+  );
+});

filename: packages/react-reconciler/src/ReactFiberCommitWork.js
patch: @@ -117,6 +117,7 @@ import {
   DidCapture,
   AffectedParentLayout,
   ViewTransitionNamedStatic,
+  PortalStatic,
 } from './ReactFiberFlags';
 import {
   commitStartTime,
@@ -1182,66 +1183,104 @@ function commitTransitionProgress(offscreenFiber: Fiber) {
   }
 }
 
-function hideOrUnhideAllChildren(finishedWork: Fiber, isHidden: boolean) {
-  // Only hide or unhide the top-most host nodes.
-  let hostSubtreeRoot = null;
+function hideOrUnhideAllChildren(parentFiber: Fiber, isHidden: boolean) {
+  if (!supportsMutation) {
+    return;
+  }
+  // Finds the nearest host component children and updates their visibility
+  // to either hidden or visible.
+  let child = parentFiber.child;
+  while (child !== null) {
+    hideOrUnhideAllChildrenOnFiber(child, isHidden);
+    child = child.sibling;
+  }
+}
 
-  if (supportsMutation) {
-    // We only have the top Fiber that was inserted but we need to recurse down its
-    // children to find all the terminal nodes.
-    let node: Fiber = finishedWork;
-    while (true) {
-      if (
-        node.tag === HostComponent ||
-        (supportsResources ? node.tag === HostHoistable : false)
-      ) {
-        if (hostSubtreeRoot === null) {
-          hostSubtreeRoot = node;
-          commitShowHideHostInstance(node, isHidden);
-        }
-      } else if (node.tag === HostText) {
-        if (hostSubtreeRoot === null) {
-          commitShowHideHostTextInstance(node, isHidden);
-        }
-      } else if (node.tag === DehydratedFragment) {
-        if (hostSubtreeRoot === null) {
-          commitShowHideSuspenseBoundary(node, isHidden);
-        }
-      } else if (
-        (node.tag === OffscreenComponent ||
-          node.tag === LegacyHiddenComponent) &&
-        (node.memoizedState: OffscreenState) !== null &&
-        node !== finishedWork
-      ) {
+function hideOrUnhideAllChildrenOnFiber(fiber: Fiber, isHidden: boolean) {
+  if (!supportsMutation) {
+    return;
+  }
+  switch (fiber.tag) {
+    case HostComponent:
+    case HostHoistable: {
+      // Found the nearest host component. Hide it.
+      commitShowHideHostInstance(fiber, isHidden);
+      // Typically, only the nearest host nodes need to be hidden, since that
+      // has the effect of also hiding everything inside of them.
+      //
+      // However, there's a special case for portals, because portals do not
+      // exist in the regular host tree hierarchy; we can't assume that just
+      // because a portal's HostComponent parent in the React tree will also be
+      // a parent in the actual host tree.
+      //
+      // So, if any portals exist within the tree, regardless of how deeply
+      // nested they are, we need to repeat this algorithm for its children.
+      hideOrUnhideNearestPortals(fiber, isHidden);
+      return;
+    }
+    case HostText: {
+      commitShowHideHostTextInstance(fiber, isHidden);
+      return;
+    }
+    case DehydratedFragment: {
+      commitShowHideSuspenseBoundary(fiber, isHidden);
+      return;
+    }
+    case OffscreenComponent:
+    case LegacyHiddenComponent: {
+      const offscreenState: OffscreenState | null = fiber.memoizedState;
+      if (offscreenState !== null) {
         // Found a nested Offscreen component that is hidden.
         // Don't search any deeper. This tree should remain hidden.
-      } else if (node.child !== null) {
-        node.child.return = node;
-        node = node.child;
-        continue;
-      }
-
-      if (node === finishedWork) {
-        return;
+      } else {
+        hideOrUnhideAllChildren(fiber, isHidden);
       }
-      while (node.sibling === null) {
-        if (node.return === null || node.return === finishedWork) {
-          return;
-        }
-
-        if (hostSubtreeRoot === node) {
-          hostSubtreeRoot = null;
-        }
+      return;
+    }
+    default: {
+      hideOrUnhideAllChildren(fiber, isHidden);
+      return;
+    }
+  }
+}
 
-        node = node.return;
-      }
+function hideOrUnhideNearestPortals(parentFiber: Fiber, isHidden: boolean) {
+  if (!supportsMutation) {
+    return;
+  }
+  if (parentFiber.subtreeFlags & PortalStatic) {
+    let child = parentFiber.child;
+    while (child !== null) {
+      hideOrUnhideNearestPortalsOnFiber(child, isHidden);
+      child = child.sibling;
+    }
+  }
+}
 
-      if (hostSubtreeRoot === node) {
-        hostSubtreeRoot = null;
+function hideOrUnhideNearestPortalsOnFiber(fiber: Fiber, isHidden: boolean) {
+  if (!supportsMutation) {
+    return;
+  }
+  switch (fiber.tag) {
+    case HostPortal: {
+      // Found a portal. Switch back to the normal hide/unhide algorithm to
+      // toggle the visibility of its children.
+      hideOrUnhideAllChildrenOnFiber(fiber, isHidden);
+      return;
+    }
+    case OffscreenComponent: {
+      const offscreenState: OffscreenState | null = fiber.memoizedState;
+      if (offscreenState !== null) {
+        // Found a nested Offscreen component that is hidden. Don't search any
+        // deeper. This tree should remain hidden.
+      } else {
+        hideOrUnhideNearestPortals(fiber, isHidden);
       }
-
-      node.sibling.return = node.return;
-      node = node.sibling;
+      return;
+    }
+    default: {
+      hideOrUnhideNearestPortals(fiber, isHidden);
+      return;
     }
   }
 }
@@ -2305,6 +2344,15 @@ function commitMutationEffectsOnFiber(
       break;
     }
     case HostPortal: {
+      // For the purposes of visibility toggling, the direct children of a
+      // portal are considered "children" of the nearest hidden
+      // OffscreenComponent, regardless of whether there are any host components
+      // in between them. This is because portals are not part of the regular
+      // host tree hierarchy; we can't assume that just because a portal's
+      // HostComponent parent in the React tree will also be a parent in the
+      // actual host tree. So we must hide all of them.
+      const prevOffscreenDirectParentIsHidden = offscreenDirectParentIsHidden;
+      offscreenDirectParentIsHidden = offscreenSubtreeIsHidden;
       const prevMutationContext = pushMutationContext();
       if (supportsResources) {
         const previousHoistableRoot = currentHoistableRoot;
@@ -2326,6 +2374,7 @@ function commitMutationEffectsOnFiber(
         rootViewTransitionAffected = true;
       }
       popMutationContext(prevMutationContext);
+      offscreenDirectParentIsHidden = prevOffscreenDirectParentIsHidden;
 
       if (flags & Update) {
         if (supportsPersistence) {

filename: packages/react-reconciler/src/ReactFiberCompleteWork.js
patch: @@ -99,6 +99,7 @@ import {
   Cloned,
   ViewTransitionStatic,
   Hydrate,
+  PortalStatic,
 } from './ReactFiberFlags';
 
 import {
@@ -1665,6 +1666,7 @@ function completeWork(
       if (current === null) {
         preparePortalMount(workInProgress.stateNode.containerInfo);
       }
+      workInProgress.flags |= PortalStatic;
       bubbleProperties(workInProgress);
       return null;
     case ContextProvider:

filename: packages/react-reconciler/src/ReactFiberFlags.js
patch: @@ -83,11 +83,13 @@ export const ViewTransitionNamedStatic =
 // ViewTransitionStatic tracks whether there are an ViewTransition components from
 // the nearest HostComponent down. It resets at every HostComponent level.
 export const ViewTransitionStatic = /*         */ 0b0000010000000000000000000000000;
+// Tracks whether a HostPortal is present in the tree.
+export const PortalStatic = /*                 */ 0b0000100000000000000000000000000;
 
 // Flag used to identify newly inserted fibers. It isn't reset after commit unlike `Placement`.
-export const PlacementDEV = /*                 */ 0b0000100000000000000000000000000;
-export const MountLayoutDev = /*               */ 0b0001000000000000000000000000000;
-export const MountPassiveDev = /*              */ 0b0010000000000000000000000000000;
+export const PlacementDEV = /*                 */ 0b0001000000000000000000000000000;
+export const MountLayoutDev = /*               */ 0b0010000000000000000000000000000;
+export const MountPassiveDev = /*              */ 0b0100000000000000000000000000000;
 
 // Groups of flags that are used in the commit phase to skip over trees that
 // don't contain effects, by checking subtreeFlags.
@@ -139,4 +141,5 @@ export const StaticMask =
   RefStatic |
   MaySuspendCommit |
   ViewTransitionStatic |
-  ViewTransitionNamedStatic;
+  ViewTransitionNamedStatic |
+  PortalStatic;