Clean-up: Move Offscreen logic from Suspense fiber (#21925)
Much of the visibility-toggling logic is shared between the Suspense and Offscreen types, but there is some duplicated code that exists in both. Specifically, when a Suspense fiber's state switches from suspended to resolved, we schedule an effect on the parent Suspense fiber, rather than the inner Offscreen fiber. Then in the commit phase, the Suspense fiber is responsible for committing the visibility effect on Offscreen. There two main reasons we implemented it this way, neither of which apply any more: - The inner Offscreen fiber that wraps around the Suspense children used to be conditionally added only when the boundary was in its fallback state. So when toggling back to visible, there was no inner fiber to handle the visibility effect. This is no longer the case — the primary children are always wrapped in an Offscreen fiber. - When the Suspense fiber is in its fallback state, the inner Offscreen fiber does not have a complete phase, because we bail out of rendering that tree. In the old effects list implementation, that meant the Offscreen fiber did not get added to the effect list, so it didn't have a commit phase. In the new recursive effects implementation, there's no list to maintain. Marking a flag on the inner fiber is sufficient to schedule a commit effect. Given that these are no relevant, I was able to remove a lot of old code and shift more of the logic out of the Suspense implementation and into the Offscreen implementation so that it is shared by both. (Being able to share the implementaiton like this was in fact one of the reasons we stopped conditionally removing the inner Offscreen fiber.) As a bonus, this happens to fix a TODO in the Offscreen implementation for persistent (Fabric) mode, where newly inserted nodes inside an already hidden tree must also be hidden. Though we'll still need to make this work in mutation (DOM) mode, too.
Andrew Clark committed
Jul 20, 2021 at 19:05 UTC
9ab90de602357407fb03a27715b61761a258a8c4
5 files changed
+106
-226
packages/react-reconciler/src/ReactFiberCommitWork.new.js
+3
-25
@@ -2121,34 +2121,12 @@ function commitMutationEffectsOnFiber(finishedWork: Fiber, root: FiberRoot) {
2121
case SuspenseComponent: {
2122
const newState: OffscreenState | null = finishedWork.memoizedState;
2123
const isHidden = newState !== null;
2124
- const current = finishedWork.alternate;
2125
- const wasHidden = current !== null && current.memoizedState !== null;
2126
- const offscreenBoundary: Fiber = (finishedWork.child: any);
2127
-
2124
if (isHidden) {
2125
+ const current = finishedWork.alternate;
2126
+ const wasHidden = current !== null && current.memoizedState !== null;
2127
if (!wasHidden) {
2128
+ // TODO: Move to passive phase
2129
markCommitTimeOfFallback();
2131
- if (supportsMutation) {
2132
- hideOrUnhideAllChildren(offscreenBoundary, true);
2133
- }
2134
- if (
2135
- enableSuspenseLayoutEffectSemantics &&
2136
- (offscreenBoundary.mode & ConcurrentMode) !== NoMode
2137
- ) {
2138
- let offscreenChild = offscreenBoundary.child;
2139
- while (offscreenChild !== null) {
2140
- nextEffect = offscreenChild;
2141
- disappearLayoutEffects_begin(offscreenChild);
2142
- offscreenChild = offscreenChild.sibling;
2143
- }
2144
- }
2145
- }
2146
- } else {
2147
- if (wasHidden) {
2148
- if (supportsMutation) {
2149
- hideOrUnhideAllChildren(offscreenBoundary, false);
2150
- }
2151
- // TODO: Move re-appear call here for symmetry?
2130
}
2131
}
2132
break;
packages/react-reconciler/src/ReactFiberCommitWork.old.js
+3
-25
@@ -2121,34 +2121,12 @@ function commitMutationEffectsOnFiber(finishedWork: Fiber, root: FiberRoot) {
2121
case SuspenseComponent: {
2122
const newState: OffscreenState | null = finishedWork.memoizedState;
2123
const isHidden = newState !== null;
2124
- const current = finishedWork.alternate;
2125
- const wasHidden = current !== null && current.memoizedState !== null;
2126
- const offscreenBoundary: Fiber = (finishedWork.child: any);
2127
-
2124
if (isHidden) {
2125
+ const current = finishedWork.alternate;
2126
+ const wasHidden = current !== null && current.memoizedState !== null;
2127
if (!wasHidden) {
2128
+ // TODO: Move to passive phase
2129
markCommitTimeOfFallback();
2131
- if (supportsMutation) {
2132
- hideOrUnhideAllChildren(offscreenBoundary, true);
2133
- }
2134
- if (
2135
- enableSuspenseLayoutEffectSemantics &&
2136
- (offscreenBoundary.mode & ConcurrentMode) !== NoMode
2137
- ) {
2138
- let offscreenChild = offscreenBoundary.child;
2139
- while (offscreenChild !== null) {
2140
- nextEffect = offscreenChild;
2141
- disappearLayoutEffects_begin(offscreenChild);
2142
- offscreenChild = offscreenChild.sibling;
2143
- }
2144
- }
2145
- }
2146
- } else {
2147
- if (wasHidden) {
2148
- if (supportsMutation) {
2149
- hideOrUnhideAllChildren(offscreenBoundary, false);
2150
- }
2151
- // TODO: Move re-appear call here for symmetry?
2130
}
2131
}
2132
break;
packages/react-reconciler/src/ReactFiberCompleteWork.new.js
+34
-80
@@ -324,37 +324,17 @@ if (supportsMutation) {
324
// If we have a portal child, then we don't want to traverse
325
// down its children. Instead, we'll get insertions from each child in
326
// the portal directly.
327
- } else if (node.tag === SuspenseComponent) {
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) {
332
- const primaryChildParent = node.child;
333
- if (primaryChildParent !== null) {
334
- if (primaryChildParent.child !== null) {
335
- primaryChildParent.child.return = primaryChildParent;
336
- appendAllChildren(
337
- parent,
338
- primaryChildParent,
339
- true,
340
- newIsHidden,
341
- );
342
- }
343
- const fallbackChildParent = primaryChildParent.sibling;
344
- if (fallbackChildParent !== null) {
345
- fallbackChildParent.return = node;
346
- node = fallbackChildParent;
347
- continue;
348
- }
349
- }
350
- }
351
- }
352
- if (node.child !== null) {
353
- // Continue traversing like normal
354
- node.child.return = node;
355
- node = node.child;
356
- continue;
327
+ } else if (
328
+ node.tag === OffscreenComponent &&
329
+ node.memoizedState !== null
330
+ ) {
331
+ // The children in this boundary are hidden. Toggle their visibility
332
+ // before appending.
333
+ const child = node.child;
334
+ if (child !== null) {
335
+ child.return = node;
336
}
337
+ appendAllChildren(parent, node, true, true);
338
} else if (node.child !== null) {
339
node.child.return = node;
340
node = node.child;
@@ -409,37 +389,17 @@ if (supportsMutation) {
389
// If we have a portal child, then we don't want to traverse
390
// down its children. Instead, we'll get insertions from each child in
391
// the portal directly.
412
- } else if (node.tag === SuspenseComponent) {
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) {
417
- const primaryChildParent = node.child;
418
- if (primaryChildParent !== null) {
419
- if (primaryChildParent.child !== null) {
420
- primaryChildParent.child.return = primaryChildParent;
421
- appendAllChildrenToContainer(
422
- containerChildSet,
423
- primaryChildParent,
424
- true,
425
- newIsHidden,
426
- );
427
- }
428
- const fallbackChildParent = primaryChildParent.sibling;
429
- if (fallbackChildParent !== null) {
430
- fallbackChildParent.return = node;
431
- node = fallbackChildParent;
432
- continue;
433
- }
434
- }
435
- }
436
- }
437
- if (node.child !== null) {
438
- // Continue traversing like normal
439
- node.child.return = node;
440
- node = node.child;
441
- continue;
392
+ } else if (
393
+ node.tag === OffscreenComponent &&
394
+ node.memoizedState !== null
395
+ ) {
396
+ // The children in this boundary are hidden. Toggle their visibility
397
+ // before appending.
398
+ const child = node.child;
399
+ if (child !== null) {
400
+ child.return = node;
401
}
402
+ appendAllChildrenToContainer(containerChildSet, node, true, true);
403
} else if (node.child !== null) {
404
node.child.return = node;
405
node = node.child;
@@ -1056,7 +1016,21 @@ function completeWork(
1016
prevDidTimeout = prevState !== null;
1017
}
1018
1019
+ // If the suspended state of the boundary changes, we need to schedule
1020
+ // an effect to toggle the subtree's visibility. When we switch from
1021
+ // fallback -> primary, the inner Offscreen fiber schedules this effect
1022
+ // as part of its normal complete phase. But when we switch from
1023
+ // primary -> fallback, the inner Offscreen fiber does not have a complete
1024
+ // phase. So we need to schedule its effect here.
1025
+ //
1026
+ // We also use this flag to connect/disconnect the effects, but the same
1027
+ // logic applies: when re-connecting, the Offscreen fiber's complete
1028
+ // phase will handle scheduling the effect. It's only when the fallback
1029
+ // is active that we have to do anything special.
1030
if (nextDidTimeout && !prevDidTimeout) {
1031
+ const offscreenFiber: Fiber = (workInProgress.child: any);
1032
+ offscreenFiber.flags |= Visibility;
1033
+
1034
// TODO: This will still suspend a synchronous tree if anything
1035
// in the concurrent tree already suspended during this render.
1036
// This is a known bug.
@@ -1096,26 +1070,6 @@ function completeWork(
1070
workInProgress.flags |= Update;
1071
}
1072
1099
- if (supportsMutation) {
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
-
1073
if (
1074
enableSuspenseCallback &&
1075
workInProgress.updateQueue !== null &&
packages/react-reconciler/src/ReactFiberCompleteWork.old.js
+34
-80
@@ -324,37 +324,17 @@ if (supportsMutation) {
324
// If we have a portal child, then we don't want to traverse
325
// down its children. Instead, we'll get insertions from each child in
326
// the portal directly.
327
- } else if (node.tag === SuspenseComponent) {
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) {
332
- const primaryChildParent = node.child;
333
- if (primaryChildParent !== null) {
334
- if (primaryChildParent.child !== null) {
335
- primaryChildParent.child.return = primaryChildParent;
336
- appendAllChildren(
337
- parent,
338
- primaryChildParent,
339
- true,
340
- newIsHidden,
341
- );
342
- }
343
- const fallbackChildParent = primaryChildParent.sibling;
344
- if (fallbackChildParent !== null) {
345
- fallbackChildParent.return = node;
346
- node = fallbackChildParent;
347
- continue;
348
- }
349
- }
350
- }
351
- }
352
- if (node.child !== null) {
353
- // Continue traversing like normal
354
- node.child.return = node;
355
- node = node.child;
356
- continue;
327
+ } else if (
328
+ node.tag === OffscreenComponent &&
329
+ node.memoizedState !== null
330
+ ) {
331
+ // The children in this boundary are hidden. Toggle their visibility
332
+ // before appending.
333
+ const child = node.child;
334
+ if (child !== null) {
335
+ child.return = node;
336
}
337
+ appendAllChildren(parent, node, true, true);
338
} else if (node.child !== null) {
339
node.child.return = node;
340
node = node.child;
@@ -409,37 +389,17 @@ if (supportsMutation) {
389
// If we have a portal child, then we don't want to traverse
390
// down its children. Instead, we'll get insertions from each child in
391
// the portal directly.
412
- } else if (node.tag === SuspenseComponent) {
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) {
417
- const primaryChildParent = node.child;
418
- if (primaryChildParent !== null) {
419
- if (primaryChildParent.child !== null) {
420
- primaryChildParent.child.return = primaryChildParent;
421
- appendAllChildrenToContainer(
422
- containerChildSet,
423
- primaryChildParent,
424
- true,
425
- newIsHidden,
426
- );
427
- }
428
- const fallbackChildParent = primaryChildParent.sibling;
429
- if (fallbackChildParent !== null) {
430
- fallbackChildParent.return = node;
431
- node = fallbackChildParent;
432
- continue;
433
- }
434
- }
435
- }
436
- }
437
- if (node.child !== null) {
438
- // Continue traversing like normal
439
- node.child.return = node;
440
- node = node.child;
441
- continue;
392
+ } else if (
393
+ node.tag === OffscreenComponent &&
394
+ node.memoizedState !== null
395
+ ) {
396
+ // The children in this boundary are hidden. Toggle their visibility
397
+ // before appending.
398
+ const child = node.child;
399
+ if (child !== null) {
400
+ child.return = node;
401
}
402
+ appendAllChildrenToContainer(containerChildSet, node, true, true);
403
} else if (node.child !== null) {
404
node.child.return = node;
405
node = node.child;
@@ -1056,7 +1016,21 @@ function completeWork(
1016
prevDidTimeout = prevState !== null;
1017
}
1018
1019
+ // If the suspended state of the boundary changes, we need to schedule
1020
+ // an effect to toggle the subtree's visibility. When we switch from
1021
+ // fallback -> primary, the inner Offscreen fiber schedules this effect
1022
+ // as part of its normal complete phase. But when we switch from
1023
+ // primary -> fallback, the inner Offscreen fiber does not have a complete
1024
+ // phase. So we need to schedule its effect here.
1025
+ //
1026
+ // We also use this flag to connect/disconnect the effects, but the same
1027
+ // logic applies: when re-connecting, the Offscreen fiber's complete
1028
+ // phase will handle scheduling the effect. It's only when the fallback
1029
+ // is active that we have to do anything special.
1030
if (nextDidTimeout && !prevDidTimeout) {
1031
+ const offscreenFiber: Fiber = (workInProgress.child: any);
1032
+ offscreenFiber.flags |= Visibility;
1033
+
1034
// TODO: This will still suspend a synchronous tree if anything
1035
// in the concurrent tree already suspended during this render.
1036
// This is a known bug.
@@ -1096,26 +1070,6 @@ function completeWork(
1070
workInProgress.flags |= Update;
1071
}
1072
1099
- if (supportsMutation) {
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
-
1073
if (
1074
enableSuspenseCallback &&
1075
workInProgress.updateQueue !== null &&
packages/react-reconciler/src/__tests__/ReactOffscreen-test.js
+32
-16
@@ -201,10 +201,14 @@ describe('ReactOffscreen', () => {
201
});
202
// No layout effect.
203
expect(Scheduler).toHaveYielded(['Child']);
204
- // TODO: Offscreen does not yet hide/unhide children correctly. Until we do,
205
- // it should only be used inside a host component wrapper whose visibility
206
- // is toggled simultaneously.
207
- expect(root).toMatchRenderedOutput(<span prop="Child" />);
204
+ if (gate(flags => flags.persistent)) {
205
+ expect(root).toMatchRenderedOutput(<span hidden={true} prop="Child" />);
206
+ } else {
207
+ // TODO: Offscreen does not yet hide/unhide children correctly in mutation
208
+ // mode. Until we do, it should only be used inside a host component
209
+ // wrapper whose visibility is toggled simultaneously.
210
+ expect(root).toMatchRenderedOutput(<span prop="Child" />);
211
+ }
212
213
// Unhide the tree. The layout effect is mounted.
214
await act(async () => {
@@ -251,10 +255,14 @@ describe('ReactOffscreen', () => {
255
);
256
});
257
expect(Scheduler).toHaveYielded(['Unmount layout', 'Child']);
254
- // TODO: Offscreen does not yet hide/unhide children correctly. Until we do,
255
- // it should only be used inside a host component wrapper whose visibility
256
- // is toggled simultaneously.
257
- expect(root).toMatchRenderedOutput(<span prop="Child" />);
258
+ if (gate(flags => flags.persistent)) {
259
+ expect(root).toMatchRenderedOutput(<span hidden={true} prop="Child" />);
260
+ } else {
261
+ // TODO: Offscreen does not yet hide/unhide children correctly in mutation
262
+ // mode. Until we do, it should only be used inside a host component
263
+ // wrapper whose visibility is toggled simultaneously.
264
+ expect(root).toMatchRenderedOutput(<span prop="Child" />);
265
+ }
266
267
// Unhide the tree. The layout effect is re-mounted.
268
await act(async () => {
@@ -291,10 +299,14 @@ describe('ReactOffscreen', () => {
299
);
300
});
301
expect(Scheduler).toHaveYielded(['Child']);
294
- // TODO: Offscreen does not yet hide/unhide children correctly. Until we do,
295
- // it should only be used inside a host component wrapper whose visibility
296
- // is toggled simultaneously.
297
- expect(root).toMatchRenderedOutput(<span prop="Child" />);
302
+ if (gate(flags => flags.persistent)) {
303
+ expect(root).toMatchRenderedOutput(<span hidden={true} prop="Child" />);
304
+ } else {
305
+ // TODO: Offscreen does not yet hide/unhide children correctly in mutation
306
+ // mode. Until we do, it should only be used inside a host component
307
+ // wrapper whose visibility is toggled simultaneously.
308
+ expect(root).toMatchRenderedOutput(<span prop="Child" />);
309
+ }
310
311
// Show the tree. The layout effect is mounted.
312
await act(async () => {
@@ -316,9 +328,13 @@ describe('ReactOffscreen', () => {
328
);
329
});
330
expect(Scheduler).toHaveYielded(['Unmount layout', 'Child']);
319
- // TODO: Offscreen does not yet hide/unhide children correctly. Until we do,
320
- // it should only be used inside a host component wrapper whose visibility
321
- // is toggled simultaneously.
322
- expect(root).toMatchRenderedOutput(<span prop="Child" />);
331
+ if (gate(flags => flags.persistent)) {
332
+ expect(root).toMatchRenderedOutput(<span hidden={true} prop="Child" />);
333
+ } else {
334
+ // TODO: Offscreen does not yet hide/unhide children correctly in mutation
335
+ // mode. Until we do, it should only be used inside a host component
336
+ // wrapper whose visibility is toggled simultaneously.
337
+ expect(root).toMatchRenderedOutput(<span prop="Child" />);
338
+ }
339
});
340
});