[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>
Andrew Clark committed
Jul 14, 2021 at 13:37 UTC
a97b5ac078499e33bbcf937935ab7139a317bac4
6 files changed
+223
-103
packages/react-reconciler/src/ReactFiberCommitWork.new.js
+41
-31
@@ -66,7 +66,6 @@ import {
66
ChildDeletion,
67
Snapshot,
68
Update,
69
- Callback,
69
Ref,
70
Hydrating,
71
HydratingAndUpdate,
@@ -75,6 +74,7 @@ import {
74
MutationMask,
75
LayoutMask,
76
PassiveMask,
77
+ Visibility,
78
} from './ReactFiberFlags';
79
import getComponentNameFromFiber from 'react-reconciler/src/getComponentNameFromFiber';
80
import invariant from 'shared/invariant';
@@ -615,7 +615,7 @@ function commitLayoutEffectOnFiber(
615
finishedWork: Fiber,
616
committedLanes: Lanes,
617
): void {
618
- if ((finishedWork.flags & (Update | Callback)) !== NoFlags) {
618
+ if ((finishedWork.flags & LayoutMask) !== NoFlags) {
619
switch (finishedWork.tag) {
620
case FunctionComponent:
621
case ForwardRef:
@@ -1776,7 +1776,7 @@ function commitWork(current: Fiber | null, finishedWork: Fiber): void {
1776
return;
1777
}
1778
case SuspenseComponent: {
1779
- commitSuspenseComponent(finishedWork);
1779
+ commitSuspenseCallback(finishedWork);
1780
attachSuspenseRetryListeners(finishedWork);
1781
return;
1782
}
@@ -1899,7 +1899,7 @@ function commitWork(current: Fiber | null, finishedWork: Fiber): void {
1899
return;
1900
}
1901
case SuspenseComponent: {
1902
- commitSuspenseComponent(finishedWork);
1902
+ commitSuspenseCallback(finishedWork);
1903
attachSuspenseRetryListeners(finishedWork);
1904
return;
1905
}
@@ -1918,13 +1918,6 @@ function commitWork(current: Fiber | null, finishedWork: Fiber): void {
1918
}
1919
break;
1920
}
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
- }
1921
}
1922
invariant(
1923
false,
@@ -1933,27 +1926,9 @@ function commitWork(current: Fiber | null, finishedWork: Fiber): void {
1926
);
1927
}
1928
1936
-function commitSuspenseComponent(finishedWork: Fiber) {
1929
+function commitSuspenseCallback(finishedWork: Fiber) {
1930
+ // TODO: Move this to passive phase
1931
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
-
1932
if (enableSuspenseCallback && newState !== null) {
1933
const suspenseCallback = finishedWork.memoizedProps.suspenseCallback;
1934
if (typeof suspenseCallback === 'function') {
@@ -2127,6 +2102,10 @@ function commitMutationEffects_complete(root: FiberRoot) {
2102
}
2103
2104
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.
2109
const flags = finishedWork.flags;
2110
2111
if (flags & ContentReset) {
@@ -2147,6 +2126,37 @@ function commitMutationEffectsOnFiber(finishedWork: Fiber, root: FiberRoot) {
2126
}
2127
}
2128
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
+
2160
// The following switch statement is only concerned about placement,
2161
// updates, and deletions. To avoid needing to add a case for every possible
2162
// bitmap value, we remove the secondary effects from the effect tag and
packages/react-reconciler/src/ReactFiberCommitWork.old.js
+41
-31
@@ -66,7 +66,6 @@ import {
66
ChildDeletion,
67
Snapshot,
68
Update,
69
- Callback,
69
Ref,
70
Hydrating,
71
HydratingAndUpdate,
@@ -75,6 +74,7 @@ import {
74
MutationMask,
75
LayoutMask,
76
PassiveMask,
77
+ Visibility,
78
} from './ReactFiberFlags';
79
import getComponentNameFromFiber from 'react-reconciler/src/getComponentNameFromFiber';
80
import invariant from 'shared/invariant';
@@ -615,7 +615,7 @@ function commitLayoutEffectOnFiber(
615
finishedWork: Fiber,
616
committedLanes: Lanes,
617
): void {
618
- if ((finishedWork.flags & (Update | Callback)) !== NoFlags) {
618
+ if ((finishedWork.flags & LayoutMask) !== NoFlags) {
619
switch (finishedWork.tag) {
620
case FunctionComponent:
621
case ForwardRef:
@@ -1776,7 +1776,7 @@ function commitWork(current: Fiber | null, finishedWork: Fiber): void {
1776
return;
1777
}
1778
case SuspenseComponent: {
1779
- commitSuspenseComponent(finishedWork);
1779
+ commitSuspenseCallback(finishedWork);
1780
attachSuspenseRetryListeners(finishedWork);
1781
return;
1782
}
@@ -1899,7 +1899,7 @@ function commitWork(current: Fiber | null, finishedWork: Fiber): void {
1899
return;
1900
}
1901
case SuspenseComponent: {
1902
- commitSuspenseComponent(finishedWork);
1902
+ commitSuspenseCallback(finishedWork);
1903
attachSuspenseRetryListeners(finishedWork);
1904
return;
1905
}
@@ -1918,13 +1918,6 @@ function commitWork(current: Fiber | null, finishedWork: Fiber): void {
1918
}
1919
break;
1920
}
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
- }
1921
}
1922
invariant(
1923
false,
@@ -1933,27 +1926,9 @@ function commitWork(current: Fiber | null, finishedWork: Fiber): void {
1926
);
1927
}
1928
1936
-function commitSuspenseComponent(finishedWork: Fiber) {
1929
+function commitSuspenseCallback(finishedWork: Fiber) {
1930
+ // TODO: Move this to passive phase
1931
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
-
1932
if (enableSuspenseCallback && newState !== null) {
1933
const suspenseCallback = finishedWork.memoizedProps.suspenseCallback;
1934
if (typeof suspenseCallback === 'function') {
@@ -2127,6 +2102,10 @@ function commitMutationEffects_complete(root: FiberRoot) {
2102
}
2103
2104
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.
2109
const flags = finishedWork.flags;
2110
2111
if (flags & ContentReset) {
@@ -2147,6 +2126,37 @@ function commitMutationEffectsOnFiber(finishedWork: Fiber, root: FiberRoot) {
2126
}
2127
}
2128
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
+
2160
// The following switch statement is only concerned about placement,
2161
// updates, and deletions. To avoid needing to add a case for every possible
2162
// bitmap value, we remove the secondary effects from the effect tag and
packages/react-reconciler/src/ReactFiberCompleteWork.new.js
+33
-20
@@ -9,7 +9,11 @@
9
10
import type {Fiber} from './ReactInternalTypes';
11
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';
17
import type {FiberRoot} from './ReactInternalTypes';
18
import type {
19
Instance,
@@ -60,6 +64,7 @@ import {
64
Ref,
65
RefStatic,
66
Update,
67
+ Visibility,
68
NoFlags,
69
DidCapture,
70
Snapshot,
@@ -320,7 +325,7 @@ if (supportsMutation) {
325
// down its children. Instead, we'll get insertions from each child in
326
// the portal directly.
327
} else if (node.tag === SuspenseComponent) {
323
- if ((node.flags & Update) !== NoFlags) {
328
+ if ((node.flags & Visibility) !== NoFlags) {
329
// Need to toggle the visibility of the primary children.
330
const newIsHidden = node.memoizedState !== null;
331
if (newIsHidden) {
@@ -405,7 +410,7 @@ if (supportsMutation) {
410
// down its children. Instead, we'll get insertions from each child in
411
// the portal directly.
412
} else if (node.tag === SuspenseComponent) {
408
- if ((node.flags & Update) !== NoFlags) {
413
+ if ((node.flags & Visibility) !== NoFlags) {
414
// Need to toggle the visibility of the primary children.
415
const newIsHidden = node.memoizedState !== null;
416
if (newIsHidden) {
@@ -1084,32 +1089,40 @@ function completeWork(
1089
}
1090
}
1091
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;
1097
}
1098
+
1099
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;
1106
}
1107
}
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
+
1119
if (
1120
enableSuspenseCallback &&
1121
workInProgress.updateQueue !== null &&
1122
workInProgress.memoizedProps.suspenseCallback != null
1123
) {
1124
// Always notify the callback
1125
+ // TODO: Move to passive phase
1126
workInProgress.flags |= Update;
1127
}
1128
bubbleProperties(workInProgress);
@@ -1396,7 +1409,7 @@ function completeWork(
1409
prevIsHidden !== nextIsHidden &&
1410
newProps.mode !== 'unstable-defer-without-hiding'
1411
) {
1399
- workInProgress.flags |= Update;
1412
+ workInProgress.flags |= Visibility;
1413
}
1414
}
1415
packages/react-reconciler/src/ReactFiberCompleteWork.old.js
+33
-20
@@ -9,7 +9,11 @@
9
10
import type {Fiber} from './ReactInternalTypes';
11
import type {Lanes, Lane} from './ReactFiberLane.old';
12
-import type {ReactScopeInstance, ReactContext} from 'shared/ReactTypes';
12
+import type {
13
+ ReactScopeInstance,
14
+ ReactContext,
15
+ Wakeable,
16
+} from 'shared/ReactTypes';
17
import type {FiberRoot} from './ReactInternalTypes';
18
import type {
19
Instance,
@@ -60,6 +64,7 @@ import {
64
Ref,
65
RefStatic,
66
Update,
67
+ Visibility,
68
NoFlags,
69
DidCapture,
70
Snapshot,
@@ -320,7 +325,7 @@ if (supportsMutation) {
325
// down its children. Instead, we'll get insertions from each child in
326
// the portal directly.
327
} else if (node.tag === SuspenseComponent) {
323
- if ((node.flags & Update) !== NoFlags) {
328
+ if ((node.flags & Visibility) !== NoFlags) {
329
// Need to toggle the visibility of the primary children.
330
const newIsHidden = node.memoizedState !== null;
331
if (newIsHidden) {
@@ -405,7 +410,7 @@ if (supportsMutation) {
410
// down its children. Instead, we'll get insertions from each child in
411
// the portal directly.
412
} else if (node.tag === SuspenseComponent) {
408
- if ((node.flags & Update) !== NoFlags) {
413
+ if ((node.flags & Visibility) !== NoFlags) {
414
// Need to toggle the visibility of the primary children.
415
const newIsHidden = node.memoizedState !== null;
416
if (newIsHidden) {
@@ -1084,32 +1089,40 @@ function completeWork(
1089
}
1090
}
1091
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;
1097
}
1098
+
1099
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;
1106
}
1107
}
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
+
1119
if (
1120
enableSuspenseCallback &&
1121
workInProgress.updateQueue !== null &&
1122
workInProgress.memoizedProps.suspenseCallback != null
1123
) {
1124
// Always notify the callback
1125
+ // TODO: Move to passive phase
1126
workInProgress.flags |= Update;
1127
}
1128
bubbleProperties(workInProgress);
@@ -1396,7 +1409,7 @@ function completeWork(
1409
prevIsHidden !== nextIsHidden &&
1410
newProps.mode !== 'unstable-defer-without-hiding'
1411
) {
1399
- workInProgress.flags |= Update;
1412
+ workInProgress.flags |= Visibility;
1413
}
1414
}
1415
packages/react-reconciler/src/ReactFiberFlags.js
+1
-1
@@ -82,7 +82,7 @@ export const MutationMask =
82
Ref |
83
Hydrating |
84
Visibility;
85
-export const LayoutMask = Update | Callback | Ref;
85
+export const LayoutMask = Update | Callback | Ref | Visibility;
86
87
// TODO: Split into PassiveMountMask and PassiveUnmountMask
88
export const PassiveMask = Passive | ChildDeletion;
packages/react-reconciler/src/__tests__/ReactSuspenseEffectsSemanticsDOM-test.js
new
+74
@@ -0,0 +1,74 @@
1
+/**
2
+ * Copyright (c) Facebook, Inc. and its 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 ReactDOM;
14
+let act;
15
+
16
+describe('ReactSuspenseEffectsSemanticsDOM', () => {
17
+ beforeEach(() => {
18
+ jest.resetModules();
19
+
20
+ React = require('react');
21
+ ReactDOM = require('react-dom');
22
+ act = require('jest-react').act;
23
+ });
24
+
25
+ it('should not cause a cycle when combined with a render phase update', () => {
26
+ let scheduleSuspendingUpdate;
27
+
28
+ function App() {
29
+ const [value, setValue] = React.useState(true);
30
+
31
+ scheduleSuspendingUpdate = () => setValue(!value);
32
+
33
+ return (
34
+ <>
35
+ <React.Suspense fallback="Loading...">
36
+ <ComponentThatCausesBug value={value} />
37
+ <ComponentThatSuspendsOnUpdate shouldSuspend={!value} />
38
+ </React.Suspense>
39
+ </>
40
+ );
41
+ }
42
+
43
+ function ComponentThatCausesBug({value}) {
44
+ const [mirroredValue, setMirroredValue] = React.useState(value);
45
+ if (mirroredValue !== value) {
46
+ setMirroredValue(value);
47
+ }
48
+
49
+ // eslint-disable-next-line no-unused-vars
50
+ const [_, setRef] = React.useState(null);
51
+
52
+ return <div ref={setRef} />;
53
+ }
54
+
55
+ const promise = Promise.resolve();
56
+
57
+ function ComponentThatSuspendsOnUpdate({shouldSuspend}) {
58
+ if (shouldSuspend) {
59
+ // Fake Suspend
60
+ throw promise;
61
+ }
62
+ return null;
63
+ }
64
+
65
+ act(() => {
66
+ const root = ReactDOM.createRoot(document.createElement('div'));
67
+ root.render(<App />);
68
+ });
69
+
70
+ act(() => {
71
+ scheduleSuspendingUpdate();
72
+ });
73
+ });
74
+});