Fix: Activity should hide portal contents

packages/react-dom/src/__tests__/ReactDOMActivity-test.js

@@ -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>',
+      );
+    },
+  );
+});

packages/react-reconciler/src/ReactFiberCommitWork.js

@@ -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) {

packages/react-reconciler/src/ReactFiberCompleteWork.js

@@ -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:

packages/react-reconciler/src/ReactFiberFlags.js

@@ -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;