Do not unmount layout effects if ancestor Offscreen is hidden (#25628)
This is a follow up on https://github.com/facebook/react/pull/25592 There is another condition Offscreen calls `recursivelyTraverseDisappearLayoutEffects` when it shouldn't. Offscreen may be nested. When nested Offscreen is hidden, it should only unmount layout effects if it meets following conditions: 1. This is an update, not first mount. 2. This Offscreen was hidden before. 3. No ancestor Offscreen is hidden. Previously, we were not accounting for the third condition.
Samuel Susla committed
Nov 4, 2022 at 13:31 UTC
4bd245e9ee22458bcd5b68524c47eaaab2cf2058
3 files changed
+46
-14
packages/react-reconciler/src/ReactFiberCommitWork.new.js
+10
-6
@@ -2869,7 +2869,6 @@ function commitMutationEffectsOnFiber(
2869
2870
if (flags & Visibility) {
2871
const offscreenInstance: OffscreenInstance = finishedWork.stateNode;
2872
- const offscreenBoundary: Fiber = finishedWork;
2872
2873
// Track the current state on the Offscreen instance so we can
2874
// read it during an event
@@ -2880,11 +2879,16 @@ function commitMutationEffectsOnFiber(
2879
}
2880
2881
if (isHidden) {
2883
- // Check if this is an update, and the tree was previously visible.
2884
- if (current !== null && !wasHidden) {
2885
- if ((offscreenBoundary.mode & ConcurrentMode) !== NoMode) {
2882
+ const isUpdate = current !== null;
2883
+ const isAncestorOffscreenHidden = offscreenSubtreeIsHidden;
2884
+ // Only trigger disapper layout effects if:
2885
+ // - This is an update, not first mount.
2886
+ // - This Offscreen was not hidden before.
2887
+ // - No ancestor Offscreen is hidden.
2888
+ if (isUpdate && !wasHidden && !isAncestorOffscreenHidden) {
2889
+ if ((finishedWork.mode & ConcurrentMode) !== NoMode) {
2890
// Disappear the layout effects of all the children
2887
- recursivelyTraverseDisappearLayoutEffects(offscreenBoundary);
2891
+ recursivelyTraverseDisappearLayoutEffects(finishedWork);
2892
}
2893
}
2894
} else {
@@ -2897,7 +2901,7 @@ function commitMutationEffectsOnFiber(
2901
if (supportsMutation && !isOffscreenManual(finishedWork)) {
2902
// TODO: This needs to run whenever there's an insertion or update
2903
// inside a hidden Offscreen tree.
2900
- hideOrUnhideAllChildren(offscreenBoundary, isHidden);
2904
+ hideOrUnhideAllChildren(finishedWork, isHidden);
2905
}
2906
}
2907
packages/react-reconciler/src/ReactFiberCommitWork.old.js
+10
-6
@@ -2869,7 +2869,6 @@ function commitMutationEffectsOnFiber(
2869
2870
if (flags & Visibility) {
2871
const offscreenInstance: OffscreenInstance = finishedWork.stateNode;
2872
- const offscreenBoundary: Fiber = finishedWork;
2872
2873
// Track the current state on the Offscreen instance so we can
2874
// read it during an event
@@ -2880,11 +2879,16 @@ function commitMutationEffectsOnFiber(
2879
}
2880
2881
if (isHidden) {
2883
- // Check if this is an update, and the tree was previously visible.
2884
- if (current !== null && !wasHidden) {
2885
- if ((offscreenBoundary.mode & ConcurrentMode) !== NoMode) {
2882
+ const isUpdate = current !== null;
2883
+ const isAncestorOffscreenHidden = offscreenSubtreeIsHidden;
2884
+ // Only trigger disapper layout effects if:
2885
+ // - This is an update, not first mount.
2886
+ // - This Offscreen was not hidden before.
2887
+ // - No ancestor Offscreen is hidden.
2888
+ if (isUpdate && !wasHidden && !isAncestorOffscreenHidden) {
2889
+ if ((finishedWork.mode & ConcurrentMode) !== NoMode) {
2890
// Disappear the layout effects of all the children
2887
- recursivelyTraverseDisappearLayoutEffects(offscreenBoundary);
2891
+ recursivelyTraverseDisappearLayoutEffects(finishedWork);
2892
}
2893
}
2894
} else {
@@ -2897,7 +2901,7 @@ function commitMutationEffectsOnFiber(
2901
if (supportsMutation && !isOffscreenManual(finishedWork)) {
2902
// TODO: This needs to run whenever there's an insertion or update
2903
// inside a hidden Offscreen tree.
2900
- hideOrUnhideAllChildren(offscreenBoundary, isHidden);
2904
+ hideOrUnhideAllChildren(finishedWork, isHidden);
2905
}
2906
}
2907
packages/react-reconciler/src/__tests__/ReactOffscreen-test.js
+26
-2
@@ -281,13 +281,21 @@ describe('ReactOffscreen', () => {
281
componentWillUnmount() {
282
Scheduler.unstable_yieldValue('componentWillUnmount');
283
}
284
+
285
+ componentDidMount() {
286
+ Scheduler.unstable_yieldValue('componentDidMount');
287
+ }
288
}
289
290
+ let _setIsVisible;
291
+ let isFirstRender = true;
292
+
293
function App() {
294
const [isVisible, setIsVisible] = React.useState(true);
288
-
289
- if (isVisible === true) {
295
+ _setIsVisible = setIsVisible;
296
+ if (isFirstRender === true) {
297
setIsVisible(false);
298
+ isFirstRender = false;
299
}
300
301
return (
@@ -303,7 +311,23 @@ describe('ReactOffscreen', () => {
311
await act(async () => {
312
root.render(<App />);
313
});
314
+
315
+ expect(Scheduler).toHaveYielded(['Child']);
316
+ expect(root).toMatchRenderedOutput(<span hidden={true} prop="Child" />);
317
+
318
+ await act(async () => {
319
+ _setIsVisible(true);
320
+ });
321
+
322
+ expect(Scheduler).toHaveYielded(['Child']);
323
+ expect(root).toMatchRenderedOutput(<span hidden={true} prop="Child" />);
324
+
325
+ await act(async () => {
326
+ _setIsVisible(false);
327
+ });
328
+
329
expect(Scheduler).toHaveYielded(['Child']);
330
+ expect(root).toMatchRenderedOutput(<span hidden={true} prop="Child" />);
331
});
332
333
// @gate enableOffscreen