From 350e02d21bbc61ff48cb655b4e5450f6eb703425 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Thu, 30 Apr 2020 10:57:42 -0700 Subject: [PATCH 1/2] Don't clear a container because of a stale legacy root --- .../src/ReactFiberCompleteWork.new.js | 17 +++++++++++++++-- .../src/ReactFiberCompleteWork.old.js | 17 +++++++++++++++-- 2 files changed, 30 insertions(+), 4 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberCompleteWork.new.js b/packages/react-reconciler/src/ReactFiberCompleteWork.new.js index 3caaba82979be..620279aced2e8 100644 --- a/packages/react-reconciler/src/ReactFiberCompleteWork.new.js +++ b/packages/react-reconciler/src/ReactFiberCompleteWork.new.js @@ -687,8 +687,21 @@ function completeWork( // It's also safe to do for updates too, because current.child would only be null // if the previous render was null (so the the container would already be empty). // - // The additional root.hydrate check is required for hydration in legacy mode with no fallback. - workInProgress.effectTag |= Snapshot; + // The additional root.hydrate check above is required for hydration in legacy mode with no fallback. + // + // The root container check below also avoids a potential legacy mode problem + // where unmounting from a container then rendering into it again + // can sometimes cause the container to be cleared after the new render. + const containerInfo = fiberRoot.containerInfo; + const legacyRootContainer = + containerInfo == null ? null : containerInfo._reactRootContainer; + if ( + legacyRootContainer == null || + legacyRootContainer._internalRoot == null || + legacyRootContainer._internalRoot === fiberRoot + ) { + workInProgress.effectTag |= Snapshot; + } } } updateHostContainer(workInProgress); diff --git a/packages/react-reconciler/src/ReactFiberCompleteWork.old.js b/packages/react-reconciler/src/ReactFiberCompleteWork.old.js index fe032eba86817..818d08ca469f4 100644 --- a/packages/react-reconciler/src/ReactFiberCompleteWork.old.js +++ b/packages/react-reconciler/src/ReactFiberCompleteWork.old.js @@ -685,8 +685,21 @@ function completeWork( // It's also safe to do for updates too, because current.child would only be null // if the previous render was null (so the the container would already be empty). // - // The additional root.hydrate check is required for hydration in legacy mode with no fallback. - workInProgress.effectTag |= Snapshot; + // The additional root.hydrate check above is required for hydration in legacy mode with no fallback. + // + // The root container check below also avoids a potential legacy mode problem + // where unmounting from a container then rendering into it again + // can sometimes cause the container to be cleared after the new render. + const containerInfo = fiberRoot.containerInfo; + const legacyRootContainer = + containerInfo == null ? null : containerInfo._reactRootContainer; + if ( + legacyRootContainer == null || + legacyRootContainer._internalRoot == null || + legacyRootContainer._internalRoot === fiberRoot + ) { + workInProgress.effectTag |= Snapshot; + } } } updateHostContainer(workInProgress); From b6312ff05414a8ec638187bcabfffe24770210aa Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Thu, 30 Apr 2020 14:37:48 -0700 Subject: [PATCH 2/2] Added test repro for FB error --- .../src/__tests__/ReactDOMFiber-test.js | 32 +++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/packages/react-dom/src/__tests__/ReactDOMFiber-test.js b/packages/react-dom/src/__tests__/ReactDOMFiber-test.js index 51c66e773f243..9391b34c446a2 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFiber-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFiber-test.js @@ -1280,4 +1280,36 @@ describe('ReactDOMFiber', () => { ); expect(didCallOnChange).toBe(true); }); + + it('unmounted legacy roots should never clear newer root content from a container', () => { + const ref = React.createRef(); + + function OldApp() { + const hideOnFocus = () => { + // This app unmounts itself inside of a focus event. + ReactDOM.unmountComponentAtNode(container); + }; + + return ( + + ); + } + + function NewApp() { + return ; + } + + ReactDOM.render(, container); + ref.current.focus(); + + ReactDOM.render(, container); + + // Calling focus again will flush previously scheduled discerete work for the old root- + // but this should not clear out the newly mounted app. + ref.current.focus(); + + expect(container.textContent).toBe('new'); + }); });