Do not unmount layout effects on initial Offscreen mount (#25592)
`wasHidden` is evaluted to false if `current` is null. This means Offscreen has never been shown but this code assumes it is going from 'visible' to 'hidden' and unmounts layout effects. To fix this, only unmount layout effects if `current` is not null. I'm not able to repro this problem or write unit test for it. I see this crash bug in production test. The problem with repro is that if Offscreen starts as hidden, it's render is deferred and current is no longer null.
Samuel Susla committed
Nov 1, 2022 at 15:11 UTC
ab075a232409ea1f566526c6a5dca30f658280de
3 files changed
+42
-2
packages/react-reconciler/src/ReactFiberCommitWork.new.js
+2
-1
@@ -2880,7 +2880,8 @@ function commitMutationEffectsOnFiber(
2880
}
2881
2882
if (isHidden) {
2883
- if (!wasHidden) {
2883
+ // Check if this is an update, and the tree was previously visible.
2884
+ if (current !== null && !wasHidden) {
2885
if ((offscreenBoundary.mode & ConcurrentMode) !== NoMode) {
2886
// Disappear the layout effects of all the children
2887
recursivelyTraverseDisappearLayoutEffects(offscreenBoundary);
packages/react-reconciler/src/ReactFiberCommitWork.old.js
+2
-1
@@ -2880,7 +2880,8 @@ function commitMutationEffectsOnFiber(
2880
}
2881
2882
if (isHidden) {
2883
- if (!wasHidden) {
2883
+ // Check if this is an update, and the tree was previously visible.
2884
+ if (current !== null && !wasHidden) {
2885
if ((offscreenBoundary.mode & ConcurrentMode) !== NoMode) {
2886
// Disappear the layout effects of all the children
2887
recursivelyTraverseDisappearLayoutEffects(offscreenBoundary);
packages/react-reconciler/src/__tests__/ReactOffscreen-test.js
+38
@@ -268,6 +268,44 @@ describe('ReactOffscreen', () => {
268
expect(root).toMatchRenderedOutput(<span prop="Child" />);
269
});
270
271
+ // @gate enableOffscreen
272
+ it('nested offscreen does not call componentWillUnmount when hidden', async () => {
273
+ // This is a bug that appeared during production test of <unstable_Offscreen />.
274
+ // It is a very specific scenario with nested Offscreens. The inner offscreen
275
+ // goes from visible to hidden in synchronous update.
276
+ class ClassComponent extends React.Component {
277
+ render() {
278
+ return <Text text="Child" />;
279
+ }
280
+
281
+ componentWillUnmount() {
282
+ Scheduler.unstable_yieldValue('componentWillUnmount');
283
+ }
284
+ }
285
+
286
+ function App() {
287
+ const [isVisible, setIsVisible] = React.useState(true);
288
+
289
+ if (isVisible === true) {
290
+ setIsVisible(false);
291
+ }
292
+
293
+ return (
294
+ <Offscreen mode="hidden">
295
+ <Offscreen mode={isVisible ? 'visible' : 'hidden'}>
296
+ <ClassComponent />
297
+ </Offscreen>
298
+ </Offscreen>
299
+ );
300
+ }
301
+
302
+ const root = ReactNoop.createRoot();
303
+ await act(async () => {
304
+ root.render(<App />);
305
+ });
306
+ expect(Scheduler).toHaveYielded(['Child']);
307
+ });
308
+
309
// @gate enableOffscreen
310
it('mounts/unmounts layout effects when visibility changes (starting hidden)', async () => {
311
function Child({text}) {