Fix: Hide new/updated nodes in already hidden tree (#21929)
When a new node is added to an already hidden Offscreen tree, we need to schedule a visibility effect to hide it. Previously we would only hide when the boundary initially switches from visible to hidden, which meant that newly inserted nodes would be visible. We need to do the same thing for nodes that are updated, because the update might affect the DOM node's `style.display` property. The implementation is to check the `subtreeFlags` for an Insertion or Update effect. This only affects Offscreen, not Suspense, because Suspense boundaries cannot be updated while in their fallback (hidden) state. And it only affects mutation mode, because in persistent mode we implement hiding by cloning the host tree during the complete phase, which already happens on every update.
Andrew Clark committed
Jul 26, 2021 at 11:18 UTC
419cc9c3799bc296c3c2a2c93880826aca680886
3 files changed
+120
-44
packages/react-reconciler/src/ReactFiberCompleteWork.new.js
+20
-6
@@ -63,6 +63,7 @@ import {NoMode, ConcurrentMode, ProfileMode} from './ReactTypeOfMode';
63
import {
64
Ref,
65
RefStatic,
66
+ Placement,
67
Update,
68
Visibility,
69
NoFlags,
@@ -1369,13 +1370,26 @@ function completeWork(
1370
}
1371
}
1372
1372
- // Don't bubble properties for hidden children.
1373
- if (
1374
- !nextIsHidden ||
1375
- includesSomeLane(subtreeRenderLanes, (OffscreenLane: Lane)) ||
1376
- (workInProgress.mode & ConcurrentMode) === NoMode
1377
- ) {
1373
+ if (!nextIsHidden || (workInProgress.mode & ConcurrentMode) === NoMode) {
1374
bubbleProperties(workInProgress);
1375
+ } else {
1376
+ // Don't bubble properties for hidden children unless we're rendering
1377
+ // at offscreen priority.
1378
+ if (includesSomeLane(subtreeRenderLanes, (OffscreenLane: Lane))) {
1379
+ bubbleProperties(workInProgress);
1380
+ if (supportsMutation) {
1381
+ // Check if there was an insertion or update in the hidden subtree.
1382
+ // If so, we need to hide those nodes in the commit phase, so
1383
+ // schedule a visibility effect.
1384
+ if (
1385
+ workInProgress.tag !== LegacyHiddenComponent &&
1386
+ workInProgress.subtreeFlags & (Placement | Update) &&
1387
+ newProps.mode !== 'unstable-defer-without-hiding'
1388
+ ) {
1389
+ workInProgress.flags |= Visibility;
1390
+ }
1391
+ }
1392
+ }
1393
}
1394
1395
if (enableCache) {
packages/react-reconciler/src/ReactFiberCompleteWork.old.js
+20
-6
@@ -63,6 +63,7 @@ import {NoMode, ConcurrentMode, ProfileMode} from './ReactTypeOfMode';
63
import {
64
Ref,
65
RefStatic,
66
+ Placement,
67
Update,
68
Visibility,
69
NoFlags,
@@ -1369,13 +1370,26 @@ function completeWork(
1370
}
1371
}
1372
1372
- // Don't bubble properties for hidden children.
1373
- if (
1374
- !nextIsHidden ||
1375
- includesSomeLane(subtreeRenderLanes, (OffscreenLane: Lane)) ||
1376
- (workInProgress.mode & ConcurrentMode) === NoMode
1377
- ) {
1373
+ if (!nextIsHidden || (workInProgress.mode & ConcurrentMode) === NoMode) {
1374
bubbleProperties(workInProgress);
1375
+ } else {
1376
+ // Don't bubble properties for hidden children unless we're rendering
1377
+ // at offscreen priority.
1378
+ if (includesSomeLane(subtreeRenderLanes, (OffscreenLane: Lane))) {
1379
+ bubbleProperties(workInProgress);
1380
+ if (supportsMutation) {
1381
+ // Check if there was an insertion or update in the hidden subtree.
1382
+ // If so, we need to hide those nodes in the commit phase, so
1383
+ // schedule a visibility effect.
1384
+ if (
1385
+ workInProgress.tag !== LegacyHiddenComponent &&
1386
+ workInProgress.subtreeFlags & (Placement | Update) &&
1387
+ newProps.mode !== 'unstable-defer-without-hiding'
1388
+ ) {
1389
+ workInProgress.flags |= Visibility;
1390
+ }
1391
+ }
1392
+ }
1393
}
1394
1395
if (enableCache) {
packages/react-reconciler/src/__tests__/ReactOffscreen-test.js
+80
-32
@@ -201,14 +201,7 @@ describe('ReactOffscreen', () => {
201
});
202
// No layout effect.
203
expect(Scheduler).toHaveYielded(['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
- }
204
+ expect(root).toMatchRenderedOutput(<span hidden={true} prop="Child" />);
205
206
// Unhide the tree. The layout effect is mounted.
207
await act(async () => {
@@ -255,14 +248,7 @@ describe('ReactOffscreen', () => {
248
);
249
});
250
expect(Scheduler).toHaveYielded(['Unmount layout', '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
- }
251
+ expect(root).toMatchRenderedOutput(<span hidden={true} prop="Child" />);
252
253
// Unhide the tree. The layout effect is re-mounted.
254
await act(async () => {
@@ -299,14 +285,7 @@ describe('ReactOffscreen', () => {
285
);
286
});
287
expect(Scheduler).toHaveYielded(['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
- }
288
+ expect(root).toMatchRenderedOutput(<span hidden={true} prop="Child" />);
289
290
// Show the tree. The layout effect is mounted.
291
await act(async () => {
@@ -328,14 +307,7 @@ describe('ReactOffscreen', () => {
307
);
308
});
309
expect(Scheduler).toHaveYielded(['Unmount layout', '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
- }
310
+ expect(root).toMatchRenderedOutput(<span hidden={true} prop="Child" />);
311
});
312
313
// @gate experimental || www
@@ -385,4 +357,80 @@ describe('ReactOffscreen', () => {
357
});
358
expect(Scheduler).toHaveYielded(['Unmount layout']);
359
});
360
+
361
+ // @gate experimental || www
362
+ it('hides new insertions into an already hidden tree', async () => {
363
+ const root = ReactNoop.createRoot();
364
+ await act(async () => {
365
+ root.render(
366
+ <Offscreen mode="hidden">
367
+ <span>Hi</span>
368
+ </Offscreen>,
369
+ );
370
+ });
371
+ expect(root).toMatchRenderedOutput(<span hidden={true}>Hi</span>);
372
+
373
+ // Insert a new node into the hidden tree
374
+ await act(async () => {
375
+ root.render(
376
+ <Offscreen mode="hidden">
377
+ <span>Hi</span>
378
+ <span>Something new</span>
379
+ </Offscreen>,
380
+ );
381
+ });
382
+ expect(root).toMatchRenderedOutput(
383
+ <>
384
+ <span hidden={true}>Hi</span>
385
+ {/* This new node should also be hidden */}
386
+ <span hidden={true}>Something new</span>
387
+ </>,
388
+ );
389
+ });
390
+
391
+ // @gate experimental || www
392
+ it('hides updated nodes inside an already hidden tree', async () => {
393
+ const root = ReactNoop.createRoot();
394
+ await act(async () => {
395
+ root.render(
396
+ <Offscreen mode="hidden">
397
+ <span>Hi</span>
398
+ </Offscreen>,
399
+ );
400
+ });
401
+ expect(root).toMatchRenderedOutput(<span hidden={true}>Hi</span>);
402
+
403
+ // Set the `hidden` prop to on an already hidden node
404
+ await act(async () => {
405
+ root.render(
406
+ <Offscreen mode="hidden">
407
+ <span hidden={false}>Hi</span>
408
+ </Offscreen>,
409
+ );
410
+ });
411
+ // It should still be hidden, because the Offscreen container overrides it
412
+ expect(root).toMatchRenderedOutput(<span hidden={true}>Hi</span>);
413
+
414
+ // Unhide the boundary
415
+ await act(async () => {
416
+ root.render(
417
+ <Offscreen mode="visible">
418
+ <span hidden={true}>Hi</span>
419
+ </Offscreen>,
420
+ );
421
+ });
422
+ // It should still be hidden, because of the prop
423
+ expect(root).toMatchRenderedOutput(<span hidden={true}>Hi</span>);
424
+
425
+ // Remove the `hidden` prop
426
+ await act(async () => {
427
+ root.render(
428
+ <Offscreen mode="visible">
429
+ <span>Hi</span>
430
+ </Offscreen>,
431
+ );
432
+ });
433
+ // Now it's visible
434
+ expect(root).toMatchRenderedOutput(<span>Hi</span>);
435
+ });
436
});