diff --git a/apps/desktop/src/App.vue b/apps/desktop/src/App.vue index f0280cdb72..ea35af060b 100644 --- a/apps/desktop/src/App.vue +++ b/apps/desktop/src/App.vue @@ -175,6 +175,7 @@ import { savedSqlErrorMessage } from "@/lib/savedSql/savedSqlErrors"; import { savedSqlDefaultTargetForWrite } from "@/lib/savedSql/savedSqlExecutionTarget"; import { countActiveUpdateBlockingTasks } from "@/lib/app/appUpdateTaskGuard"; import { initSavedSqlEditorPositions } from "@/lib/app/savedSqlEditorPosition"; +import { dismissOpenModalSurface } from "@/lib/ui/modalSurface"; import { hasTreeNodeDatabaseContext } from "@/lib/sidebar/treeNodeContext"; import { objectBrowserTablesToAiTreeNodes } from "@/lib/ai/objectBrowserToAiTargets"; import { aiTargetFromTab, type AiConversationBinding } from "@/lib/ai/aiConversationBinding"; @@ -4203,6 +4204,10 @@ function refreshPluginWorkbenches(pluginId: string): void { } async function closeActiveSurface() { + // Cmd+W (and the native macOS "Close Tab" menu item) must not reach the surface behind + // an open modal dialog. While a dialog owns the window the keystroke dismisses that + // dialog instead, exactly like Esc, so the shortcut never goes dead on the user. + if (dismissOpenModalSurface()) return; if (showSettingsPage.value) { closeSettingsPage(); } else if (showPluginCenter.value) { diff --git a/apps/desktop/src/lib/ui/__tests__/modalSurface.spec.ts b/apps/desktop/src/lib/ui/__tests__/modalSurface.spec.ts new file mode 100644 index 0000000000..43ea6c6896 --- /dev/null +++ b/apps/desktop/src/lib/ui/__tests__/modalSurface.spec.ts @@ -0,0 +1,139 @@ +// @vitest-environment happy-dom + +import { createApp, defineComponent, h, nextTick, ref, type App, type Component } from "vue"; +import { afterEach, describe, expect, it } from "vitest"; +import { Dialog, DialogContent, DialogScrollContent } from "@/components/ui/dialog"; +import { Popover, PopoverContent } from "@/components/ui/popover"; +import { dismissOpenModalSurface, hasOpenModalSurface } from "@/lib/ui/modalSurface"; + +const mountedApps: App[] = []; + +afterEach(() => { + for (const app of mountedApps.splice(0)) app.unmount(); + document.body.innerHTML = ""; +}); + +function mount(component: Component) { + const app = createApp(component); + mountedApps.push(app); + app.mount(document.createElement("div")); + return app; +} + +describe("hasOpenModalSurface", () => { + it("detects the overlay of an open dialog", async () => { + const open = ref(true); + mount(defineComponent({ setup: () => () => h(Dialog, { open: open.value }, () => h(DialogContent, {}, () => h("p", "body"))) })); + await nextTick(); + + const overlay = document.querySelector('[data-slot="dialog-overlay"]'); + expect(overlay?.getAttribute("data-state")).toBe("open"); + expect(hasOpenModalSurface()).toBe(true); + + open.value = false; + await nextTick(); + expect(hasOpenModalSurface()).toBe(false); + }); + + it("ignores an overlay that is only finishing its exit animation", () => { + document.body.innerHTML = '
'; + expect(hasOpenModalSurface()).toBe(false); + }); + + it("ignores non-blocking dialogs that render no overlay", async () => { + mount( + defineComponent({ + setup: () => () => h(Dialog, { open: true, modal: false }, () => h(DialogScrollContent, { showOverlay: false }, () => h("p", "body"))), + }), + ); + await nextTick(); + + const content = document.querySelector('[data-slot="dialog-content"]'); + expect(content?.getAttribute("data-state")).toBe("open"); + expect(document.querySelector('[data-slot="dialog-overlay"]')).toBeNull(); + expect(hasOpenModalSurface()).toBe(false); + }); + + it("ignores open popovers that reuse role=dialog", async () => { + mount(defineComponent({ setup: () => () => h(Popover, { open: true }, () => h(PopoverContent, {}, () => h("p", "body"))) })); + await nextTick(); + + const popover = document.querySelector('[data-slot="popover-content"]'); + expect(popover?.getAttribute("role")).toBe("dialog"); + expect(popover?.getAttribute("data-state")).toBe("open"); + expect(hasOpenModalSurface()).toBe(false); + }); + + it("honours hand-rolled modal opt-in and peek opt-out", () => { + document.body.innerHTML = '
'; + expect(hasOpenModalSurface()).toBe(true); + + document.body.innerHTML = '
'; + expect(hasOpenModalSurface()).toBe(false); + + document.body.innerHTML = '
'; + expect(hasOpenModalSurface()).toBe(false); + }); + + it("stays false without a queryable root", () => { + expect(hasOpenModalSurface(null)).toBe(false); + expect(hasOpenModalSurface({} as ParentNode)).toBe(false); + }); +}); + +describe("dismissOpenModalSurface", () => { + it("lets the top-most layer take the keystroke first, exactly like Esc", async () => { + const dialogOpen = ref(true); + const popoverOpen = ref(true); + mount( + defineComponent({ + setup: () => () => + h(Dialog, { open: dialogOpen.value, "onUpdate:open": (value: boolean) => (dialogOpen.value = value) }, () => + h(DialogContent, {}, () => [h(Popover, { open: popoverOpen.value, "onUpdate:open": (value: boolean) => (popoverOpen.value = value) }, () => h(PopoverContent, {}, () => h("p", "popover")))]), + ), + }), + ); + await nextTick(); + + expect(dismissOpenModalSurface()).toBe(true); + await nextTick(); + + // The popover above the dialog absorbs the keystroke, so the dialog stays open -- + // same as pressing Esc once. (The next press reaches the dialog once the popover's + // exit animation has unmounted its layer, which happy-dom cannot run.) + expect(popoverOpen.value).toBe(false); + expect(dialogOpen.value).toBe(true); + expect(hasOpenModalSurface()).toBe(true); + }); + + it("closes the dialog that owns the window", async () => { + const open = ref(true); + mount( + defineComponent({ + setup: () => () => h(Dialog, { open: open.value, "onUpdate:open": (value: boolean) => (open.value = value) }, () => h(DialogContent, {}, () => h("p", "body"))), + }), + ); + await nextTick(); + + expect(dismissOpenModalSurface()).toBe(true); + await nextTick(); + + expect(open.value).toBe(false); + expect(hasOpenModalSurface()).toBe(false); + }); + + it("leaves the window alone when no modal is open", () => { + const dispatched: Event[] = []; + const target = { dispatchEvent: (event: Event) => dispatched.push(event) } as unknown as Window; + + expect(dismissOpenModalSurface(target)).toBe(false); + expect(dispatched).toEqual([]); + }); + + it("stays inert without a dispatch target", () => { + document.body.innerHTML = '
'; + + expect(dismissOpenModalSurface(null)).toBe(false); + expect(hasOpenModalSurface()).toBe(true); + }); +}); diff --git a/apps/desktop/src/lib/ui/modalSurface.ts b/apps/desktop/src/lib/ui/modalSurface.ts new file mode 100644 index 0000000000..d768dfbde1 --- /dev/null +++ b/apps/desktop/src/lib/ui/modalSurface.ts @@ -0,0 +1,43 @@ +/** + * Whether a modal layer currently owns the window, so an app-level shortcut such as + * "close tab" (Cmd/Ctrl+W) is handed to that dialog instead of tearing down the surface + * behind it. + * + * Modal Reka dialogs render through `DialogContent.vue` / `DialogScrollContent.vue`, + * which mount `data-slot="dialog-overlay"` with the `data-state` Reka writes onto the + * layer. `data-state="open"` skips a layer that is only on screen for its exit + * animation. + * + * The overlay -- not the dialog body -- is the marker on purpose, because the overlay is + * what actually covers the app (Reka gives it `pointer-events: auto`). The one dialog + * that stays interactive renders no overlay at all: `DialogScrollContent` with + * `:show-overlay="false"`, used by the background table import. `role="dialog"` alone is + * not enough either: Reka popovers, date pickers and similar floating surfaces reuse that + * role and stay open while the user keeps working in the shell. Hand-rolled full-screen + * modals opt in with `aria-modal="true"`, while non-modal peek overlays pass + * `aria-modal="false"`. + */ +const OPEN_MODAL_SURFACE_SELECTOR = '[data-slot="dialog-overlay"][data-state="open"], [role="dialog"][aria-modal="true"]'; + +/** True while a modal dialog layer is open above the current surface. */ +export function hasOpenModalSurface(root: ParentNode | null | undefined = globalThis.document ?? null): boolean { + if (!root || typeof root.querySelector !== "function") return false; + return root.querySelector(OPEN_MODAL_SURFACE_SELECTOR) !== null; +} + +/** + * Hand "close this" to the modal dialog that owns the window, and report whether a modal + * took the shortcut. The keystroke is replayed as a cancelable Escape keydown on the + * window because that is the dismissal path every Reka layer already implements: only the + * top-most layer in the layer stack reacts, layer-specific guards + * (`@escape-key-down`, e.g. cancelling a pending inline edit first) still run, and focus + * returns where users expect it. + * + * So Cmd+W with a dialog open closes that dialog -- an open popover inside it first, the + * dialog itself on the next press -- instead of leaving the user with a dead shortcut. + */ +export function dismissOpenModalSurface(target: Pick | null | undefined = globalThis.window ?? null): boolean { + if (!target || typeof target.dispatchEvent !== "function" || !hasOpenModalSurface()) return false; + target.dispatchEvent(new KeyboardEvent("keydown", { key: "Escape", bubbles: true, cancelable: true })); + return true; +} diff --git a/docs/assets/close-tab-modal-guard/after-cmd-w.jpg b/docs/assets/close-tab-modal-guard/after-cmd-w.jpg new file mode 100644 index 0000000000..8374c450b7 Binary files /dev/null and b/docs/assets/close-tab-modal-guard/after-cmd-w.jpg differ diff --git a/docs/assets/close-tab-modal-guard/before-no-dialog.jpg b/docs/assets/close-tab-modal-guard/before-no-dialog.jpg new file mode 100644 index 0000000000..c74ca45177 Binary files /dev/null and b/docs/assets/close-tab-modal-guard/before-no-dialog.jpg differ diff --git a/docs/assets/close-tab-modal-guard/modal-open.jpg b/docs/assets/close-tab-modal-guard/modal-open.jpg new file mode 100644 index 0000000000..fbb935ba04 Binary files /dev/null and b/docs/assets/close-tab-modal-guard/modal-open.jpg differ diff --git a/packages/app-tests/closeTabModalGuard.test.ts b/packages/app-tests/closeTabModalGuard.test.ts new file mode 100644 index 0000000000..6119b06247 --- /dev/null +++ b/packages/app-tests/closeTabModalGuard.test.ts @@ -0,0 +1,31 @@ +import { strict as assert } from "node:assert"; +import { readFileSync } from "node:fs"; +import { test } from "vitest"; + +const app = readFileSync("apps/desktop/src/App.vue", "utf8"); + +function functionBody(source: string, signature: string): string { + const start = source.indexOf(signature); + assert.notEqual(start, -1, `missing ${signature}`); + const end = source.indexOf("\n}", start); + assert.notEqual(end, -1, `unterminated ${signature}`); + return source.slice(start, end); +} + +test("closing a tab stops at an open modal dialog", () => { + // Cmd+W and the native macOS "Close Tab" menu item both funnel into + // closeActiveSurface, so the modal handling has to live there. + const body = functionBody(app, "async function closeActiveSurface()"); + assert.match(body, /if \(dismissOpenModalSurface\(\)\) return;/); + assert.match(app, /import \{ dismissOpenModalSurface \} from "@\/lib\/ui\/modalSurface";/); + assert.match(app, /if \(isCloseTabShortcut\(e, shortcuts\)\) \{\s*e\.preventDefault\(\);\s*await closeActiveSurface\(\);/); +}); + +test("the modal branch hands the keystroke to the dialog instead of dropping it", () => { + const body = functionBody(app, "async function closeActiveSurface()"); + const guardIndex = body.indexOf("dismissOpenModalSurface()"); + const settingsIndex = body.indexOf("showSettingsPage.value"); + assert.notEqual(guardIndex, -1, "closeActiveSurface must dismiss an open modal"); + assert.ok(guardIndex < settingsIndex, "the modal must be dismissed before the surface behind it closes"); + assert.match(body, /if \(dismissOpenModalSurface\(\)\) return;/); +});