Skip to content

Commit 1d68bce

Browse files
sebmarkbagesebmarkbage
authored
[Fiber] Don't unhide a node if a direct parent offscreen is still hidden (facebook#34821)
If an inner Offscreen commits an unhide, but an outer Offscreen is still hidden but they're controlling the same DOM node then we shouldn't unhide the DOM node yet. This keeps track of whether we're directly inside a hidden offscreen. It might be better to just do the tree search instead of keeping the stack state since it's a rare case. Although this hide/unhide path does trigger a lot of times even when there's no change. This was technically a bug with Suspense too but it doesn't appear because a suspended Suspense boundary never commits its partial state. If it did, it would trigger this same path. But it can happen with an outer Activity and inner Suspense.
1 parent ead9218 commit 1d68bce

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)