[popups] Cover interaction ownership regressions

Author: atomiks

packages/react/src/dialog/popup/DialogPopup.test.tsx

@@ -2,7 +2,7 @@ import { expect, vi } from 'vitest';
 import * as React from 'react';
 import { Dialog } from '@base-ui/react/dialog';
 import { AlertDialog } from '@base-ui/react/alert-dialog';
-import { act, waitFor, screen } from '@mui/internal-test-utils';
+import { act, fireEvent, waitFor, screen } from '@mui/internal-test-utils';
 import { describeConformance, createRenderer, isJSDOM, waitSingleFrame } from '#test-utils';
 
 describe('<Dialog.Popup />', () => {
@@ -189,6 +189,39 @@ describe('<Dialog.Popup />', () => {
       });
     });
 
+    it('passes the latest interaction type to initialFocus after reopening', async () => {
+      const initialFocus = vi.fn(() => false);
+
+      const { user } = await render(
+        <Dialog.Root modal={false}>
+          <Dialog.Trigger>Open</Dialog.Trigger>
+          <Dialog.Portal>
+            <Dialog.Popup initialFocus={initialFocus}>Content</Dialog.Popup>
+          </Dialog.Portal>
+        </Dialog.Root>,
+      );
+
+      const trigger = screen.getByText('Open');
+      await act(async () => trigger.focus());
+      await user.keyboard('[Enter]');
+
+      await waitFor(() => {
+        expect(initialFocus).toHaveBeenLastCalledWith('keyboard');
+      });
+
+      await user.keyboard('[Escape]');
+      await waitFor(() => {
+        expect(screen.queryByRole('dialog')).toBe(null);
+      });
+
+      fireEvent.pointerDown(trigger, { pointerType: 'touch' });
+      fireEvent.click(trigger, { detail: 1 });
+
+      await waitFor(() => {
+        expect(initialFocus).toHaveBeenLastCalledWith('touch');
+      });
+    });
+
     it('should not move focus when initialFocus is false', async () => {
       function TestComponent() {
         return (

packages/react/src/dialog/root/DialogRoot.test.tsx

@@ -32,6 +32,42 @@ describe('<Dialog.Root />', () => {
     expectedPopupRole: 'dialog',
   });
 
+  it('keeps trigger ownership when another trigger mounts while open', async () => {
+    function Test() {
+      const [showSecondTrigger, setShowSecondTrigger] = React.useState(false);
+
+      return (
+        <Dialog.Root modal={false}>
+          <Dialog.Trigger id="trigger-1">Trigger 1</Dialog.Trigger>
+          {showSecondTrigger && <Dialog.Trigger id="trigger-2">Trigger 2</Dialog.Trigger>}
+          <Dialog.Portal>
+            <Dialog.Popup>
+              <button onClick={() => setShowSecondTrigger(true)}>Mount trigger 2</button>
+            </Dialog.Popup>
+          </Dialog.Portal>
+        </Dialog.Root>
+      );
+    }
+
+    const { user } = await render(<Test />);
+    const trigger1 = screen.getByRole('button', { name: 'Trigger 1' });
+
+    await user.click(trigger1);
+
+    const popup = await screen.findByRole('dialog');
+    const trigger1Controls = trigger1.getAttribute('aria-controls');
+
+    expect(trigger1Controls).toBe(popup.getAttribute('id'));
+
+    await user.click(screen.getByRole('button', { name: 'Mount trigger 2' }));
+
+    const trigger2 = screen.getByRole('button', { name: 'Trigger 2' });
+    expect(trigger1).toHaveAttribute('aria-expanded', 'true');
+    expect(trigger1.getAttribute('aria-controls')).toBe(trigger1Controls);
+    expect(trigger2).toHaveAttribute('aria-expanded', 'false');
+    expect(trigger2).not.toHaveAttribute('aria-controls');
+  });
+
   describe.for([
     { name: 'contained triggers', Component: ContainedTriggerDialog },
     { name: 'detached triggers', Component: DetachedTriggerDialog },

packages/react/src/floating-ui-react/hooks/useClientPoint.test.tsx

@@ -1,10 +1,12 @@
-import { test, expect } from 'vitest';
+import { test, expect, vi } from 'vitest';
 import * as React from 'react';
 import type { Coords } from '@floating-ui/react-dom';
 import { flushMicrotasks } from '@mui/internal-test-utils';
 import { fireEvent, render, screen } from '@testing-library/react';
 import { createChangeEventDetails } from '../../internals/createBaseUIEventDetails';
 import { REASONS } from '../../internals/reasons';
+import { PopupTriggerMap } from '../../utils/popups';
+import { FloatingRootStore } from '../components/FloatingRootStore';
 import { useClientPoint, useFloating, useInteractions } from '../index';
 
 function expectLocation({ x, y }: Coords) {
@@ -21,6 +23,20 @@ function expectRect({ x, y, width, height }: DOMRectInit) {
   expect(Number(screen.getByTestId('height')?.textContent)).toBe(height);
 }
 
+function createRootStore(referenceElement: HTMLElement) {
+  return new FloatingRootStore({
+    open: false,
+    transitionStatus: undefined,
+    referenceElement,
+    floatingElement: document.createElement('div'),
+    triggerElements: new PopupTriggerMap(),
+    floatingId: undefined,
+    syncOnly: false,
+    nested: false,
+    onOpenChange: vi.fn(),
+  });
+}
+
 function App({
   enabled = true,
   axis,
@@ -86,6 +102,17 @@ function App({
   );
 }
 
+function ClientPointStoreTest({
+  store,
+  enabled = true,
+}: {
+  store: FloatingRootStore;
+  enabled?: boolean;
+}) {
+  useClientPoint(store, { enabled });
+  return null;
+}
+
 test('updates position from trigger props', async () => {
   render(<App useTriggerProps />);
 
@@ -264,6 +291,28 @@ test('cleans up window listener when closing or disabling', async () => {
   expectRect({ x: 0, y: 0, width: 0, height: 0 });
 });
 
+test('clears virtual references on unmount without retaining a stale DOM reference', async () => {
+  const reference = document.createElement('button');
+  const store = createRootStore(reference);
+  const virtualReference = {
+    getBoundingClientRect: () => reference.getBoundingClientRect(),
+  };
+
+  store.set('positionReference', virtualReference);
+
+  const { rerender, unmount } = render(<ClientPointStoreTest store={store} />);
+
+  rerender(<ClientPointStoreTest store={store} enabled={false} />);
+  await flushMicrotasks();
+
+  expect(store.state.positionReference).toBe(reference);
+
+  store.set('positionReference', virtualReference);
+  unmount();
+
+  expect(store.state.positionReference).toBe(null);
+});
+
 test('axis x', async () => {
   render(<App axis="x" />);
 

packages/react/src/floating-ui-react/hooks/useClientPoint.ts

@@ -126,8 +126,8 @@ export function useClientPoint(
   const [pointerType, setPointerType] = React.useState<string | undefined>();
   const [reactive, setReactive] = React.useState([]);
 
-  const resetReference = useStableCallback(() => {
-    store.set('positionReference', domReference);
+  const resetReference = useStableCallback((reference: Element | null) => {
+    store.set('positionReference', reference);
   });
 
   const setReference = useStableCallback(
@@ -176,7 +176,7 @@ export function useClientPoint(
 
   React.useEffect(() => {
     if (!enabled) {
-      resetReference();
+      resetReference(domReference);
       return undefined;
     }
 
@@ -204,7 +204,7 @@ export function useClientPoint(
     if (!dataRef.current.openEvent || isMouseBasedEvent(dataRef.current.openEvent)) {
       cleanupListenerRef.current = addEventListener(win, 'mousemove', handleMouseMove);
     } else {
-      store.set('positionReference', domReference);
+      resetReference(domReference);
     }
 
     return cleanupListener;
@@ -220,8 +220,13 @@ export function useClientPoint(
     reactive,
   ]);
 
-  // Keep this separate from the main effect so disabling cursor tracking also clears the virtual reference.
-  React.useEffect(() => resetReference, [resetReference]);
+  // Clear virtual cursor references when the hook unmounts. Enabled flips are handled above.
+  React.useEffect(
+    () => () => {
+      store.set('positionReference', null);
+    },
+    [store],
+  );
 
   React.useEffect(() => {
     if (enabled && !floating) {

packages/react/src/floating-ui-react/hooks/useFloating.test.tsx

@@ -0,0 +1,42 @@
+import { expect, test, vi } from 'vitest';
+import * as React from 'react';
+import { flushMicrotasks, render } from '@mui/internal-test-utils';
+import { PopupTriggerMap } from '../../utils/popups';
+import { FloatingRootStore } from '../components/FloatingRootStore';
+import { useFloating } from './useFloating';
+
+function createRootStore(floatingElement: HTMLElement) {
+  return new FloatingRootStore({
+    open: true,
+    transitionStatus: undefined,
+    referenceElement: document.createElement('button'),
+    floatingElement,
+    triggerElements: new PopupTriggerMap(),
+    floatingId: undefined,
+    syncOnly: false,
+    nested: false,
+    onOpenChange: vi.fn(),
+  });
+}
+
+function Test({ rootContext }: { rootContext: FloatingRootStore }) {
+  useFloating({ rootContext });
+  return null;
+}
+
+test('preserves an externally synced floating element when the root context changes', async () => {
+  const firstFloatingElement = document.createElement('div');
+  const secondFloatingElement = document.createElement('div');
+  const firstStore = createRootStore(firstFloatingElement);
+  const secondStore = createRootStore(secondFloatingElement);
+
+  const { rerender } = render(<Test rootContext={firstStore} />);
+  await flushMicrotasks();
+
+  expect(firstStore.state.floatingElement).toBe(firstFloatingElement);
+
+  rerender(<Test rootContext={secondStore} />);
+  await flushMicrotasks();
+
+  expect(secondStore.state.floatingElement).toBe(secondFloatingElement);
+});

packages/react/src/floating-ui-react/hooks/useSyncedFloatingRootContext.ts

@@ -93,7 +93,7 @@ export function useSyncedFloatingRootContext<
     store.update(valuesToSync);
   }, [open, floatingId, referenceElement, floatingElement, store]);
 
-  // TODO: When `setOpen` is a part of the PopupStore API, we don't need to sync it.
+  // Keep non-reactive context values fresh for interactions that call `store.setOpen`.
   store.context.onOpenChange = handleOpenChange;
   store.context.nested = nested;
 

packages/react/src/popover/popup/PopoverPopup.test.tsx

@@ -1,4 +1,4 @@
-import { expect } from 'vitest';
+import { expect, vi } from 'vitest';
 import * as React from 'react';
 import { Popover } from '@base-ui/react/popover';
 import { act, fireEvent, flushMicrotasks, screen, waitFor } from '@mui/internal-test-utils';
@@ -187,6 +187,41 @@ describe('<Popover.Popup />', () => {
       });
     });
 
+    it('passes the latest interaction type to initialFocus after reopening', async () => {
+      const initialFocus = vi.fn(() => false);
+
+      const { user } = await render(
+        <Popover.Root>
+          <Popover.Trigger>Open</Popover.Trigger>
+          <Popover.Portal>
+            <Popover.Positioner>
+              <Popover.Popup initialFocus={initialFocus}>Content</Popover.Popup>
+            </Popover.Positioner>
+          </Popover.Portal>
+        </Popover.Root>,
+      );
+
+      const trigger = screen.getByText('Open');
+      await act(async () => trigger.focus());
+      await user.keyboard('[Enter]');
+
+      await waitFor(() => {
+        expect(initialFocus).toHaveBeenLastCalledWith('keyboard');
+      });
+
+      await user.keyboard('{Escape}');
+      await waitFor(() => {
+        expect(screen.queryByRole('dialog')).toBe(null);
+      });
+
+      fireEvent.pointerDown(trigger, { pointerType: 'touch' });
+      fireEvent.click(trigger, { detail: 1 });
+
+      await waitFor(() => {
+        expect(initialFocus).toHaveBeenLastCalledWith('touch');
+      });
+    });
+
     it('should not move focus when initialFocus is false', async () => {
       function TestComponent() {
         return (

packages/react/src/preview-card/store/PreviewCardStore.ts

@@ -8,6 +8,7 @@ import {
   popupStoreSelectors,
   PopupStoreState,
   PopupTriggerMap,
+  setOpenTriggerState,
   usePopupStore,
 } from '../../utils/popups';
 import { type PreviewCardRoot } from '../root/PreviewCardRoot';
@@ -90,13 +91,7 @@ export class PreviewCardStore<Payload> extends ReactStore<
         updatedState.instantType = undefined;
       }
 
-      // If a popup is closing, the `trigger` may be null.
-      // We want to keep the previous value so that exit animations are played and focus is returned correctly.
-      const newTriggerId = eventDetails.trigger?.id ?? null;
-      if (newTriggerId || nextOpen) {
-        updatedState.activeTriggerId = newTriggerId;
-        updatedState.activeTriggerElement = eventDetails.trigger ?? null;
-      }
+      setOpenTriggerState(updatedState, nextOpen, eventDetails.trigger);
 
       this.update(updatedState);
     };

packages/react/src/tooltip/store/TooltipStore.ts

@@ -11,6 +11,7 @@ import {
   popupStoreSelectors,
   PopupStoreState,
   PopupTriggerMap,
+  setOpenTriggerState,
   usePopupStore,
 } from '../../utils/popups';
 
@@ -103,13 +104,7 @@ export class TooltipStore<Payload> extends ReactStore<
         updatedState.instantType = undefined;
       }
 
-      // If a popup is closing, the `trigger` may be null.
-      // We want to keep the previous value so that exit animations are played and focus is returned correctly.
-      const newTriggerId = eventDetails.trigger?.id ?? null;
-      if (newTriggerId || nextOpen) {
-        updatedState.activeTriggerId = newTriggerId;
-        updatedState.activeTriggerElement = eventDetails.trigger ?? null;
-      }
+      setOpenTriggerState(updatedState, nextOpen, eventDetails.trigger);
 
       this.update(updatedState);
     };

packages/react/src/utils/popups/popupStoreUtils.ts

@@ -135,7 +135,7 @@ export function setOpenTriggerState(
  * Sets up trigger data forwarding to the store.
  *
  * @param triggerId Id of the trigger.
- * @param triggerElement The trigger DOM element.
+ * @param triggerElementRef Ref for the trigger DOM element.
  * @param store The Store instance managing the popup state.
  * @param stateUpdates An object with state updates to apply when the trigger is active.
  */
@@ -198,8 +198,8 @@ export type PayloadChildRenderFunction<Payload> = (arg: {
 
 /**
  * Ensures that when there's only one trigger element registered, it is set as the active trigger.
- * This allows controlled popups to work correctly without an explicit triggerId, maintaining compatibility
- * with the contained triggers.
+ * This keeps triggerCount reactive while open and allows controlled popups to work correctly without
+ * an explicit triggerId, maintaining compatibility with contained triggers.
  *
  * This should be called on the Root part.
  *