diff --git a/packages/react-dom/src/__tests__/ReactDOMFragmentRefs-test.js b/packages/react-dom/src/__tests__/ReactDOMFragmentRefs-test.js index d2e7ad08cd5..60c86301e4a 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFragmentRefs-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFragmentRefs-test.js @@ -17,12 +17,14 @@ let act; let container; let Fragment; let Activity; +let Scheduler; let mockIntersectionObserver; let simulateIntersection; let setClientRects; let mockRangeClientRects; let assertConsoleErrorDev; let assertConsoleWarnDev; +let assertLog; function Wrapper({children}) { return children; @@ -38,6 +40,7 @@ describe('FragmentRefs', () => { ReactDOM = require('react-dom'); createPortal = ReactDOM.createPortal; act = require('internal-test-utils').act; + Scheduler = require('scheduler'); const IntersectionMocks = require('./utils/IntersectionMocks'); mockIntersectionObserver = IntersectionMocks.mockIntersectionObserver; simulateIntersection = IntersectionMocks.simulateIntersection; @@ -46,6 +49,7 @@ describe('FragmentRefs', () => { assertConsoleErrorDev = require('internal-test-utils').assertConsoleErrorDev; assertConsoleWarnDev = require('internal-test-utils').assertConsoleWarnDev; + assertLog = require('internal-test-utils').assertLog; container = document.createElement('div'); document.body.innerHTML = ''; @@ -153,6 +157,152 @@ describe('FragmentRefs', () => { expect(childD.reactFragments.has(fragmentParentRef.current)).toBe(true); }); + // @gate enableFragmentRefs + it('runs the ref cleanup when an inline ref callback changes identity', async () => { + const fragmentInstances = []; + let rerender; + + function Test() { + const [step, setStep] = React.useState(0); + rerender = () => { + setStep(p => p + 1); + }; + + return ( + { + fragmentInstances.push(fragmentInstance); + Scheduler.log(`fragment attach ${step}`); + return () => { + Scheduler.log(`fragment cleanup ${step}`); + }; + }}> +
{ + Scheduler.log(`host attach ${step}`); + return () => { + Scheduler.log(`host cleanup ${step}`); + }; + }} + /> + + ); + } + + const root = ReactDOMClient.createRoot(container); + await act(() => root.render()); + assertLog(['fragment attach 0', 'host attach 0']); + + await act(rerender); + // Both refs are inlined, so both change identity and are detached before + // being re-attached. The Fragment detaches ahead of its children, which is + // the same order it uses when the Fragment itself is deleted. + assertLog([ + 'fragment cleanup 0', + 'host cleanup 0', + 'fragment attach 1', + 'host attach 1', + ]); + + await act(() => root.render(null)); + // The cleanups created by the final render run on unmount. + assertLog(['fragment cleanup 1', 'host cleanup 1']); + + // The same FragmentInstance is handed to every attach, so a callback that + // registers event listeners or observers on it can rely on its cleanup to + // unregister them again. + expect(fragmentInstances).toHaveLength(2); + expect(fragmentInstances[0]).toBe(fragmentInstances[1]); + }); + + // @gate enableFragmentRefs + it('runs the ref cleanup when the ref is removed from a mounted Fragment', async () => { + function Test({withRef}) { + return ( + { + Scheduler.log('attach'); + return () => { + Scheduler.log('cleanup'); + }; + } + : null + }> +
+ + ); + } + + const root = ReactDOMClient.createRoot(container); + await act(() => root.render()); + assertLog(['attach']); + + // The Fragment stays mounted and only the ref goes away. commitAttachRef + // bails out on a null ref, so the detach is the only thing that can run the + // cleanup here. + await act(() => root.render()); + assertLog(['cleanup']); + + // Nothing is left to clean up by the time the Fragment is deleted. + await act(() => root.render(null)); + assertLog([]); + }); + + // @gate enableFragmentRefs + it('detaches and reattaches Fragment refs when StrictMode double invokes', async () => { + // This one collects its own log rather than using Scheduler.log, because + // setIsStrictModeForDevtools disables yield values for the duration of the + // double invoke to keep StrictMode tests quiet, which would hide the very + // detach and reattach this test is here to observe. + const logs = []; + let rerender; + + function Test() { + const [step, setStep] = React.useState(0); + rerender = () => { + setStep(p => p + 1); + }; + + return ( + { + logs.push(`attach ${step}`); + return () => { + logs.push(`cleanup ${step}`); + }; + }}> +
+ + ); + } + + const root = ReactDOMClient.createRoot(container); + await act(() => + root.render( + + + , + ), + ); + if (__DEV__) { + // The double invoke goes through disappearLayoutEffects and + // reappearLayoutEffects rather than through the mutation and layout + // phases, so it exercises a separate pair of Fragment cases. + expect(logs).toEqual(['attach 0', 'cleanup 0', 'attach 0']); + } else { + expect(logs).toEqual(['attach 0']); + } + + // The double invoke only applies to newly mounted fibers, so an update + // detaches and reattaches once in both environments. + logs.length = 0; + await act(rerender); + expect(logs).toEqual(['cleanup 0', 'attach 1']); + }); + describe('focus methods', () => { describe('focus()', () => { // @gate enableFragmentRefs diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.js b/packages/react-reconciler/src/ReactFiberCommitWork.js index 3ecde389240..68e6b619222 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.js @@ -2780,6 +2780,11 @@ function commitMutationEffectsOnFiber( } case Fragment: if (enableFragmentRefs) { + if (flags & Ref) { + if (!offscreenSubtreeWasHidden && current !== null) { + safelyDetachRef(current, current.return); + } + } if (current && current.stateNode !== null) { updateFragmentInstanceFiber(finishedWork, current.stateNode); }