Skip to content

Commit 5268492

Browse files
acdliteacdlite
authored
Fix: Activity should hide portal contents (facebook#35091)
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.
1 parent c83be7d commit 5268492

File tree

4 files changed

+245
-57
lines changed

4 files changed

+245
-57
lines changed

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

Lines changed: 134 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,134 @@
1+
/**
2+
* Copyright (c) Meta Platforms, Inc. and affiliates.
3+
*
4+
* This source code is licensed under the MIT license found in the
5+
* LICENSE file in the root directory of this source tree.
6+
*
7+
* @emails react-core
8+
*/
9+
10+
'use strict';
11+
12+
let React;
13+
let Activity;
14+
let useState;
15+
let ReactDOM;
16+
let ReactDOMClient;
17+
let act;
18+
19+
describe('ReactDOMActivity', () => {
20+
let container;
21+
22+
beforeEach(() => {
23+
jest.resetModules();
24+
React = require('react');
25+
Activity = React.Activity;
26+
useState = React.useState;
27+
ReactDOM = require('react-dom');
28+
ReactDOMClient = require('react-dom/client');
29+
act = require('internal-test-utils').act;
30+
container = document.createElement('div');
31+
document.body.appendChild(container);
32+
});
33+
34+
afterEach(() => {
35+
document.body.removeChild(container);
36+
});
37+
38+
// @gate enableActivity
39+
it(
40+
'hiding an Activity boundary also hides the direct children of any ' +
41+
'portals it contains, regardless of how deeply nested they are',
42+
async () => {
43+
const portalContainer = document.createElement('div');
44+
45+
let setShow;
46+
function Accordion({children}) {
47+
const [shouldShow, _setShow] = useState(true);
48+
setShow = _setShow;
49+
return (
50+
<Activity mode={shouldShow ? 'visible' : 'hidden'}>
51+
{children}
52+
</Activity>
53+
);
54+
}
55+
56+
function App({portalContents}) {
57+
return (
58+
<Accordion>
59+
<div>
60+
{ReactDOM.createPortal(
61+
<div>Portal contents</div>,
62+
portalContainer,
63+
)}
64+
</div>
65+
</Accordion>
66+
);
67+
}
68+
69+
const root = ReactDOMClient.createRoot(container);
70+
await act(() => root.render(<App />));
71+
expect(container.innerHTML).toBe('<div></div>');
72+
expect(portalContainer.innerHTML).toBe('<div>Portal contents</div>');
73+
74+
// Hide the Activity boundary. Not only are the nearest DOM elements hidden,
75+
// but also the children of the nested portal contained within it.
76+
await act(() => setShow(false));
77+
expect(container.innerHTML).toBe('<div style="display: none;"></div>');
78+
expect(portalContainer.innerHTML).toBe(
79+
'<div style="display: none;">Portal contents</div>',
80+
);
81+
},
82+
);
83+
84+
// @gate enableActivity
85+
it(
86+
'revealing an Activity boundary inside a portal does not reveal the ' +
87+
'portal contents if has a hidden Activity parent',
88+
async () => {
89+
const portalContainer = document.createElement('div');
90+
91+
let setShow;
92+
function Accordion({children}) {
93+
const [shouldShow, _setShow] = useState(false);
94+
setShow = _setShow;
95+
return (
96+
<Activity mode={shouldShow ? 'visible' : 'hidden'}>
97+
{children}
98+
</Activity>
99+
);
100+
}
101+
102+
function App({portalContents}) {
103+
return (
104+
<Activity mode="hidden">
105+
<div>
106+
{ReactDOM.createPortal(
107+
<Accordion>
108+
<div>Portal contents</div>
109+
</Accordion>,
110+
portalContainer,
111+
)}
112+
</div>
113+
</Activity>
114+
);
115+
}
116+
117+
// Start with both boundaries hidden.
118+
const root = ReactDOMClient.createRoot(container);
119+
await act(() => root.render(<App />));
120+
expect(container.innerHTML).toBe('<div style="display: none;"></div>');
121+
expect(portalContainer.innerHTML).toBe(
122+
'<div style="display: none;">Portal contents</div>',
123+
);
124+
125+
// Reveal the inner Activity boundary. It should not reveal its children,
126+
// because there's a parent Activity boundary that is still hidden.
127+
await act(() => setShow(true));
128+
expect(container.innerHTML).toBe('<div style="display: none;"></div>');
129+
expect(portalContainer.innerHTML).toBe(
130+
'<div style="display: none;">Portal contents</div>',
131+
);
132+
},
133+
);
134+
});

‎packages/react-reconciler/src/ReactFiberCommitWork.js‎

Lines changed: 102 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -117,6 +117,7 @@ import {
117117
DidCapture,
118118
AffectedParentLayout,
119119
ViewTransitionNamedStatic,
120+
PortalStatic,
120121
} from './ReactFiberFlags';
121122
import {
122123
commitStartTime,
@@ -1182,66 +1183,104 @@ function commitTransitionProgress(offscreenFiber: Fiber) {
11821183
}
11831184
}
11841185

1185-
function hideOrUnhideAllChildren(finishedWork: Fiber, isHidden: boolean) {
1186-
// Only hide or unhide the top-most host nodes.
1187-
let hostSubtreeRoot = null;
1186+
function hideOrUnhideAllChildren(parentFiber: Fiber, isHidden: boolean) {
1187+
if (!supportsMutation) {
1188+
return;
1189+
}
1190+
// Finds the nearest host component children and updates their visibility
1191+
// to either hidden or visible.
1192+
let child = parentFiber.child;
1193+
while (child !== null) {
1194+
hideOrUnhideAllChildrenOnFiber(child, isHidden);
1195+
child = child.sibling;
1196+
}
1197+
}
11881198

1189-
if (supportsMutation) {
1190-
// We only have the top Fiber that was inserted but we need to recurse down its
1191-
// children to find all the terminal nodes.
1192-
let node: Fiber = finishedWork;
1193-
while (true) {
1194-
if (
1195-
node.tag === HostComponent ||
1196-
(supportsResources ? node.tag === HostHoistable : false)
1197-
) {
1198-
if (hostSubtreeRoot === null) {
1199-
hostSubtreeRoot = node;
1200-
commitShowHideHostInstance(node, isHidden);
1201-
}
1202-
} else if (node.tag === HostText) {
1203-
if (hostSubtreeRoot === null) {
1204-
commitShowHideHostTextInstance(node, isHidden);
1205-
}
1206-
} else if (node.tag === DehydratedFragment) {
1207-
if (hostSubtreeRoot === null) {
1208-
commitShowHideSuspenseBoundary(node, isHidden);
1209-
}
1210-
} else if (
1211-
(node.tag === OffscreenComponent ||
1212-
node.tag === LegacyHiddenComponent) &&
1213-
(node.memoizedState: OffscreenState) !== null &&
1214-
node !== finishedWork
1215-
) {
1199+
function hideOrUnhideAllChildrenOnFiber(fiber: Fiber, isHidden: boolean) {
1200+
if (!supportsMutation) {
1201+
return;
1202+
}
1203+
switch (fiber.tag) {
1204+
case HostComponent:
1205+
case HostHoistable: {
1206+
// Found the nearest host component. Hide it.
1207+
commitShowHideHostInstance(fiber, isHidden);
1208+
// Typically, only the nearest host nodes need to be hidden, since that
1209+
// has the effect of also hiding everything inside of them.
1210+
//
1211+
// However, there's a special case for portals, because portals do not
1212+
// exist in the regular host tree hierarchy; we can't assume that just
1213+
// because a portal's HostComponent parent in the React tree will also be
1214+
// a parent in the actual host tree.
1215+
//
1216+
// So, if any portals exist within the tree, regardless of how deeply
1217+
// nested they are, we need to repeat this algorithm for its children.
1218+
hideOrUnhideNearestPortals(fiber, isHidden);
1219+
return;
1220+
}
1221+
case HostText: {
1222+
commitShowHideHostTextInstance(fiber, isHidden);
1223+
return;
1224+
}
1225+
case DehydratedFragment: {
1226+
commitShowHideSuspenseBoundary(fiber, isHidden);
1227+
return;
1228+
}
1229+
case OffscreenComponent:
1230+
case LegacyHiddenComponent: {
1231+
const offscreenState: OffscreenState | null = fiber.memoizedState;
1232+
if (offscreenState !== null) {
12161233
// Found a nested Offscreen component that is hidden.
12171234
// Don't search any deeper. This tree should remain hidden.
1218-
} else if (node.child !== null) {
1219-
node.child.return = node;
1220-
node = node.child;
1221-
continue;
1222-
}
1223-
1224-
if (node === finishedWork) {
1225-
return;
1235+
} else {
1236+
hideOrUnhideAllChildren(fiber, isHidden);
12261237
}
1227-
while (node.sibling === null) {
1228-
if (node.return === null || node.return === finishedWork) {
1229-
return;
1230-
}
1231-
1232-
if (hostSubtreeRoot === node) {
1233-
hostSubtreeRoot = null;
1234-
}
1238+
return;
1239+
}
1240+
default: {
1241+
hideOrUnhideAllChildren(fiber, isHidden);
1242+
return;
1243+
}
1244+
}
1245+
}
12351246

1236-
node = node.return;
1237-
}
1247+
function hideOrUnhideNearestPortals(parentFiber: Fiber, isHidden: boolean) {
1248+
if (!supportsMutation) {
1249+
return;
1250+
}
1251+
if (parentFiber.subtreeFlags & PortalStatic) {
1252+
let child = parentFiber.child;
1253+
while (child !== null) {
1254+
hideOrUnhideNearestPortalsOnFiber(child, isHidden);
1255+
child = child.sibling;
1256+
}
1257+
}
1258+
}
12381259

1239-
if (hostSubtreeRoot === node) {
1240-
hostSubtreeRoot = null;
1260+
function hideOrUnhideNearestPortalsOnFiber(fiber: Fiber, isHidden: boolean) {
1261+
if (!supportsMutation) {
1262+
return;
1263+
}
1264+
switch (fiber.tag) {
1265+
case HostPortal: {
1266+
// Found a portal. Switch back to the normal hide/unhide algorithm to
1267+
// toggle the visibility of its children.
1268+
hideOrUnhideAllChildrenOnFiber(fiber, isHidden);
1269+
return;
1270+
}
1271+
case OffscreenComponent: {
1272+
const offscreenState: OffscreenState | null = fiber.memoizedState;
1273+
if (offscreenState !== null) {
1274+
// Found a nested Offscreen component that is hidden. Don't search any
1275+
// deeper. This tree should remain hidden.
1276+
} else {
1277+
hideOrUnhideNearestPortals(fiber, isHidden);
12411278
}
1242-
1243-
node.sibling.return = node.return;
1244-
node = node.sibling;
1279+
return;
1280+
}
1281+
default: {
1282+
hideOrUnhideNearestPortals(fiber, isHidden);
1283+
return;
12451284
}
12461285
}
12471286
}
@@ -2305,6 +2344,15 @@ function commitMutationEffectsOnFiber(
23052344
break;
23062345
}
23072346
case HostPortal: {
2347+
// For the purposes of visibility toggling, the direct children of a
2348+
// portal are considered "children" of the nearest hidden
2349+
// OffscreenComponent, regardless of whether there are any host components
2350+
// in between them. This is because portals are not part of the regular
2351+
// host tree hierarchy; we can't assume that just because a portal's
2352+
// HostComponent parent in the React tree will also be a parent in the
2353+
// actual host tree. So we must hide all of them.
2354+
const prevOffscreenDirectParentIsHidden = offscreenDirectParentIsHidden;
2355+
offscreenDirectParentIsHidden = offscreenSubtreeIsHidden;
23082356
const prevMutationContext = pushMutationContext();
23092357
if (supportsResources) {
23102358
const previousHoistableRoot = currentHoistableRoot;
@@ -2326,6 +2374,7 @@ function commitMutationEffectsOnFiber(
23262374
rootViewTransitionAffected = true;
23272375
}
23282376
popMutationContext(prevMutationContext);
2377+
offscreenDirectParentIsHidden = prevOffscreenDirectParentIsHidden;
23292378

23302379
if (flags & Update) {
23312380
if (supportsPersistence) {

‎packages/react-reconciler/src/ReactFiberCompleteWork.js‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -99,6 +99,7 @@ import {
9999
Cloned,
100100
ViewTransitionStatic,
101101
Hydrate,
102+
PortalStatic,
102103
} from './ReactFiberFlags';
103104

104105
import {
@@ -1665,6 +1666,7 @@ function completeWork(
16651666
if (current === null) {
16661667
preparePortalMount(workInProgress.stateNode.containerInfo);
16671668
}
1669+
workInProgress.flags |= PortalStatic;
16681670
bubbleProperties(workInProgress);
16691671
return null;
16701672
case ContextProvider:

‎packages/react-reconciler/src/ReactFiberFlags.js‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -83,11 +83,13 @@ export const ViewTransitionNamedStatic =
8383
// ViewTransitionStatic tracks whether there are an ViewTransition components from
8484
// the nearest HostComponent down. It resets at every HostComponent level.
8585
export const ViewTransitionStatic = /* */ 0b0000010000000000000000000000000;
86+
// Tracks whether a HostPortal is present in the tree.
87+
export const PortalStatic = /* */ 0b0000100000000000000000000000000;
8688

8789
// Flag used to identify newly inserted fibers. It isn't reset after commit unlike `Placement`.
88-
export const PlacementDEV = /* */ 0b0000100000000000000000000000000;
89-
export const MountLayoutDev = /* */ 0b0001000000000000000000000000000;
90-
export const MountPassiveDev = /* */ 0b0010000000000000000000000000000;
90+
export const PlacementDEV = /* */ 0b0001000000000000000000000000000;
91+
export const MountLayoutDev = /* */ 0b0010000000000000000000000000000;
92+
export const MountPassiveDev = /* */ 0b0100000000000000000000000000000;
9193

9294
// Groups of flags that are used in the commit phase to skip over trees that
9395
// don't contain effects, by checking subtreeFlags.
@@ -139,4 +141,5 @@ export const StaticMask =
139141
RefStatic |
140142
MaySuspendCommit |
141143
ViewTransitionStatic |
142-
ViewTransitionNamedStatic;
144+
ViewTransitionNamedStatic |
145+
PortalStatic;

0 commit comments

Comments
 (0)