Skip to content

Commit fb7a66f

Browse files
sebmarkbagesebmarkbage
committed
Don't unhide if a parent offscreen is still hidden
1 parent ead9218 commit fb7a66f

File tree

3 files changed

+184
-3
lines changed

3 files changed

+184
-3
lines changed

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

Lines changed: 17 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -292,6 +292,9 @@ import type {Flags} from './ReactFiberFlags';
292292
// Allows us to avoid traversing the return path to find the nearest Offscreen ancestor.
293293
let offscreenSubtreeIsHidden: boolean = false;
294294
let offscreenSubtreeWasHidden: boolean = false;
295+
// Track whether there's a hidden offscreen above with no HostComponent between. If so,
296+
// it overrides the hiddenness of the HostComponent below.
297+
let offscreenDirectParentIsHidden: boolean = false;
295298

296299
// Used to track if a form needs to be reset at the end of the mutation phase.
297300
let needsFormReset = false;
@@ -2141,8 +2144,14 @@ function commitMutationEffectsOnFiber(
21412144
// Fall through
21422145
}
21432146
case HostComponent: {
2147+
// We've hit a host component, so it's no longer a direct parent.
2148+
const prevOffscreenDirectParentIsHidden = offscreenDirectParentIsHidden;
2149+
offscreenDirectParentIsHidden = false;
2150+
21442151
recursivelyTraverseMutationEffects(root, finishedWork, lanes);
21452152

2153+
offscreenDirectParentIsHidden = prevOffscreenDirectParentIsHidden;
2154+
21462155
commitReconciliationEffects(finishedWork, lanes);
21472156

21482157
if (flags & Ref) {
@@ -2422,10 +2431,14 @@ function commitMutationEffectsOnFiber(
24222431
// effects again.
24232432
const prevOffscreenSubtreeIsHidden = offscreenSubtreeIsHidden;
24242433
const prevOffscreenSubtreeWasHidden = offscreenSubtreeWasHidden;
2434+
const prevOffscreenDirectParentIsHidden = offscreenDirectParentIsHidden;
24252435
offscreenSubtreeIsHidden = prevOffscreenSubtreeIsHidden || isHidden;
2436+
offscreenDirectParentIsHidden =
2437+
prevOffscreenDirectParentIsHidden || isHidden;
24262438
offscreenSubtreeWasHidden = prevOffscreenSubtreeWasHidden || wasHidden;
24272439
recursivelyTraverseMutationEffects(root, finishedWork, lanes);
24282440
offscreenSubtreeWasHidden = prevOffscreenSubtreeWasHidden;
2441+
offscreenDirectParentIsHidden = prevOffscreenDirectParentIsHidden;
24292442
offscreenSubtreeIsHidden = prevOffscreenSubtreeIsHidden;
24302443

24312444
if (
@@ -2504,9 +2517,10 @@ function commitMutationEffectsOnFiber(
25042517
}
25052518

25062519
if (supportsMutation) {
2507-
// TODO: This needs to run whenever there's an insertion or update
2508-
// inside a hidden Offscreen tree.
2509-
hideOrUnhideAllChildren(finishedWork, isHidden);
2520+
// If it's trying to unhide but the parent is still hidden, then we should not unhide.
2521+
if (isHidden || !offscreenDirectParentIsHidden) {
2522+
hideOrUnhideAllChildren(finishedWork, isHidden);
2523+
}
25102524
}
25112525
}
25122526

‎packages/react-reconciler/src/__tests__/Activity-test.js‎

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ let waitForPaint;
1414
let waitFor;
1515
let assertLog;
1616
let assertConsoleErrorDev;
17+
let Suspense;
1718

1819
describe('Activity', () => {
1920
beforeEach(() => {
@@ -25,6 +26,7 @@ describe('Activity', () => {
2526
act = require('internal-test-utils').act;
2627
LegacyHidden = React.unstable_LegacyHidden;
2728
Activity = React.Activity;
29+
Suspense = React.Suspense;
2830
useState = React.useState;
2931
useInsertionEffect = React.useInsertionEffect;
3032
useLayoutEffect = React.useLayoutEffect;
@@ -1424,6 +1426,72 @@ describe('Activity', () => {
14241426
);
14251427
});
14261428

1429+
// @gate enableActivity
1430+
it('reveal an inner Activity boundary without revealing an outer one on the same host child', async () => {
1431+
// This ensures that no update is scheduled, which would cover up the bug if the parent
1432+
// then re-hides the child on the way up.
1433+
const memoizedElement = <div />;
1434+
function App({showOuter, showInner}) {
1435+
return (
1436+
<Activity mode={showOuter ? 'visible' : 'hidden'} name="Outer">
1437+
<Activity mode={showInner ? 'visible' : 'hidden'} name="Inner">
1438+
{memoizedElement}
1439+
</Activity>
1440+
</Activity>
1441+
);
1442+
}
1443+
1444+
const root = ReactNoop.createRoot();
1445+
1446+
// Prerender the whole tree.
1447+
await act(() => {
1448+
root.render(<App showOuter={false} showInner={false} />);
1449+
});
1450+
expect(root).toMatchRenderedOutput(<div hidden={true} />);
1451+
1452+
await act(() => {
1453+
root.render(<App showOuter={false} showInner={true} />);
1454+
});
1455+
expect(root).toMatchRenderedOutput(<div hidden={true} />);
1456+
});
1457+
1458+
// @gate enableActivity
1459+
it('reveal an inner Suspense boundary without revealing an outer Activity on the same host child', async () => {
1460+
// This ensures that no update is scheduled, which would cover up the bug if the parent
1461+
// then re-hides the child on the way up.
1462+
const memoizedElement = <div />;
1463+
const promise = new Promise(() => {});
1464+
function App({showOuter, showInner}) {
1465+
return (
1466+
<Activity mode={showOuter ? 'visible' : 'hidden'} name="Outer">
1467+
<Suspense name="Inner">
1468+
{memoizedElement}
1469+
{showInner ? null : promise}
1470+
</Suspense>
1471+
</Activity>
1472+
);
1473+
}
1474+
1475+
const root = ReactNoop.createRoot();
1476+
1477+
// Prerender the whole tree.
1478+
await act(() => {
1479+
root.render(<App showOuter={false} showInner={true} />);
1480+
});
1481+
expect(root).toMatchRenderedOutput(<div hidden={true} />);
1482+
1483+
// Resuspend the inner.
1484+
await act(() => {
1485+
root.render(<App showOuter={false} showInner={false} />);
1486+
});
1487+
expect(root).toMatchRenderedOutput(<div hidden={true} />);
1488+
1489+
await act(() => {
1490+
root.render(<App showOuter={false} showInner={true} />);
1491+
});
1492+
expect(root).toMatchRenderedOutput(<div hidden={true} />);
1493+
});
1494+
14271495
// @gate enableActivity
14281496
it('insertion effects are not disconnected when the visibility changes', async () => {
14291497
function Child({step}) {

‎packages/react-reconciler/src/__tests__/ReactSuspenseEffectsSemantics-test.js‎

Lines changed: 99 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1401,6 +1401,105 @@ describe('ReactSuspenseEffectsSemantics', () => {
14011401
);
14021402
});
14031403

1404+
// @gate enableLegacyCache
1405+
it('should wait to reveal an inner child when inner one reveals first', async () => {
1406+
function App({outerChildren, innerChildren}) {
1407+
return (
1408+
<Suspense fallback={<Text text="OuterFallback" />} name="Outer">
1409+
<Suspense fallback={<Text text="InnerFallback" />} name="Inner">
1410+
<div>{innerChildren}</div>
1411+
</Suspense>
1412+
{outerChildren}
1413+
</Suspense>
1414+
);
1415+
}
1416+
1417+
// Mount
1418+
await act(() => {
1419+
ReactNoop.render(<App />);
1420+
});
1421+
assertLog([]);
1422+
expect(ReactNoop).toMatchRenderedOutput(<div />);
1423+
1424+
// Resuspend inner boundary
1425+
await act(() => {
1426+
ReactNoop.render(
1427+
<App
1428+
outerChildren={null}
1429+
innerChildren={<AsyncText text="InnerAsync" />}
1430+
/>,
1431+
);
1432+
});
1433+
assertLog([
1434+
'Suspend:InnerAsync',
1435+
'Text:InnerFallback render',
1436+
'Text:InnerFallback create insertion',
1437+
'Text:InnerFallback create layout',
1438+
'Text:InnerFallback create passive',
1439+
'Suspend:InnerAsync',
1440+
]);
1441+
expect(ReactNoop).toMatchRenderedOutput(
1442+
<>
1443+
<div hidden={true} />
1444+
<span prop="InnerFallback" />
1445+
</>,
1446+
);
1447+
1448+
// Resuspend both boundaries
1449+
await act(() => {
1450+
ReactNoop.render(
1451+
<App
1452+
outerChildren={<AsyncText text="OuterAsync" />}
1453+
innerChildren={<AsyncText text="InnerAsync" />}
1454+
/>,
1455+
);
1456+
});
1457+
assertLog([
1458+
'Suspend:InnerAsync',
1459+
'Text:InnerFallback render',
1460+
'Suspend:OuterAsync',
1461+
'Text:OuterFallback render',
1462+
'Text:InnerFallback destroy layout',
1463+
'Text:OuterFallback create insertion',
1464+
'Text:OuterFallback create layout',
1465+
'Text:OuterFallback create passive',
1466+
'Suspend:InnerAsync',
1467+
'Text:InnerFallback render',
1468+
'Suspend:OuterAsync',
1469+
]);
1470+
expect(ReactNoop).toMatchRenderedOutput(
1471+
<>
1472+
<div hidden={true} />
1473+
<span prop="InnerFallback" hidden={true} />
1474+
<span prop="OuterFallback" />
1475+
</>,
1476+
);
1477+
1478+
// Unsuspend the inner Suspense subtree only
1479+
// Interestingly, this never commits because the tree is left suspended.
1480+
// If it did commit, it would potentially cause the div to incorrectly reappear.
1481+
await act(() => {
1482+
ReactNoop.render(
1483+
<App
1484+
outerChildren={<AsyncText text="OuterAsync" />}
1485+
innerChildren={null}
1486+
/>,
1487+
);
1488+
});
1489+
assertLog([
1490+
'Suspend:OuterAsync',
1491+
'Text:OuterFallback render',
1492+
'Suspend:OuterAsync',
1493+
]);
1494+
expect(ReactNoop).toMatchRenderedOutput(
1495+
<>
1496+
<div hidden={true} />
1497+
<span prop="InnerFallback" hidden={true} />
1498+
<span prop="OuterFallback" />
1499+
</>,
1500+
);
1501+
});
1502+
14041503
// @gate enableLegacyCache
14051504
it('should show nested host nodes if multiple boundaries resolve at the same time', async () => {
14061505
function App({innerChildren = null, outerChildren = null}) {

0 commit comments

Comments
 (0)