diff --git a/e2e/scenarios/dialog-outside-dismiss.test.ts b/e2e/scenarios/dialog-outside-dismiss.test.ts new file mode 100644 index 000000000..b50988907 --- /dev/null +++ b/e2e/scenarios/dialog-outside-dismiss.test.ts @@ -0,0 +1,107 @@ +// Cross-target (browser): a dialog decides from its contents whether a click +// outside closes it. While it holds a form field — including the one-time key +// reveal's read-only inputs — a stray click on the page behind it must not +// discard what the user typed or was shown. A field-free confirm dialog has +// nothing to lose, so the same click dismisses it without firing its action. +// The API keys page exercises both kinds in one flow. +import { Effect } from "effect"; +import { expect } from "@effect/vitest"; +import { AccountApi, addGroup } from "@executor-js/api"; + +import { scenario } from "../src/scenario"; +import { Api, Browser, Target } from "../src/services"; +import { visit } from "../src/surfaces/browser"; + +const accountApi = addGroup(AccountApi); + +const KEY_NAME = "outside-click-check"; + +scenario( + "Dialogs (UI) · Outside click keeps a form open, closes a field-free confirm", + {}, + Effect.gen(function* () { + const target = yield* Target; + const browser = yield* Browser; + const { client: makeClient } = yield* Api; + const identity = yield* target.newIdentity(); + const client = yield* makeClient(accountApi, identity); + + const session = browser.session(identity, async ({ page, step }) => { + const createDialog = page.getByRole("dialog").filter({ hasText: "Create API key" }); + const revokeDialog = page.getByRole("dialog").filter({ hasText: "Revoke API key" }); + const nameInput = page.locator("#create-key-name"); + const keyRow = page.getByText(KEY_NAME, { exact: true }); + // The overlay covers the whole viewport and the dialog panel is centered, + // so the top-left corner is always outside the panel. + const clickOutside = async () => { + await page.mouse.click(8, 8); + // Dismissal is synchronous with the pointer event; this beat only gives + // a wrongly-dismissed dialog time to actually unmount before the + // "still open" assertions run. + await page.waitForTimeout(400); + }; + + await step("Open the create dialog and name the key", async () => { + await visit(page, "/api-keys"); + await page.getByRole("button", { name: "New key" }).click(); + await nameInput.waitFor(); + await nameInput.fill(KEY_NAME); + }); + + await step("A stray outside click does not discard the form", async () => { + await clickOutside(); + await createDialog.waitFor({ state: "visible" }); + expect(await nameInput.inputValue(), "the typed name survives the stray click").toBe( + KEY_NAME, + ); + }); + + await step("The one-time key reveal also survives an outside click", async () => { + await createDialog.getByRole("button", { name: "Create key" }).click(); + await createDialog.getByText("It is only shown once.").waitFor(); + await clickOutside(); + await createDialog.getByText("It is only shown once.").waitFor({ state: "visible" }); + }); + + await step("Closing the reveal deliberately lands the key in the list", async () => { + // "Close" also names the corner X; the footer's ghost button is the + // deliberate path under test. + await createDialog + .locator("[data-slot='dialog-footer']") + .getByRole("button", { name: "Close" }) + .click(); + await createDialog.waitFor({ state: "detached" }); + await keyRow.waitFor(); + }); + + await step("An outside click dismisses the field-free revoke confirm", async () => { + await page.getByRole("button", { name: `Revoke ${KEY_NAME}` }).click(); + await revokeDialog.waitFor({ state: "visible" }); + await clickOutside(); + await revokeDialog.waitFor({ state: "detached" }); + // Dismissal closed the dialog without firing the revoke. + await keyRow.waitFor({ state: "visible" }); + }); + + await step("Confirming the revoke still removes the key", async () => { + await page.getByRole("button", { name: `Revoke ${KEY_NAME}` }).click(); + await revokeDialog.getByRole("button", { name: "Revoke key" }).click(); + await revokeDialog.waitFor({ state: "detached" }); + await keyRow.waitFor({ state: "detached" }); + }); + }); + + // The final step revokes the key through the UI; this sweep only matters + // when a mid-test failure aborts the session before that step runs. + yield* Effect.ensuring( + session, + Effect.gen(function* () { + const { apiKeys } = yield* client.account.listApiKeys(); + yield* Effect.forEach( + apiKeys.filter((key) => key.name === KEY_NAME), + (key) => client.account.revokeApiKey({ params: { apiKeyId: key.id } }), + ); + }).pipe(Effect.ignore), + ); + }), +); diff --git a/packages/react/src/components/dialog.tsx b/packages/react/src/components/dialog.tsx index 91ae54ed5..f145372f2 100644 --- a/packages/react/src/components/dialog.tsx +++ b/packages/react/src/components/dialog.tsx @@ -2,6 +2,7 @@ import * as React from "react"; import { XIcon } from "lucide-react"; import { Dialog as DialogPrimitive } from "radix-ui"; +import { useComposedRef } from "../lib/compose-refs"; import { applyOutsideDismissPolicy } from "../lib/outside-dismiss"; import { cn } from "../lib/utils"; import { Button } from "./button"; @@ -42,17 +43,19 @@ function DialogContent({ className, children, showCloseButton = true, - dismissOnOutsideClick = false, + dismissOnOutsideClick, onInteractOutside, onPointerDownOutside, forceOverlay = false, + ref, ...props }: React.ComponentProps & { showCloseButton?: boolean; - /** Let a click outside the dialog close it. Off by default: a stray click on - * the page behind a form must not discard what the user typed. Turn it on - * for a dialog with nothing to lose — a confirmation, a picker, a read-only - * panel. Escape and the close button close either way. */ + /** Whether a click outside the dialog closes it. Unset, the dialog decides + * from its contents: it stays open while it holds a form field — a stray + * click on the page behind a form must not discard what the user typed — + * and closes when it has nothing to lose. Pass a boolean to override the + * detection. Escape and the close button close either way. */ dismissOnOutsideClick?: boolean; /** Pair with ``: Radix renders no overlay in non-modal * mode, so this renders a plain dim layer instead. It still eats outside @@ -60,6 +63,8 @@ function DialogContent({ * portaled popups. */ forceOverlay?: boolean; }) { + const contentRef = React.useRef | null>(null); + const composedRef = useComposedRef(ref, contentRef); return ( @@ -68,13 +73,14 @@ function DialogContent({ ) : null} { onPointerDownOutside?.(event); - applyOutsideDismissPolicy(event, dismissOnOutsideClick); + applyOutsideDismissPolicy(event, dismissOnOutsideClick, contentRef.current); }} onInteractOutside={(event) => { onInteractOutside?.(event); - applyOutsideDismissPolicy(event, dismissOnOutsideClick); + applyOutsideDismissPolicy(event, dismissOnOutsideClick, contentRef.current); }} className={cn( "fixed top-[50%] left-[50%] z-50 grid w-full max-w-[calc(100%-2rem)] translate-x-[-50%] translate-y-[-50%] gap-4 rounded-lg border bg-background p-6 shadow-lg duration-200 outline-none data-[state=closed]:animate-out data-[state=closed]:fade-out-0 data-[state=closed]:zoom-out-95 data-[state=open]:animate-in data-[state=open]:fade-in-0 data-[state=open]:zoom-in-95 sm:max-w-lg", diff --git a/packages/react/src/components/sheet.tsx b/packages/react/src/components/sheet.tsx index 204500665..24e49c5ee 100644 --- a/packages/react/src/components/sheet.tsx +++ b/packages/react/src/components/sheet.tsx @@ -4,6 +4,7 @@ import * as React from "react"; import { XIcon } from "lucide-react"; import { Dialog as SheetPrimitive } from "radix-ui"; +import { useComposedRef } from "../lib/compose-refs"; import { applyOutsideDismissPolicy } from "../lib/outside-dismiss"; import { cn } from "../lib/utils"; @@ -44,31 +45,36 @@ function SheetContent({ children, side = "right", showCloseButton = true, - dismissOnOutsideClick = false, + dismissOnOutsideClick, onInteractOutside, onPointerDownOutside, + ref, ...props }: React.ComponentProps & { side?: "top" | "right" | "bottom" | "left"; showCloseButton?: boolean; - /** Let a click outside the sheet close it. Off by default: a stray click on - * the page behind a form must not discard what the user typed. Turn it on - * for a sheet with nothing to lose — navigation, a picker, a read-only - * panel. Escape and the close button close either way. */ + /** Whether a click outside the sheet closes it. Unset, the sheet decides + * from its contents: it stays open while it holds a form field — a stray + * click on the page behind a form must not discard what the user typed — + * and closes when it has nothing to lose. Pass a boolean to override the + * detection. Escape and the close button close either way. */ dismissOnOutsideClick?: boolean; }) { + const contentRef = React.useRef | null>(null); + const composedRef = useComposedRef(ref, contentRef); return ( { onPointerDownOutside?.(event); - applyOutsideDismissPolicy(event, dismissOnOutsideClick); + applyOutsideDismissPolicy(event, dismissOnOutsideClick, contentRef.current); }} onInteractOutside={(event) => { onInteractOutside?.(event); - applyOutsideDismissPolicy(event, dismissOnOutsideClick); + applyOutsideDismissPolicy(event, dismissOnOutsideClick, contentRef.current); }} className={cn( "fixed z-50 flex flex-col gap-4 bg-background shadow-lg transition ease-in-out data-[state=closed]:animate-out data-[state=closed]:duration-300 data-[state=open]:animate-in data-[state=open]:duration-500", diff --git a/packages/react/src/lib/compose-refs.test.ts b/packages/react/src/lib/compose-refs.test.ts new file mode 100644 index 000000000..6c6d55e9f --- /dev/null +++ b/packages/react/src/lib/compose-refs.test.ts @@ -0,0 +1,68 @@ +import { describe, expect, it } from "@effect/vitest"; + +import { composedRefCallback } from "./compose-refs"; + +/** + * The contract under test: both refs always see the same node, across the + * three shapes an incoming ref can take (object, plain callback, React 19 + * callback with cleanup). The cleanup case matters most — React never calls + * the callback with `null` when a cleanup was returned, so clearing the local + * ref has to happen inside the merged cleanup or it dangles forever. + */ +describe("composedRefCallback", () => { + const node = { nodeName: "DIV" }; + + it("tracks attach and detach in both an object ref and the local ref", () => { + const outer: { current: typeof node | null } = { current: null }; + const local: { current: typeof node | null } = { current: null }; + const callback = composedRefCallback(outer, local); + + expect(callback(node)).toBeUndefined(); + expect(outer.current).toBe(node); + expect(local.current).toBe(node); + + callback(null); + expect(outer.current).toBeNull(); + expect(local.current).toBeNull(); + }); + + it("forwards attach and detach to a plain callback ref", () => { + const seen: Array = []; + const local: { current: typeof node | null } = { current: null }; + const callback = composedRefCallback((value: typeof node | null) => { + seen.push(value); + }, local); + + expect(callback(node)).toBeUndefined(); + callback(null); + expect(seen).toEqual([node, null]); + expect(local.current).toBeNull(); + }); + + it("preserves a React 19 cleanup and clears the local ref inside it", () => { + let cleanedUp = false; + const local: { current: typeof node | null } = { current: null }; + const callback = composedRefCallback(() => { + return () => { + cleanedUp = true; + }; + }, local); + + const cleanup = callback(node); + expect(local.current).toBe(node); + expect(typeof cleanup).toBe("function"); + + (cleanup as () => void)(); + expect(cleanedUp).toBe(true); + expect(local.current).toBeNull(); + }); + + it("still tracks the local ref when no outer ref is given", () => { + const local: { current: typeof node | null } = { current: null }; + const callback = composedRefCallback(undefined, local); + callback(node); + expect(local.current).toBe(node); + callback(null); + expect(local.current).toBeNull(); + }); +}); diff --git a/packages/react/src/lib/compose-refs.ts b/packages/react/src/lib/compose-refs.ts new file mode 100644 index 000000000..bb8fa300c --- /dev/null +++ b/packages/react/src/lib/compose-refs.ts @@ -0,0 +1,33 @@ +import * as React from "react"; + +/** + * One callback ref that keeps an incoming ref and a local ref on the same node. + * + * A React 19 callback ref may return a cleanup function; when the incoming ref + * does, React runs that cleanup instead of calling the callback again with + * `null`, so the local ref must be cleared inside the cleanup too. + */ +export const composedRefCallback = + (outer: React.Ref | undefined, local: React.RefObject): React.RefCallback => + (node) => { + local.current = node; + if (typeof outer === "function") { + const cleanup = outer(node); + if (typeof cleanup === "function") { + return () => { + local.current = null; + cleanup(); + }; + } + return undefined; + } + if (outer) outer.current = node; + return undefined; + }; + +/** `composedRefCallback`, kept stable across renders for stable inputs so the + * incoming ref is not detached and reattached on every render. */ +export const useComposedRef = ( + outer: React.Ref | undefined, + local: React.RefObject, +): React.RefCallback => React.useMemo(() => composedRefCallback(outer, local), [outer, local]); diff --git a/packages/react/src/lib/outside-dismiss.test.ts b/packages/react/src/lib/outside-dismiss.test.ts index 554713f3d..229c472dc 100644 --- a/packages/react/src/lib/outside-dismiss.test.ts +++ b/packages/react/src/lib/outside-dismiss.test.ts @@ -2,7 +2,9 @@ import { describe, expect, it } from "@effect/vitest"; import { applyOutsideDismissPolicy, + containsFormField, dismissesOnOutsideInteraction, + FORM_FIELD_SELECTOR, PORTALED_POPUP_SELECTOR, } from "./outside-dismiss"; @@ -15,27 +17,56 @@ import { * through here, which is why no case below can close a surface with a key. */ describe("dismissesOnOutsideInteraction", () => { - it("keeps the surface open by default", () => { + it("keeps a surface with a form field open by default", () => { expect( dismissesOnOutsideInteraction({ - dismissOnOutsideClick: false, + dismissOnOutsideClick: undefined, insidePortaledPopup: false, + containsFormField: true, }), ).toBe(false); }); - it("closes the surface when it opts in", () => { + it("closes a surface with no form field by default", () => { + expect( + dismissesOnOutsideInteraction({ + dismissOnOutsideClick: undefined, + insidePortaledPopup: false, + containsFormField: false, + }), + ).toBe(true); + }); + + it("closes a form surface that explicitly opts in", () => { expect( - dismissesOnOutsideInteraction({ dismissOnOutsideClick: true, insidePortaledPopup: false }), + dismissesOnOutsideInteraction({ + dismissOnOutsideClick: true, + insidePortaledPopup: false, + containsFormField: true, + }), ).toBe(true); }); + it("keeps a field-free surface open when it explicitly opts out", () => { + expect( + dismissesOnOutsideInteraction({ + dismissOnOutsideClick: false, + insidePortaledPopup: false, + containsFormField: false, + }), + ).toBe(false); + }); + it("never closes on a click inside a portaled popup, even when it opts in", () => { // A combobox or select renders its list outside the surface, so choosing an // option arrives as an outside interaction. Dismissing there would drop the // selection before it lands. expect( - dismissesOnOutsideInteraction({ dismissOnOutsideClick: true, insidePortaledPopup: true }), + dismissesOnOutsideInteraction({ + dismissOnOutsideClick: true, + insidePortaledPopup: true, + containsFormField: false, + }), ).toBe(false); }); }); @@ -54,18 +85,93 @@ const outsideEvent = () => { }; }; +/** A stand-in for the surface node, recording what it was asked for. */ +const surfaceStub = (options: { readonly hasFormField: boolean }) => { + const queries: Array = []; + return { + surface: { + querySelector: (selectors: string) => { + queries.push(selectors); + return options.hasFormField ? { nodeName: "INPUT" } : null; + }, + }, + queries: () => queries, + }; +}; + describe("applyOutsideDismissPolicy", () => { - it("blocks a plain outside click by default", () => { + it("blocks a plain outside click while the surface holds a form field", () => { const outside = outsideEvent(); - applyOutsideDismissPolicy(outside.event, false); + const stub = surfaceStub({ hasFormField: true }); + applyOutsideDismissPolicy(outside.event, undefined, stub.surface); expect(outside.prevented()).toBe(true); + expect(stub.queries()).toEqual([FORM_FIELD_SELECTOR]); + }); + + it("lets a plain outside click through when the surface has no form field", () => { + const outside = outsideEvent(); + const stub = surfaceStub({ hasFormField: false }); + applyOutsideDismissPolicy(outside.event, undefined, stub.surface); + expect(outside.prevented()).toBe(false); }); - it("lets a plain outside click through when the surface opts in", () => { + it("lets an outside click through when a form surface opts in", () => { const outside = outsideEvent(); - applyOutsideDismissPolicy(outside.event, true); + const stub = surfaceStub({ hasFormField: true }); + applyOutsideDismissPolicy(outside.event, true, stub.surface); expect(outside.prevented()).toBe(false); }); + + it("blocks an outside click when a field-free surface opts out", () => { + const outside = outsideEvent(); + const stub = surfaceStub({ hasFormField: false }); + applyOutsideDismissPolicy(outside.event, false, stub.surface); + expect(outside.prevented()).toBe(true); + }); + + it("blocks the click when the surface node is not mounted yet", () => { + // No node means no way to prove the surface is safe to discard, so the + // policy falls back to keeping it open. + const outside = outsideEvent(); + applyOutsideDismissPolicy(outside.event, undefined, null); + expect(outside.prevented()).toBe(true); + }); +}); + +describe("containsFormField", () => { + it("reports a field when the surface query matches", () => { + expect(containsFormField(surfaceStub({ hasFormField: true }).surface)).toBe(true); + }); + + it("reports no field for a missing surface", () => { + expect(containsFormField(null)).toBe(false); + }); +}); + +describe("FORM_FIELD_SELECTOR", () => { + it("covers native fields, contenteditable, and ARIA widget roles", () => { + // base-ui and Radix render select/combobox triggers, checkboxes, switches, + // radios, and sliders as buttons whose only signature is the ARIA role, so + // every one of these must stay in the selector or a dialog holding it + // starts dismissing on stray clicks. + for (const part of [ + "input", + "textarea", + "select", + "[contenteditable]:not([contenteditable='false'])", + "[role='checkbox']", + "[role='combobox']", + "[role='listbox']", + "[role='radio']", + "[role='searchbox']", + "[role='slider']", + "[role='spinbutton']", + "[role='switch']", + "[role='textbox']", + ]) { + expect(FORM_FIELD_SELECTOR.split(",")).toContain(part); + } + }); }); describe("PORTALED_POPUP_SELECTOR", () => { diff --git a/packages/react/src/lib/outside-dismiss.ts b/packages/react/src/lib/outside-dismiss.ts index 94e36eaa2..e4df73126 100644 --- a/packages/react/src/lib/outside-dismiss.ts +++ b/packages/react/src/lib/outside-dismiss.ts @@ -3,12 +3,13 @@ * * Radix dismisses an overlay surface when an outside interaction is not * default-prevented. That loses whatever the user typed, and a stray click on - * the page behind a form is easy to make. So the default here is the opposite - * of Radix's: an outside interaction keeps the surface open. Escape and the - * close button are unaffected — they still close. + * the page behind a form is easy to make. So by default the surface decides + * from its own contents: while it holds a form field, an outside interaction + * keeps it open; a surface with nothing to lose (a confirmation, a picker, a + * read-only panel) closes. Escape and the close button are unaffected — they + * still close. * - * A surface with nothing to lose (a confirmation, a picker, a read-only panel) - * opts back in with `dismissOnOutsideClick`. + * `dismissOnOutsideClick` overrides the detection in either direction. */ /** base-ui popups (combobox/select) portal their list OUTSIDE the surface, so a @@ -23,16 +24,49 @@ export const isInsidePortaledPopup = (target: unknown): boolean => target instanceof Element && target.closest(PORTALED_POPUP_SELECTOR) !== null; +/** + * Controls whose presence means an outside click could discard user state. + * + * Tag selectors alone are not enough: base-ui and Radix render select and + * combobox triggers, checkboxes, switches, radios, and sliders as `button` + * elements carrying only an ARIA role, so the roles have to be matched too. + */ +export const FORM_FIELD_SELECTOR = [ + "input", + "textarea", + "select", + "[contenteditable]:not([contenteditable='false'])", + "[role='checkbox']", + "[role='combobox']", + "[role='listbox']", + "[role='radio']", + "[role='searchbox']", + "[role='slider']", + "[role='spinbutton']", + "[role='switch']", + "[role='textbox']", +].join(","); + +/** The one Element method the policy needs, so tests can stub it without a DOM. */ +type SurfaceLike = { querySelector(selectors: string): unknown }; + +/** True when the surface currently holds a form field. */ +export const containsFormField = (surface: SurfaceLike | null): boolean => + surface !== null && surface.querySelector(FORM_FIELD_SELECTOR) !== null; + /** * Whether an outside interaction should close the surface. * * Pure so the decision is testable without a DOM: the caller does the element - * lookup and passes the answer in. + * lookups and passes the answers in. `dismissOnOutsideClick` left undefined + * means "decide from the contents". */ export const dismissesOnOutsideInteraction = (input: { - readonly dismissOnOutsideClick: boolean; + readonly dismissOnOutsideClick: boolean | undefined; readonly insidePortaledPopup: boolean; -}): boolean => input.dismissOnOutsideClick && !input.insidePortaledPopup; + readonly containsFormField: boolean; +}): boolean => + !input.insidePortaledPopup && (input.dismissOnOutsideClick ?? !input.containsFormField); /** The shape Radix hands to `onInteractOutside` and `onPointerDownOutside`. */ type OutsideInteractionEvent = { @@ -40,14 +74,20 @@ type OutsideInteractionEvent = { readonly preventDefault: () => void; }; -/** Apply the policy to a Radix outside-interaction event. */ +/** Apply the policy to a Radix outside-interaction event. Radix dispatches the + * event on the OUTSIDE element, so the surface node must be passed in. A + * missing node cannot prove the surface is safe to discard, so it stays open. */ export const applyOutsideDismissPolicy = ( event: OutsideInteractionEvent, - dismissOnOutsideClick: boolean, + dismissOnOutsideClick: boolean | undefined, + surface: SurfaceLike | null, ): void => { - const dismisses = dismissesOnOutsideInteraction({ - dismissOnOutsideClick, - insidePortaledPopup: isInsidePortaledPopup(event.detail.originalEvent.target), - }); + const dismisses = + surface !== null && + dismissesOnOutsideInteraction({ + dismissOnOutsideClick, + insidePortaledPopup: isInsidePortaledPopup(event.detail.originalEvent.target), + containsFormField: containsFormField(surface), + }); if (!dismisses) event.preventDefault(); };