Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -787,5 +787,78 @@ describe('<Dialog.Root />', () => {
expect(screen.queryByRole('dialog')).toBe(null);
});
});

it('does not overwrite a programmatic payload when a trigger mounts while open', async () => {
const dialog = Dialog.createHandle<number>();

function App({ showTrigger }: { showTrigger: boolean }) {
return (
<div>
{showTrigger && (
<Dialog.Trigger handle={dialog} id="late-trigger" payload={42}>
Late trigger
</Dialog.Trigger>
)}
<Dialog.Root handle={dialog}>
{({ payload }: { payload: number | undefined }) => (
<Dialog.Portal>
<Dialog.Popup data-testid="content">{payload}</Dialog.Popup>
</Dialog.Portal>
)}
</Dialog.Root>
</div>
);
}

const { setProps } = await render(<App showTrigger={false} />);

await act(() => dialog.openWithPayload(8));
await waitFor(() => {
expect(screen.queryByRole('dialog')).not.toBe(null);
});
expect(screen.getByTestId('content').textContent).toBe('8');

// Mounting a trigger after the dialog was opened programmatically must not overwrite
// the payload set via `openWithPayload`.
await setProps({ showTrigger: true });

expect(screen.getByTestId('content').textContent).toBe('8');
});

it('does not associate a late single trigger with a programmatically-opened dialog', async () => {
const dialog = Dialog.createHandle<number>();

function App({ showTrigger }: { showTrigger: boolean }) {
return (
<div>
{showTrigger && (
<Dialog.Trigger data-testid="late-trigger" handle={dialog} id="late-trigger">
Late trigger
</Dialog.Trigger>
)}
<Dialog.Root handle={dialog} modal={false}>
<Dialog.Portal>
<Dialog.Popup data-testid="content">Content</Dialog.Popup>
</Dialog.Portal>
</Dialog.Root>
</div>
);
}

const { setProps } = await render(<App showTrigger={false} />);

await act(() => dialog.openWithPayload(1));
await waitFor(() => {
expect(screen.queryByRole('dialog')).not.toBe(null);
});

await setProps({ showTrigger: true });

const trigger = screen.getByTestId('late-trigger');
// The dialog was opened without a trigger, so a late single trigger must not claim an
// ARIA relationship (aria-controls / aria-expanded) to a popup it didn't open.
expect(trigger).not.toHaveAttribute('aria-controls');
expect(trigger).toHaveAttribute('aria-expanded', 'false');
});
});
});
6 changes: 6 additions & 0 deletions packages/react/src/dialog/store/DialogHandle.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,12 @@ export class DialogHandle<Payload> {
true,
createChangeEventDetails(REASONS.imperativeAction, undefined, triggerElement),
);

if (triggerId && !triggerElement) {
// The intended trigger hasn't mounted yet. Record the association so it is claimed
// (with its payload) once it registers, instead of being treated as a trigger-less open.
this.store.update({ activeTriggerId: triggerId, openedWithoutTrigger: false });
}
}

/**
Expand Down
24 changes: 21 additions & 3 deletions packages/react/src/utils/popups/popupStoreUtils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,12 @@ export function setPopupOpenState(

const triggerId = trigger?.id ?? null;

if (open) {
// Track whether this open was initiated without a trigger (e.g. `openWithPayload`), so a
// later-mounting trigger doesn't claim ownership and overwrite programmatically set state.
state.openedWithoutTrigger = trigger == null;
}

// If a popup is closing, the `trigger` may be undefined.
// We want to keep the previous value so that exit animations are played and focus is returned correctly.
if (triggerId || open) {
Expand Down Expand Up @@ -209,10 +215,11 @@ export function useTriggerDataForwarding<State extends PopupStoreState<unknown>>
return;
}

if (activeTriggerId == null && open) {
if (activeTriggerId == null && open && !store.state.openedWithoutTrigger) {
// If a popup is already open, a detached trigger can mount before any active trigger
// has been established. Claim the first registered trigger so trigger-owned focus
// management and ARIA relationships work.
// management and ARIA relationships work. Popups opened imperatively without a trigger
// (`openedWithoutTrigger`) are excluded so a late trigger can't overwrite their payload.
store.update({
activeTriggerId: triggerId,
activeTriggerElement: element,
Expand Down Expand Up @@ -270,6 +277,12 @@ export function useImplicitActiveTrigger<State extends PopupStoreState<unknown>>
if (store.state.triggerCount !== 0) {
store.set('triggerCount', 0);
}
// Reset the triggerless-open flag once closed so a later open through a path that
// doesn't run `setPopupOpenState` (e.g. controlled `open`/`defaultOpen`) isn't
// suppressed by a stale value.
if (store.state.openedWithoutTrigger) {
store.set('openedWithoutTrigger', false);
}
return;
}

Expand All @@ -292,7 +305,12 @@ export function useImplicitActiveTrigger<State extends PopupStoreState<unknown>>
}
}

if (!lostActiveTriggerId && !activeTriggerId && triggerCount === 1) {
if (
!lostActiveTriggerId &&
!activeTriggerId &&
triggerCount === 1 &&
!store.state.openedWithoutTrigger
) {
const iteratorResult = store.context.triggerElements.entries().next();
if (!iteratorResult.done) {
const [implicitTriggerId, implicitTriggerElement] = iteratorResult.value;
Expand Down
11 changes: 10 additions & 1 deletion packages/react/src/utils/popups/store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,12 @@ export type PopupStoreState<Payload> = {
* The currently active trigger DOM element.
*/
activeTriggerElement: Element | null;
/**
* Whether the popup was opened imperatively without a trigger (e.g. via a handle's
* `openWithPayload`/`open(null)`). When set, a later-mounting trigger must not claim ownership
* and overwrite programmatically set state such as `payload`.
*/
openedWithoutTrigger: boolean;
/**
* ID of the trigger (external prop).
*/
Expand Down Expand Up @@ -94,6 +100,7 @@ export function createInitialPopupStoreState<Payload>(): PopupStoreState<Payload
payload: undefined,
activeTriggerId: null,
activeTriggerElement: null,
openedWithoutTrigger: false,
triggerIdProp: undefined,
popupElement: null,
positionerElement: null,
Expand Down Expand Up @@ -168,7 +175,9 @@ function triggerOwnsOpenPopupOrIsOnlyTrigger(state: S, triggerId: string | undef
triggerId !== undefined &&
openSelector(state) &&
activeTriggerIdSelector(state) == null &&
state.triggerCount === 1
state.triggerCount === 1 &&
// A popup opened without a trigger must not be associated with a lone late trigger.
!state.openedWithoutTrigger
);
}

Expand Down
Loading