From f26cf747f07a1abba3a864aaff01d2cad9c2a4a6 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Mon, 19 Apr 2021 15:56:57 -0400 Subject: [PATCH] Only hide outermost host nodes when Offscreen is hidden --- .../src/ReactFiberCommitWork.new.js | 39 ++++--- .../src/ReactFiberCommitWork.old.js | 39 ++++--- .../ReactSuspenseEffectsSemantics-test.js | 110 ++++++++++++++++++ 3 files changed, 154 insertions(+), 34 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js index 4785ff9b24efc..1e1a3f0b98452 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.new.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.new.js @@ -1025,8 +1025,8 @@ function hideOrUnhideAllChildren(finishedWork, isHidden) { const current = finishedWork.alternate; const wasHidden = current !== null && current.memoizedState !== null; - // Only hide the top-most host nodes. - let hiddenHostSubtreeRoot = null; + // Only hide or unhide the top-most host nodes. + let hostSubtreeRoot = null; if (supportsMutation) { // We only have the top Fiber that was inserted but we need to recurse down its @@ -1034,12 +1034,15 @@ function hideOrUnhideAllChildren(finishedWork, isHidden) { let node: Fiber = finishedWork; while (true) { if (node.tag === HostComponent) { - const instance = node.stateNode; - if (isHidden && hiddenHostSubtreeRoot === null) { - hiddenHostSubtreeRoot = node; - hideInstance(instance); - } else { - unhideInstance(node.stateNode, node.memoizedProps); + if (hostSubtreeRoot === null) { + hostSubtreeRoot = node; + + const instance = node.stateNode; + if (isHidden) { + hideInstance(instance); + } else { + unhideInstance(node.stateNode, node.memoizedProps); + } } if (enableSuspenseLayoutEffectSemantics && isModernRoot) { @@ -1058,11 +1061,13 @@ function hideOrUnhideAllChildren(finishedWork, isHidden) { } } } else if (node.tag === HostText) { - const instance = node.stateNode; - if (isHidden && hiddenHostSubtreeRoot === null) { - hideTextInstance(instance); - } else { - unhideTextInstance(instance, node.memoizedProps); + if (hostSubtreeRoot === null) { + const instance = node.stateNode; + if (isHidden) { + hideTextInstance(instance); + } else { + unhideTextInstance(instance, node.memoizedProps); + } } } else if ( (node.tag === OffscreenComponent || @@ -1132,15 +1137,15 @@ function hideOrUnhideAllChildren(finishedWork, isHidden) { return; } - if (hiddenHostSubtreeRoot === node) { - hiddenHostSubtreeRoot = null; + if (hostSubtreeRoot === node) { + hostSubtreeRoot = null; } node = node.return; } - if (hiddenHostSubtreeRoot === node) { - hiddenHostSubtreeRoot = null; + if (hostSubtreeRoot === node) { + hostSubtreeRoot = null; } node.sibling.return = node.return; diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.old.js b/packages/react-reconciler/src/ReactFiberCommitWork.old.js index f39c96f5d1df6..f631c0131aff2 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.old.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.old.js @@ -1025,8 +1025,8 @@ function hideOrUnhideAllChildren(finishedWork, isHidden) { const current = finishedWork.alternate; const wasHidden = current !== null && current.memoizedState !== null; - // Only hide the top-most host nodes. - let hiddenHostSubtreeRoot = null; + // Only hide or unhide the top-most host nodes. + let hostSubtreeRoot = null; if (supportsMutation) { // We only have the top Fiber that was inserted but we need to recurse down its @@ -1034,12 +1034,15 @@ function hideOrUnhideAllChildren(finishedWork, isHidden) { let node: Fiber = finishedWork; while (true) { if (node.tag === HostComponent) { - const instance = node.stateNode; - if (isHidden && hiddenHostSubtreeRoot === null) { - hiddenHostSubtreeRoot = node; - hideInstance(instance); - } else { - unhideInstance(node.stateNode, node.memoizedProps); + if (hostSubtreeRoot === null) { + hostSubtreeRoot = node; + + const instance = node.stateNode; + if (isHidden) { + hideInstance(instance); + } else { + unhideInstance(node.stateNode, node.memoizedProps); + } } if (enableSuspenseLayoutEffectSemantics && isModernRoot) { @@ -1058,11 +1061,13 @@ function hideOrUnhideAllChildren(finishedWork, isHidden) { } } } else if (node.tag === HostText) { - const instance = node.stateNode; - if (isHidden && hiddenHostSubtreeRoot === null) { - hideTextInstance(instance); - } else { - unhideTextInstance(instance, node.memoizedProps); + if (hostSubtreeRoot === null) { + const instance = node.stateNode; + if (isHidden) { + hideTextInstance(instance); + } else { + unhideTextInstance(instance, node.memoizedProps); + } } } else if ( (node.tag === OffscreenComponent || @@ -1132,15 +1137,15 @@ function hideOrUnhideAllChildren(finishedWork, isHidden) { return; } - if (hiddenHostSubtreeRoot === node) { - hiddenHostSubtreeRoot = null; + if (hostSubtreeRoot === node) { + hostSubtreeRoot = null; } node = node.return; } - if (hiddenHostSubtreeRoot === node) { - hiddenHostSubtreeRoot = null; + if (hostSubtreeRoot === node) { + hostSubtreeRoot = null; } node.sibling.return = node.return; diff --git a/packages/react-reconciler/src/__tests__/ReactSuspenseEffectsSemantics-test.js b/packages/react-reconciler/src/__tests__/ReactSuspenseEffectsSemantics-test.js index 0c9cca1f730f9..1cf5d3014f882 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspenseEffectsSemantics-test.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspenseEffectsSemantics-test.js @@ -1284,6 +1284,116 @@ describe('ReactSuspenseEffectsSemantics', () => { ]); }); + // @gate enableSuspenseLayoutEffectSemantics + // @gate enableCache + it('should show nested host nodes if multiple boundaries resolve at the same time', async () => { + function App({innerChildren = null, outerChildren = null}) { + return ( + }> + + {outerChildren} + }> + + {innerChildren} + + + ); + } + + // Mount + await ReactNoop.act(async () => { + ReactNoop.render(); + }); + expect(Scheduler).toHaveYielded([ + 'Text:Outer render', + 'Text:Inner render', + 'Text:Outer create layout', + 'Text:Inner create layout', + 'Text:Outer create passive', + 'Text:Inner create passive', + ]); + expect(ReactNoop.getChildren()).toEqual([span('Outer'), span('Inner')]); + + // Suspend the inner Suspense subtree (only inner effects should be destroyed) + ReactNoop.act(() => { + ReactNoop.render( + } />, + ); + }); + await advanceTimers(1000); + expect(Scheduler).toHaveYielded([ + 'Text:Outer render', + 'Text:Inner render', + 'Suspend:InnerAsync_1', + 'Text:InnerFallback render', + 'Text:Inner destroy layout', + 'Text:InnerFallback create layout', + ]); + expect(Scheduler).toFlushAndYield(['Text:InnerFallback create passive']); + expect(ReactNoop.getChildren()).toEqual([ + span('Outer'), + spanHidden('Inner'), + span('InnerFallback'), + ]); + + // Suspend the outer Suspense subtree (outer effects and inner fallback effects should be destroyed) + // (This check also ensures we don't destroy effects for mounted inner fallback.) + ReactNoop.act(() => { + ReactNoop.render( + } + innerChildren={} + />, + ); + }); + await advanceTimers(1000); + expect(Scheduler).toHaveYielded([ + 'Text:Outer render', + 'Suspend:OuterAsync_1', + 'Text:Inner render', + 'Suspend:InnerAsync_1', + 'Text:InnerFallback render', + 'Text:OuterFallback render', + 'Text:Outer destroy layout', + 'Text:InnerFallback destroy layout', + 'Text:OuterFallback create layout', + ]); + expect(Scheduler).toFlushAndYield(['Text:OuterFallback create passive']); + expect(ReactNoop.getChildren()).toEqual([ + spanHidden('Outer'), + spanHidden('Inner'), + spanHidden('InnerFallback'), + span('OuterFallback'), + ]); + + // Resolve both suspended trees. + await ReactNoop.act(async () => { + await resolveText('OuterAsync_1'); + await resolveText('InnerAsync_1'); + }); + expect(Scheduler).toHaveYielded([ + 'Text:Outer render', + 'AsyncText:OuterAsync_1 render', + 'Text:Inner render', + 'AsyncText:InnerAsync_1 render', + 'Text:OuterFallback destroy layout', + 'Text:Outer create layout', + 'AsyncText:OuterAsync_1 create layout', + 'Text:Inner create layout', + 'AsyncText:InnerAsync_1 create layout', + 'Text:OuterFallback destroy passive', + 'Text:InnerFallback destroy passive', + 'AsyncText:OuterAsync_1 create passive', + 'AsyncText:InnerAsync_1 create passive', + ]); + expect(ReactNoop.getChildren()).toEqual([ + span('Outer'), + span('OuterAsync_1'), + span('Inner'), + span('InnerAsync_1'), + ]); + }); + // @gate enableSuspenseLayoutEffectSemantics // @gate enableCache it('should be cleaned up inside of a fallback that suspends', async () => {