Skip to content

Commit a97b5ac

Browse files
acdliteBrian Vaughn
acdlite
and
Brian Vaughn
authored
[Bugfix] Don't hide/unhide unless visibility changes (#21875)
* Use Visibility flag to schedule a hide/show effect Instead of the Update flag, which is also used for other side-effects, like refs. I originally added the Visibility flag for this purpose in #20043 but it got reverted last winter when we were bisecting the effects refactor. * Added failing test case Co-authored-by: Brian Vaughn <bvaughn@fb.com>
1 parent f0efb7b commit a97b5ac

6 files changed

+223
-103
lines changed

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

+41-31
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,6 @@ import {
6666
ChildDeletion,
6767
Snapshot,
6868
Update,
69-
Callback,
7069
Ref,
7170
Hydrating,
7271
HydratingAndUpdate,
@@ -75,6 +74,7 @@ import {
7574
MutationMask,
7675
LayoutMask,
7776
PassiveMask,
77+
Visibility,
7878
} from './ReactFiberFlags';
7979
import getComponentNameFromFiber from 'react-reconciler/src/getComponentNameFromFiber';
8080
import invariant from 'shared/invariant';
@@ -615,7 +615,7 @@ function commitLayoutEffectOnFiber(
615615
finishedWork: Fiber,
616616
committedLanes: Lanes,
617617
): void {
618-
if ((finishedWork.flags & (Update | Callback)) !== NoFlags) {
618+
if ((finishedWork.flags & LayoutMask) !== NoFlags) {
619619
switch (finishedWork.tag) {
620620
case FunctionComponent:
621621
case ForwardRef:
@@ -1776,7 +1776,7 @@ function commitWork(current: Fiber | null, finishedWork: Fiber): void {
17761776
return;
17771777
}
17781778
case SuspenseComponent: {
1779-
commitSuspenseComponent(finishedWork);
1779+
commitSuspenseCallback(finishedWork);
17801780
attachSuspenseRetryListeners(finishedWork);
17811781
return;
17821782
}
@@ -1899,7 +1899,7 @@ function commitWork(current: Fiber | null, finishedWork: Fiber): void {
18991899
return;
19001900
}
19011901
case SuspenseComponent: {
1902-
commitSuspenseComponent(finishedWork);
1902+
commitSuspenseCallback(finishedWork);
19031903
attachSuspenseRetryListeners(finishedWork);
19041904
return;
19051905
}
@@ -1918,13 +1918,6 @@ function commitWork(current: Fiber | null, finishedWork: Fiber): void {
19181918
}
19191919
break;
19201920
}
1921-
case OffscreenComponent:
1922-
case LegacyHiddenComponent: {
1923-
const newState: OffscreenState | null = finishedWork.memoizedState;
1924-
const isHidden = newState !== null;
1925-
hideOrUnhideAllChildren(finishedWork, isHidden);
1926-
return;
1927-
}
19281921
}
19291922
invariant(
19301923
false,
@@ -1933,27 +1926,9 @@ function commitWork(current: Fiber | null, finishedWork: Fiber): void {
19331926
);
19341927
}
19351928

1936-
function commitSuspenseComponent(finishedWork: Fiber) {
1929+
function commitSuspenseCallback(finishedWork: Fiber) {
1930+
// TODO: Move this to passive phase
19371931
const newState: SuspenseState | null = finishedWork.memoizedState;
1938-
1939-
if (newState !== null) {
1940-
markCommitTimeOfFallback();
1941-
1942-
if (supportsMutation) {
1943-
// Hide the Offscreen component that contains the primary children. TODO:
1944-
// Ideally, this effect would have been scheduled on the Offscreen fiber
1945-
// itself. That's how unhiding works: the Offscreen component schedules an
1946-
// effect on itself. However, in this case, the component didn't complete,
1947-
// so the fiber was never added to the effect list in the normal path. We
1948-
// could have appended it to the effect list in the Suspense component's
1949-
// second pass, but doing it this way is less complicated. This would be
1950-
// simpler if we got rid of the effect list and traversed the tree, like
1951-
// we're planning to do.
1952-
const primaryChildParent: Fiber = (finishedWork.child: any);
1953-
hideOrUnhideAllChildren(primaryChildParent, true);
1954-
}
1955-
}
1956-
19571932
if (enableSuspenseCallback && newState !== null) {
19581933
const suspenseCallback = finishedWork.memoizedProps.suspenseCallback;
19591934
if (typeof suspenseCallback === 'function') {
@@ -2127,6 +2102,10 @@ function commitMutationEffects_complete(root: FiberRoot) {
21272102
}
21282103

21292104
function commitMutationEffectsOnFiber(finishedWork: Fiber, root: FiberRoot) {
2105+
// TODO: The factoring of this phase could probably be improved. Consider
2106+
// switching on the type of work before checking the flags. That's what
2107+
// we do in all the other phases. I think this one is only different
2108+
// because of the shared reconcilation logic below.
21302109
const flags = finishedWork.flags;
21312110

21322111
if (flags & ContentReset) {
@@ -2147,6 +2126,37 @@ function commitMutationEffectsOnFiber(finishedWork: Fiber, root: FiberRoot) {
21472126
}
21482127
}
21492128

2129+
if (flags & Visibility) {
2130+
switch (finishedWork.tag) {
2131+
case SuspenseComponent: {
2132+
const newState: OffscreenState | null = finishedWork.memoizedState;
2133+
if (newState !== null) {
2134+
markCommitTimeOfFallback();
2135+
// Hide the Offscreen component that contains the primary children.
2136+
// TODO: Ideally, this effect would have been scheduled on the
2137+
// Offscreen fiber itself. That's how unhiding works: the Offscreen
2138+
// component schedules an effect on itself. However, in this case, the
2139+
// component didn't complete, so the fiber was never added to the
2140+
// effect list in the normal path. We could have appended it to the
2141+
// effect list in the Suspense component's second pass, but doing it
2142+
// this way is less complicated. This would be simpler if we got rid
2143+
// of the effect list and traversed the tree, like we're planning to
2144+
// do.
2145+
const primaryChildParent: Fiber = (finishedWork.child: any);
2146+
hideOrUnhideAllChildren(primaryChildParent, true);
2147+
}
2148+
break;
2149+
}
2150+
case OffscreenComponent:
2151+
case LegacyHiddenComponent: {
2152+
const newState: OffscreenState | null = finishedWork.memoizedState;
2153+
const isHidden = newState !== null;
2154+
hideOrUnhideAllChildren(finishedWork, isHidden);
2155+
break;
2156+
}
2157+
}
2158+
}
2159+
21502160
// The following switch statement is only concerned about placement,
21512161
// updates, and deletions. To avoid needing to add a case for every possible
21522162
// bitmap value, we remove the secondary effects from the effect tag and

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

+41-31
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,6 @@ import {
6666
ChildDeletion,
6767
Snapshot,
6868
Update,
69-
Callback,
7069
Ref,
7170
Hydrating,
7271
HydratingAndUpdate,
@@ -75,6 +74,7 @@ import {
7574
MutationMask,
7675
LayoutMask,
7776
PassiveMask,
77+
Visibility,
7878
} from './ReactFiberFlags';
7979
import getComponentNameFromFiber from 'react-reconciler/src/getComponentNameFromFiber';
8080
import invariant from 'shared/invariant';
@@ -615,7 +615,7 @@ function commitLayoutEffectOnFiber(
615615
finishedWork: Fiber,
616616
committedLanes: Lanes,
617617
): void {
618-
if ((finishedWork.flags & (Update | Callback)) !== NoFlags) {
618+
if ((finishedWork.flags & LayoutMask) !== NoFlags) {
619619
switch (finishedWork.tag) {
620620
case FunctionComponent:
621621
case ForwardRef:
@@ -1776,7 +1776,7 @@ function commitWork(current: Fiber | null, finishedWork: Fiber): void {
17761776
return;
17771777
}
17781778
case SuspenseComponent: {
1779-
commitSuspenseComponent(finishedWork);
1779+
commitSuspenseCallback(finishedWork);
17801780
attachSuspenseRetryListeners(finishedWork);
17811781
return;
17821782
}
@@ -1899,7 +1899,7 @@ function commitWork(current: Fiber | null, finishedWork: Fiber): void {
18991899
return;
19001900
}
19011901
case SuspenseComponent: {
1902-
commitSuspenseComponent(finishedWork);
1902+
commitSuspenseCallback(finishedWork);
19031903
attachSuspenseRetryListeners(finishedWork);
19041904
return;
19051905
}
@@ -1918,13 +1918,6 @@ function commitWork(current: Fiber | null, finishedWork: Fiber): void {
19181918
}
19191919
break;
19201920
}
1921-
case OffscreenComponent:
1922-
case LegacyHiddenComponent: {
1923-
const newState: OffscreenState | null = finishedWork.memoizedState;
1924-
const isHidden = newState !== null;
1925-
hideOrUnhideAllChildren(finishedWork, isHidden);
1926-
return;
1927-
}
19281921
}
19291922
invariant(
19301923
false,
@@ -1933,27 +1926,9 @@ function commitWork(current: Fiber | null, finishedWork: Fiber): void {
19331926
);
19341927
}
19351928

1936-
function commitSuspenseComponent(finishedWork: Fiber) {
1929+
function commitSuspenseCallback(finishedWork: Fiber) {
1930+
// TODO: Move this to passive phase
19371931
const newState: SuspenseState | null = finishedWork.memoizedState;
1938-
1939-
if (newState !== null) {
1940-
markCommitTimeOfFallback();
1941-
1942-
if (supportsMutation) {
1943-
// Hide the Offscreen component that contains the primary children. TODO:
1944-
// Ideally, this effect would have been scheduled on the Offscreen fiber
1945-
// itself. That's how unhiding works: the Offscreen component schedules an
1946-
// effect on itself. However, in this case, the component didn't complete,
1947-
// so the fiber was never added to the effect list in the normal path. We
1948-
// could have appended it to the effect list in the Suspense component's
1949-
// second pass, but doing it this way is less complicated. This would be
1950-
// simpler if we got rid of the effect list and traversed the tree, like
1951-
// we're planning to do.
1952-
const primaryChildParent: Fiber = (finishedWork.child: any);
1953-
hideOrUnhideAllChildren(primaryChildParent, true);
1954-
}
1955-
}
1956-
19571932
if (enableSuspenseCallback && newState !== null) {
19581933
const suspenseCallback = finishedWork.memoizedProps.suspenseCallback;
19591934
if (typeof suspenseCallback === 'function') {
@@ -2127,6 +2102,10 @@ function commitMutationEffects_complete(root: FiberRoot) {
21272102
}
21282103

21292104
function commitMutationEffectsOnFiber(finishedWork: Fiber, root: FiberRoot) {
2105+
// TODO: The factoring of this phase could probably be improved. Consider
2106+
// switching on the type of work before checking the flags. That's what
2107+
// we do in all the other phases. I think this one is only different
2108+
// because of the shared reconcilation logic below.
21302109
const flags = finishedWork.flags;
21312110

21322111
if (flags & ContentReset) {
@@ -2147,6 +2126,37 @@ function commitMutationEffectsOnFiber(finishedWork: Fiber, root: FiberRoot) {
21472126
}
21482127
}
21492128

2129+
if (flags & Visibility) {
2130+
switch (finishedWork.tag) {
2131+
case SuspenseComponent: {
2132+
const newState: OffscreenState | null = finishedWork.memoizedState;
2133+
if (newState !== null) {
2134+
markCommitTimeOfFallback();
2135+
// Hide the Offscreen component that contains the primary children.
2136+
// TODO: Ideally, this effect would have been scheduled on the
2137+
// Offscreen fiber itself. That's how unhiding works: the Offscreen
2138+
// component schedules an effect on itself. However, in this case, the
2139+
// component didn't complete, so the fiber was never added to the
2140+
// effect list in the normal path. We could have appended it to the
2141+
// effect list in the Suspense component's second pass, but doing it
2142+
// this way is less complicated. This would be simpler if we got rid
2143+
// of the effect list and traversed the tree, like we're planning to
2144+
// do.
2145+
const primaryChildParent: Fiber = (finishedWork.child: any);
2146+
hideOrUnhideAllChildren(primaryChildParent, true);
2147+
}
2148+
break;
2149+
}
2150+
case OffscreenComponent:
2151+
case LegacyHiddenComponent: {
2152+
const newState: OffscreenState | null = finishedWork.memoizedState;
2153+
const isHidden = newState !== null;
2154+
hideOrUnhideAllChildren(finishedWork, isHidden);
2155+
break;
2156+
}
2157+
}
2158+
}
2159+
21502160
// The following switch statement is only concerned about placement,
21512161
// updates, and deletions. To avoid needing to add a case for every possible
21522162
// bitmap value, we remove the secondary effects from the effect tag and

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

+33-20
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,11 @@
99

1010
import type {Fiber} from './ReactInternalTypes';
1111
import type {Lanes, Lane} from './ReactFiberLane.new';
12-
import type {ReactScopeInstance, ReactContext} from 'shared/ReactTypes';
12+
import type {
13+
ReactScopeInstance,
14+
ReactContext,
15+
Wakeable,
16+
} from 'shared/ReactTypes';
1317
import type {FiberRoot} from './ReactInternalTypes';
1418
import type {
1519
Instance,
@@ -60,6 +64,7 @@ import {
6064
Ref,
6165
RefStatic,
6266
Update,
67+
Visibility,
6368
NoFlags,
6469
DidCapture,
6570
Snapshot,
@@ -320,7 +325,7 @@ if (supportsMutation) {
320325
// down its children. Instead, we'll get insertions from each child in
321326
// the portal directly.
322327
} else if (node.tag === SuspenseComponent) {
323-
if ((node.flags & Update) !== NoFlags) {
328+
if ((node.flags & Visibility) !== NoFlags) {
324329
// Need to toggle the visibility of the primary children.
325330
const newIsHidden = node.memoizedState !== null;
326331
if (newIsHidden) {
@@ -405,7 +410,7 @@ if (supportsMutation) {
405410
// down its children. Instead, we'll get insertions from each child in
406411
// the portal directly.
407412
} else if (node.tag === SuspenseComponent) {
408-
if ((node.flags & Update) !== NoFlags) {
413+
if ((node.flags & Visibility) !== NoFlags) {
409414
// Need to toggle the visibility of the primary children.
410415
const newIsHidden = node.memoizedState !== null;
411416
if (newIsHidden) {
@@ -1084,32 +1089,40 @@ function completeWork(
10841089
}
10851090
}
10861091

1087-
if (supportsPersistence) {
1088-
// TODO: Only schedule updates if not prevDidTimeout.
1089-
if (nextDidTimeout) {
1090-
// If this boundary just timed out, schedule an effect to attach a
1091-
// retry listener to the promise. This flag is also used to hide the
1092-
// primary children.
1093-
workInProgress.flags |= Update;
1094-
}
1092+
const wakeables: Set<Wakeable> | null = (workInProgress.updateQueue: any);
1093+
if (wakeables !== null) {
1094+
// Schedule an effect to attach a retry listener to the promise.
1095+
// TODO: Move to passive phase
1096+
workInProgress.flags |= Update;
10951097
}
1098+
10961099
if (supportsMutation) {
1097-
// TODO: Only schedule updates if these values are non equal, i.e. it changed.
1098-
if (nextDidTimeout || prevDidTimeout) {
1099-
// If this boundary just timed out, schedule an effect to attach a
1100-
// retry listener to the promise. This flag is also used to hide the
1101-
// primary children. In mutation mode, we also need the flag to
1102-
// *unhide* children that were previously hidden, so check if this
1103-
// is currently timed out, too.
1104-
workInProgress.flags |= Update;
1100+
if (nextDidTimeout !== prevDidTimeout) {
1101+
// In mutation mode, visibility is toggled by mutating the nearest
1102+
// host nodes whenever they switch from hidden -> visible or vice
1103+
// versa. We don't need to switch when the boundary updates but its
1104+
// visibility hasn't changed.
1105+
workInProgress.flags |= Visibility;
11051106
}
11061107
}
1108+
if (supportsPersistence) {
1109+
if (nextDidTimeout) {
1110+
// In persistent mode, visibility is toggled by cloning the nearest
1111+
// host nodes in the complete phase whenever the boundary is hidden.
1112+
// TODO: The plan is to add a transparent host wrapper (no layout)
1113+
// around the primary children and hide that node. Then we don't need
1114+
// to do the funky cloning business.
1115+
workInProgress.flags |= Visibility;
1116+
}
1117+
}
1118+
11071119
if (
11081120
enableSuspenseCallback &&
11091121
workInProgress.updateQueue !== null &&
11101122
workInProgress.memoizedProps.suspenseCallback != null
11111123
) {
11121124
// Always notify the callback
1125+
// TODO: Move to passive phase
11131126
workInProgress.flags |= Update;
11141127
}
11151128
bubbleProperties(workInProgress);
@@ -1396,7 +1409,7 @@ function completeWork(
13961409
prevIsHidden !== nextIsHidden &&
13971410
newProps.mode !== 'unstable-defer-without-hiding'
13981411
) {
1399-
workInProgress.flags |= Update;
1412+
workInProgress.flags |= Visibility;
14001413
}
14011414
}
14021415

0 commit comments

Comments
 (0)