From 04e498b6400e0496ca07cd960de78bf3a0f2cdde Mon Sep 17 00:00:00 2001 From: yuly_c Date: Mon, 19 Oct 2026 23:52:02 +0300 Subject: [PATCH] fix: release unique popup ownership on trigger unmount --- src/UniqueProvider/index.tsx | 47 +++-- src/UniqueProvider/useTargetState.ts | 37 +++- src/context.ts | 3 +- src/index.tsx | 12 +- tests/unique-unmount.test.tsx | 282 +++++++++++++++++++++++++++ 5 files changed, 362 insertions(+), 19 deletions(-) create mode 100644 tests/unique-unmount.test.tsx diff --git a/src/UniqueProvider/index.tsx b/src/UniqueProvider/index.tsx index 021f93b8..ff108911 100644 --- a/src/UniqueProvider/index.tsx +++ b/src/UniqueProvider/index.tsx @@ -25,7 +25,8 @@ const UniqueProvider = ({ children, postTriggerProps, }: UniqueProviderProps) => { - const [trigger, open, options, onTargetVisibleChanged] = useTargetState(); + const [trigger, open, options, onTargetVisibleChanged, releaseTarget] = + useTargetState(); // ========================== Options =========================== const mergedOptions = React.useMemo(() => { @@ -55,34 +56,55 @@ const UniqueProvider = ({ }); // ========================== Register ========================== - // Store the isOpen function from the latest show call - const isOpenRef = React.useRef<(() => boolean) | null>(null); + const ownerRef = React.useRef(null); + const delayedOwnerRef = React.useRef(null); + const isOpenRef = React.useRef(new Map boolean>()); const delayInvoke = useDelay(); const show = useEvent( (showOptions: UniqueShowOptions, isOpen: () => boolean) => { - // Store the isOpen function for later use in hide - isOpenRef.current = isOpen; + ownerRef.current = showOptions.id; + isOpenRef.current.set(showOptions.id, isOpen); + delayedOwnerRef.current = showOptions.id; delayInvoke(() => { + delayedOwnerRef.current = null; trigger(showOptions); }, showOptions.delay); }, ); - const hide = (delay: number) => { + const hide = useEvent((id: string, delay: number) => { + if (ownerRef.current !== id) { + return; + } + + delayedOwnerRef.current = id; delayInvoke(() => { - // Check if we should still hide by calling the isOpen function - // If isOpen returns true, it means another trigger wants to keep it open - if (isOpenRef.current?.()) { - return; // Don't hide if something else wants it open + delayedOwnerRef.current = null; + // A controlled owner may still need to keep its popup open. + if (ownerRef.current !== id || isOpenRef.current.get(id)?.()) { + return; } trigger(false); // Don't clear target, currentNode, options immediately, wait until animation completes }, delay); - }; + }); + + const release = useEvent((id: string) => { + // Another owner may already have a show or hide scheduled. + if (delayedOwnerRef.current === id) { + delayInvoke(() => {}, 0); + delayedOwnerRef.current = null; + } + isOpenRef.current.delete(id); + const remainingOwner = releaseTarget(id); + if (ownerRef.current === id) { + ownerRef.current = remainingOwner ?? null; + } + }); // Callback after animation completes const onVisibleChanged = useEvent((visible: boolean) => { @@ -142,8 +164,9 @@ const UniqueProvider = ({ () => ({ show, hide, + release, }), - [], + [show, hide, release], ); // =========================== Align ============================ diff --git a/src/UniqueProvider/useTargetState.ts b/src/UniqueProvider/useTargetState.ts index 04c39c9a..35385f85 100644 --- a/src/UniqueProvider/useTargetState.ts +++ b/src/UniqueProvider/useTargetState.ts @@ -17,25 +17,34 @@ export default function useTargetState(): [ /* Will always cache last which is not null */ cacheOptions: UniqueShowOptions | null, onVisibleChanged: (visible: boolean) => void, + release: (id: string) => string | undefined, ] { const [options, setOptions] = React.useState(null); const [open, setOpen] = React.useState(false); const [isAnimating, setIsAnimating] = React.useState(false); + // Unlike cached options, ownership must update between batched cleanups. + const currentOptionsRef = React.useRef(null); const pendingOptionsRef = React.useRef(null); + const updateOptions = (nextOptions: UniqueShowOptions) => { + currentOptionsRef.current = nextOptions; + setOptions(nextOptions); + }; + const trigger = useEvent((nextOptions: UniqueShowOptions | false) => { if (nextOptions === false) { // Clear pending options when hiding pendingOptionsRef.current = null; + currentOptionsRef.current = null; setOpen(false); } else { - if (isAnimating && open) { + if (isAnimating && open && currentOptionsRef.current) { // If animating (appear or enter), cache new options pendingOptionsRef.current = nextOptions; } else { setOpen(true); // Set new options - setOptions(nextOptions); + updateOptions(nextOptions); pendingOptionsRef.current = null; // Only mark as animating when transitioning from closed to open @@ -52,7 +61,7 @@ export default function useTargetState(): [ setIsAnimating(false); if (pendingOptionsRef.current) { // Apply pending options - setOptions(pendingOptionsRef.current); + updateOptions(pendingOptionsRef.current); pendingOptionsRef.current = null; } } else { @@ -62,5 +71,25 @@ export default function useTargetState(): [ } }); - return [trigger, open, options, onVisibleChanged]; + const release = useEvent((id: string) => { + if (pendingOptionsRef.current?.id === id) { + pendingOptionsRef.current = null; + } + + if (currentOptionsRef.current?.id === id) { + const nextOptions = pendingOptionsRef.current; + pendingOptionsRef.current = null; + if (nextOptions) { + updateOptions(nextOptions); + } else { + currentOptionsRef.current = null; + setOpen(false); + } + return nextOptions?.id; + } + + return pendingOptionsRef.current?.id ?? currentOptionsRef.current?.id; + }); + + return [trigger, open, options, onVisibleChanged, release]; } diff --git a/src/context.ts b/src/context.ts index 728c844f..6c60b3b6 100644 --- a/src/context.ts +++ b/src/context.ts @@ -40,7 +40,8 @@ export interface UniqueShowOptions { export interface UniqueContextProps { show: (options: UniqueShowOptions, isOpen: () => boolean) => void; - hide: (delay: number) => void; + hide: (id: string, delay: number) => void; + release: (id: string) => void; } export const UniqueContext = React.createContext( diff --git a/src/index.tsx b/src/index.tsx index 22f29a0a..d294b3b5 100644 --- a/src/index.tsx +++ b/src/index.tsx @@ -364,6 +364,14 @@ export function generateTrigger( onEsc, })); + useLayoutEffect(() => { + return () => { + if (unique && !parentContext) { + uniqueContext?.release(id); + } + }; + }, [uniqueContext, unique, parentContext, id]); + // Handle controlled state changes for UniqueProvider // Only sync to UniqueProvider when it's controlled mode // If there is a parentContext, don't call uniqueContext methods @@ -378,7 +386,7 @@ export function generateTrigger( if (mergedOpen) { uniqueContext.show(getUniqueOptions(mouseEnterDelay), isOpen); } else { - uniqueContext.hide(mouseLeaveDelay); + uniqueContext.hide(id, mouseLeaveDelay); } } }, [mergedOpen, targetEle]); @@ -415,7 +423,7 @@ export function generateTrigger( if (nextOpen) { uniqueContext.show(getUniqueOptions(delay), isOpen); } else { - uniqueContext.hide(delay); + uniqueContext.hide(id, delay); } return; } diff --git a/tests/unique-unmount.test.tsx b/tests/unique-unmount.test.tsx new file mode 100644 index 00000000..33521ce0 --- /dev/null +++ b/tests/unique-unmount.test.tsx @@ -0,0 +1,282 @@ +import React from 'react'; +import { act, cleanup, fireEvent, render } from '@testing-library/react'; +import Trigger, { UniqueProvider } from '../src'; +import type { TriggerProps } from '../src'; +import { awaitFakeTimer } from './util'; + +// The util hook returns the same snapshot ID for every trigger in tests. +jest.mock('@rc-component/util', () => ({ + ...jest.requireActual('@rc-component/util'), + useId: () => jest.requireActual('react').useId(), +})); + +function expectPopup(text: string | null) { + const popup = document.querySelector( + '.rc-trigger-popup:not(.rc-trigger-popup-hidden)', + ); + if (text === null) { + expect(popup).toBeNull(); + } else { + expect(popup?.textContent).toBe(text); + } +} + +interface PairProps { + showA?: boolean; + showB?: boolean; + delayB?: number; + motion?: TriggerProps['popupMotion']; +} + +function Pair({ showA = true, showB = true, delayB = 0, motion }: PairProps) { + return ( + + {showA && ( + Popup A} + popupMotion={motion} + > + + + )} + {showB && ( + Popup B} + popupMotion={motion} + > + + + )} + + ); +} + +describe('unique popup owner unmount', () => { + beforeEach(() => jest.useFakeTimers()); + afterEach(() => { + cleanup(); + jest.clearAllTimers(); + jest.useRealTimers(); + }); + + it.each([false, true])( + 'closes on owner unmount (unique=%s)', + async (unique) => { + const Demo = ({ show }: { show: boolean }) => ( + + {show && ( + Popup A} + > + + + )} + + ); + const { getByRole, queryByRole, rerender } = render(); + fireEvent.mouseEnter(getByRole('button')); + await awaitFakeTimer(); + expectPopup('Popup A'); + rerender(); + await awaitFakeTimer(); + expect(queryByRole('button')).toBeNull(); + expectPopup(null); + }, + ); + + it.each([false, true])( + 'releases a controlled owner under StrictMode=%s', + async (strict) => { + const onOpenChange = jest.fn(); + const Demo = ({ show }: { show: boolean }) => ( + + {show && ( + Popup A} + > + + + )} + + ); + const wrap = (show: boolean) => + strict ? ( + + + + ) : ( + + ); + const { rerender } = render(wrap(true)); + await awaitFakeTimer(); + expectPopup('Popup A'); + rerender(wrap(false)); + await awaitFakeTimer(); + expectPopup(null); + expect(onOpenChange).not.toHaveBeenCalled(); + }, + ); + + it.each([false, true])( + 'keeps B open when inactive A unmounts (previously open=%s)', + async (previouslyOpen) => { + const { getByRole, rerender } = render(); + if (previouslyOpen) { + fireEvent.mouseEnter(getByRole('button', { name: 'A' })); + await awaitFakeTimer(); + expectPopup('Popup A'); + } + fireEvent.mouseEnter(getByRole('button', { name: 'B' })); + await awaitFakeTimer(); + expectPopup('Popup B'); + rerender(); + await awaitFakeTimer(); + expectPopup('Popup B'); + }, + ); + + it('cancels a pending show when its owner unmounts', async () => { + const { getByRole, rerender } = render(); + fireEvent.mouseEnter(getByRole('button', { name: 'B' })); + rerender(); + await awaitFakeTimer(); + expectPopup(null); + }); + + it('preserves A when pending B unmounts and lets A close normally', async () => { + const { getByRole, rerender } = render(); + fireEvent.mouseEnter(getByRole('button', { name: 'A' })); + await awaitFakeTimer(); + fireEvent.mouseEnter(getByRole('button', { name: 'B' })); + rerender(); + await awaitFakeTimer(); + expectPopup('Popup A'); + fireEvent.mouseLeave(getByRole('button', { name: 'A' })); + await awaitFakeTimer(); + expectPopup(null); + }); + + it('closes unmounted A while preserving the delayed show for B', async () => { + const { getByRole, rerender } = render(); + fireEvent.mouseEnter(getByRole('button', { name: 'A' })); + await awaitFakeTimer(); + fireEvent.mouseEnter(getByRole('button', { name: 'B' })); + rerender(); + await act(async () => { + jest.advanceTimersByTime(100); + }); + expectPopup(null); + await awaitFakeTimer(); + expectPopup('Popup B'); + }); + + it("does not let a stale leave cancel the next owner's delayed show", async () => { + const { getByRole } = render(); + fireEvent.mouseEnter(getByRole('button', { name: 'A' })); + await awaitFakeTimer(); + fireEvent.mouseEnter(getByRole('button', { name: 'B' })); + fireEvent.mouseLeave(getByRole('button', { name: 'A' })); + await awaitFakeTimer(); + expectPopup('Popup B'); + }); + + it('can open again after repeated owner remounts', async () => { + const { getByRole, rerender } = render(); + for (let i = 0; i < 2; i += 1) { + fireEvent.mouseEnter(getByRole('button', { name: 'A' })); + await awaitFakeTimer(); + expectPopup('Popup A'); + rerender(); + await awaitFakeTimer(); + expectPopup(null); + rerender(); + } + fireEvent.mouseEnter(getByRole('button', { name: 'A' })); + await awaitFakeTimer(); + expectPopup('Popup A'); + }); + + it("does not let an unmounted owner's delayed hide close B", async () => { + const { getByRole, rerender } = render(); + fireEvent.mouseEnter(getByRole('button', { name: 'A' })); + await awaitFakeTimer(); + fireEvent.mouseLeave(getByRole('button', { name: 'A' })); + rerender(); + fireEvent.mouseEnter(getByRole('button', { name: 'B' })); + await awaitFakeTimer(); + expectPopup('Popup B'); + }); + + it("does not release another provider's popup", async () => { + const Demo = ({ showA }: { showA: boolean }) => ( + <> + + {showA && ( + Popup A} + > + + + )} + + + Popup B}> + + + + + ); + const { rerender } = render(); + await awaitFakeTimer(); + expectPopup('Popup B'); + rerender(); + await awaitFakeTimer(); + expect( + document.querySelector('.first-popup:not(.first-popup-hidden)'), + ).toBeNull(); + expectPopup('Popup B'); + }); + + it.each(['A', 'B', 'both'])( + "releases %s while B is waiting for A's enter motion", + async (removed) => { + const motion = { motionName: 'test-motion', motionDeadline: 1000 }; + const { getByRole, rerender } = render(); + fireEvent.mouseEnter(getByRole('button', { name: 'A' })); + for (let i = 0; i < 5; i += 1) { + await act(async () => { + jest.advanceTimersByTime(20); + }); + } + expectPopup('Popup A'); + fireEvent.mouseEnter(getByRole('button', { name: 'B' })); + expectPopup('Popup A'); + rerender( + , + ); + await awaitFakeTimer(); + // Allow the interrupted enter and subsequent leave motion to finish. + await awaitFakeTimer(); + expectPopup( + removed === 'both' ? null : removed === 'A' ? 'Popup B' : 'Popup A', + ); + }, + ); +});