Don't disappear layout effects unnecessarily (#25660)
Nested Offscreens can run into a case where outer Offscreen is revealed while inner one is hidden in a single commit. This is an edge case that was previously missed. We need to prevent call to disappear layout effects. When we go from state: ```jsx <Offscreen mode={'hidden'}> // outer offscreen <Offscreen mode={'visible'}> // inner offscreen {children} </Offscreen> </Offscreen> ``` To following. Notice that visibility of each offscreen flips. ```jsx <Offscreen mode={'visible'}> // outer offscreen <Offscreen mode={'hidden'}> // inner offscreen {children} </Offscreen> </Offscreen> ``` Inner offscreen must not call `recursivelyTraverseDisappearLayoutEffects`. Check unit tests for an example of this.
Samuel Susla committed
Nov 10, 2022 at 14:49 UTC
d1e35c70398a3341d2e090d09a0863c7fe6c3325
3 files changed
+75
-31
packages/react-reconciler/src/ReactFiberCommitWork.new.js
+4
-3
@@ -2880,12 +2880,13 @@ function commitMutationEffectsOnFiber(
2880
2881
if (isHidden) {
2882
const isUpdate = current !== null;
2883
- const isAncestorOffscreenHidden = offscreenSubtreeIsHidden;
2883
+ const wasHiddenByAncestorOffscreen =
2884
+ offscreenSubtreeIsHidden || offscreenSubtreeWasHidden;
2885
// Only trigger disapper layout effects if:
2886
// - This is an update, not first mount.
2887
// - This Offscreen was not hidden before.
2887
- // - No ancestor Offscreen is hidden.
2888
- if (isUpdate && !wasHidden && !isAncestorOffscreenHidden) {
2888
+ // - Ancestor Offscreen was not hidden in previous commit.
2889
+ if (isUpdate && !wasHidden && !wasHiddenByAncestorOffscreen) {
2890
if ((finishedWork.mode & ConcurrentMode) !== NoMode) {
2891
// Disappear the layout effects of all the children
2892
recursivelyTraverseDisappearLayoutEffects(finishedWork);
packages/react-reconciler/src/ReactFiberCommitWork.old.js
+4
-3
@@ -2880,12 +2880,13 @@ function commitMutationEffectsOnFiber(
2880
2881
if (isHidden) {
2882
const isUpdate = current !== null;
2883
- const isAncestorOffscreenHidden = offscreenSubtreeIsHidden;
2883
+ const wasHiddenByAncestorOffscreen =
2884
+ offscreenSubtreeIsHidden || offscreenSubtreeWasHidden;
2885
// Only trigger disapper layout effects if:
2886
// - This is an update, not first mount.
2887
// - This Offscreen was not hidden before.
2887
- // - No ancestor Offscreen is hidden.
2888
- if (isUpdate && !wasHidden && !isAncestorOffscreenHidden) {
2888
+ // - Ancestor Offscreen was not hidden in previous commit.
2889
+ if (isUpdate && !wasHidden && !wasHiddenByAncestorOffscreen) {
2890
if ((finishedWork.mode & ConcurrentMode) !== NoMode) {
2891
// Disappear the layout effects of all the children
2892
recursivelyTraverseDisappearLayoutEffects(finishedWork);
packages/react-reconciler/src/__tests__/ReactOffscreen-test.js
+67
-25
@@ -275,7 +275,7 @@ describe('ReactOffscreen', () => {
275
// goes from visible to hidden in synchronous update.
276
class ClassComponent extends React.Component {
277
render() {
278
- return <Text text="Child" />;
278
+ return <Text text="child" />;
279
}
280
281
componentWillUnmount() {
@@ -287,47 +287,89 @@ describe('ReactOffscreen', () => {
287
}
288
}
289
290
- let _setIsVisible;
291
- let isFirstRender = true;
290
+ const root = ReactNoop.createRoot();
291
+ await act(async () => {
292
+ // Outer and inner offscreen are hidden.
293
+ root.render(
294
+ <Offscreen mode={'hidden'}>
295
+ <Offscreen mode={'hidden'}>
296
+ <ClassComponent />
297
+ </Offscreen>
298
+ </Offscreen>,
299
+ );
300
+ });
301
293
- function App() {
294
- const [isVisible, setIsVisible] = React.useState(true);
295
- _setIsVisible = setIsVisible;
296
- if (isFirstRender === true) {
297
- setIsVisible(false);
298
- isFirstRender = false;
299
- }
302
+ expect(Scheduler).toHaveYielded(['child']);
303
+ expect(root).toMatchRenderedOutput(<span hidden={true} prop="child" />);
304
301
- return (
302
- <Offscreen mode="hidden">
303
- <Offscreen mode={isVisible ? 'visible' : 'hidden'}>
305
+ await act(async () => {
306
+ // Inner offscreen is visible.
307
+ root.render(
308
+ <Offscreen mode={'hidden'}>
309
+ <Offscreen mode={'visible'}>
310
<ClassComponent />
311
</Offscreen>
306
- </Offscreen>
312
+ </Offscreen>,
313
);
308
- }
314
+ });
315
+
316
+ expect(Scheduler).toHaveYielded(['child']);
317
+ expect(root).toMatchRenderedOutput(<span hidden={true} prop="child" />);
318
310
- const root = ReactNoop.createRoot();
319
await act(async () => {
312
- root.render(<App />);
320
+ // Inner offscreen is hidden.
321
+ root.render(
322
+ <Offscreen mode={'hidden'}>
323
+ <Offscreen mode={'hidden'}>
324
+ <ClassComponent />
325
+ </Offscreen>
326
+ </Offscreen>,
327
+ );
328
});
329
315
- expect(Scheduler).toHaveYielded(['Child']);
316
- expect(root).toMatchRenderedOutput(<span hidden={true} prop="Child" />);
330
+ expect(Scheduler).toHaveYielded(['child']);
331
+ expect(root).toMatchRenderedOutput(<span hidden={true} prop="child" />);
332
333
await act(async () => {
319
- _setIsVisible(true);
334
+ // Inner offscreen is visible.
335
+ root.render(
336
+ <Offscreen mode={'hidden'}>
337
+ <Offscreen mode={'visible'}>
338
+ <ClassComponent />
339
+ </Offscreen>
340
+ </Offscreen>,
341
+ );
342
});
343
322
- expect(Scheduler).toHaveYielded(['Child']);
323
- expect(root).toMatchRenderedOutput(<span hidden={true} prop="Child" />);
344
+ Scheduler.unstable_clearYields();
345
+
346
+ await act(async () => {
347
+ // Outer offscreen is visible.
348
+ // Inner offscreen is hidden.
349
+ root.render(
350
+ <Offscreen mode={'visible'}>
351
+ <Offscreen mode={'hidden'}>
352
+ <ClassComponent />
353
+ </Offscreen>
354
+ </Offscreen>,
355
+ );
356
+ });
357
+
358
+ expect(Scheduler).toHaveYielded(['child']);
359
360
await act(async () => {
326
- _setIsVisible(false);
361
+ // Outer offscreen is hidden.
362
+ // Inner offscreen is visible.
363
+ root.render(
364
+ <Offscreen mode={'hidden'}>
365
+ <Offscreen mode={'visible'}>
366
+ <ClassComponent />
367
+ </Offscreen>
368
+ </Offscreen>,
369
+ );
370
});
371
329
- expect(Scheduler).toHaveYielded(['Child']);
330
- expect(root).toMatchRenderedOutput(<span hidden={true} prop="Child" />);
372
+ expect(Scheduler).toHaveYielded(['child']);
373
});
374
375
// @gate enableOffscreen