From bac0d75451372047ec8f7755d2cee90cbf76c95e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E9=9B=B7=E7=94=B5=E8=8A=BD=E8=A1=A3?= Date: Fri, 9 Oct 2026 10:16:13 -0400 Subject: [PATCH] Q199: dialogs keep the keyboard inside, close on Escape, and give focus back Co-Authored-By: Claude Opus 5.5 --- src/renderer/src/app/App.tsx | 7 +- src/renderer/src/app/StaleBanner.tsx | 7 +- .../src/pages/approvals/ApprovalDialogs.tsx | 13 +-- .../src/pages/work/TaskDetailDialog.tsx | 16 ++-- src/renderer/src/shared/Dialog.tsx | 26 ++---- src/renderer/src/shared/DiscardConfirm.tsx | 18 ++++ src/renderer/src/shared/FailureToasts.tsx | 7 +- src/renderer/src/shared/Modal.tsx | 93 +++++++++++++++++++ src/renderer/src/shared/SearchDialog.tsx | 19 +--- tests/e2e/keyboard.spec.ts | 61 ++++++++++++ 10 files changed, 212 insertions(+), 55 deletions(-) create mode 100644 src/renderer/src/shared/DiscardConfirm.tsx create mode 100644 src/renderer/src/shared/Modal.tsx create mode 100644 tests/e2e/keyboard.spec.ts diff --git a/src/renderer/src/app/App.tsx b/src/renderer/src/app/App.tsx index 3bb4406d..a48450c7 100644 --- a/src/renderer/src/app/App.tsx +++ b/src/renderer/src/app/App.tsx @@ -1,3 +1,4 @@ +import { Modal } from "../shared/Modal"; import { StaleBanner } from "./StaleBanner"; import { oneOf, readViewState, seedViewState, writeViewState } from "../shared/view-state"; import { LeaveGuardContext } from "./leave-guard"; @@ -134,8 +135,8 @@ export function App() { {/* A failed manual retry is already recorded in syncFailure, which the banner shows. */} {syncFailure && void actions.reload().catch(() => undefined)} />} {leaving && ( -
-
+ setLeaving(null)}> +

{t("Leave this page?")}

{t("What you typed here is lost if you leave.")}

@@ -143,7 +144,7 @@ export function App() {
-
+ )} ); diff --git a/src/renderer/src/app/StaleBanner.tsx b/src/renderer/src/app/StaleBanner.tsx index 689d696b..7521cb88 100644 --- a/src/renderer/src/app/StaleBanner.tsx +++ b/src/renderer/src/app/StaleBanner.tsx @@ -1,3 +1,4 @@ +import { createPortal } from "react-dom"; import { Button } from "../shared/ui"; import { t } from "../i18n/i18n"; import { formatTime } from "../pages/types"; @@ -5,7 +6,8 @@ import type { SyncFailure } from "./useSnapshot"; /** Says the data on screen is no longer current: from when it is, why updating failed, and that it keeps retrying. */ export function StaleBanner({ failure, syncedAt, onRetry }: { failure: SyncFailure; syncedAt: string | null; onRetry: () => void }) { - return ( + // Outside the app root, so it stays usable while a dialog makes the app inert. + return createPortal(
{t("Not up to date.")}{" "} @@ -13,6 +15,7 @@ export function StaleBanner({ failure, syncedAt, onRetry }: { failure: SyncFailu {t("Updating failed: {reason} (attempt {count}).", { reason: failure.message, count: failure.attempts })}
-
+
, + document.body, ); } diff --git a/src/renderer/src/pages/approvals/ApprovalDialogs.tsx b/src/renderer/src/pages/approvals/ApprovalDialogs.tsx index 1f18a1c4..f0bc2746 100644 --- a/src/renderer/src/pages/approvals/ApprovalDialogs.tsx +++ b/src/renderer/src/pages/approvals/ApprovalDialogs.tsx @@ -1,3 +1,4 @@ +import { Modal } from "../../shared/Modal"; import { approvalKindLabel } from "../../i18n/labels"; import { useState } from "react"; import { Button, IconButton, Pill } from "../../shared/ui"; @@ -37,8 +38,8 @@ export function ApprovalDetailDialog({ const tone = RISK_TONES[approval.risk]; if (!tone) throw new Error("Approval risk tone is missing."); return ( -
-
event.stopPropagation()}> + +

{approval.title}

@@ -84,7 +85,7 @@ export function ApprovalDetailDialog({
-
+ ); } @@ -105,8 +106,8 @@ export function ApprovalRecordDialog({ approval, requester, decider, drift, onCl if (!approval) return null; const outcome = approval.state === APPROVAL_STATE.APPROVED ? t("Approved") : approval.state === APPROVAL_STATE.SUPERSEDED ? t("Replaced by an edit") : t("Declined"); return ( -
-
event.stopPropagation()}> + +

{approval.title}

@@ -132,6 +133,6 @@ export function ApprovalRecordDialog({ approval, requester, decider, drift, onCl )}
-
+ ); } diff --git a/src/renderer/src/pages/work/TaskDetailDialog.tsx b/src/renderer/src/pages/work/TaskDetailDialog.tsx index fd64e39d..879f8301 100644 --- a/src/renderer/src/pages/work/TaskDetailDialog.tsx +++ b/src/renderer/src/pages/work/TaskDetailDialog.tsx @@ -1,3 +1,5 @@ +import { DiscardConfirm } from "../../shared/DiscardConfirm"; +import { Modal } from "../../shared/Modal"; import { useUnsaved } from "../../shared/unsaved"; import { useBusySubmit } from "../../shared/useBusySubmit"; import { approvalStateLabel, runTriggerLabel, taskEventLabel } from "../../i18n/labels"; @@ -141,15 +143,9 @@ export function TaskDetailDialog({ const requestClose = () => (editing && editDirty ? setConfirmClose(true) : onClose()); return ( -
-
e.stopPropagation()}> - {confirmClose && ( -
- {t("Discard your changes?")} - - -
- )} + +
+ {confirmClose && setConfirmClose(false)} onDiscard={onClose} />}
{task.priority === "high" && }

{task.title}

@@ -234,7 +230,7 @@ export function TaskDetailDialog({ /> )}
-
+ ); } diff --git a/src/renderer/src/shared/Dialog.tsx b/src/renderer/src/shared/Dialog.tsx index 79e975cd..1368a73f 100644 --- a/src/renderer/src/shared/Dialog.tsx +++ b/src/renderer/src/shared/Dialog.tsx @@ -1,3 +1,5 @@ +import { DiscardConfirm } from "./DiscardConfirm"; +import { Modal } from "./Modal"; import { useState, type FormEvent, type ReactNode } from "react"; import { useUnsaved } from "./unsaved"; import { useBusySubmit } from "./useBusySubmit"; @@ -25,24 +27,16 @@ function DialogBody({ title, children, submitLabel, onClose, onSubmit, hideSubmi useUnsaved(edited); const requestClose = () => (edited ? setConfirming(true) : onClose()); return ( -
-
event.stopPropagation()} onInput={() => setEdited(true)} onChange={() => setEdited(true)} onSubmit={submit}> + + setEdited(true)} onChange={() => setEdited(true)} onSubmit={submit}>

{title}

{children}
- {confirming ? ( - // Keyed rows remount, so a clicked plain button is never reused as the submit button (whose default action would then submit). -
- {t("Discard your changes?")} - - -
- ) : ( -
- - {!hideSubmit && } -
- )} +
+ + {!hideSubmit && } +
-
+ {confirming && setConfirming(false)} onDiscard={onClose} />} + ); } diff --git a/src/renderer/src/shared/DiscardConfirm.tsx b/src/renderer/src/shared/DiscardConfirm.tsx new file mode 100644 index 00000000..3f36f9e9 --- /dev/null +++ b/src/renderer/src/shared/DiscardConfirm.tsx @@ -0,0 +1,18 @@ +import { Modal } from "./Modal"; +import { Button } from "./ui"; +import { t } from "../i18n/i18n"; + +/** Asks before typed input is thrown away, as its own layer above the dialog; Escape keeps editing. */ +export function DiscardConfirm({ onKeep, onDiscard }: { onKeep: () => void; onDiscard: () => void }) { + return ( + +
+

{t("Discard your changes?")}

+
+ + +
+
+ + ); +} diff --git a/src/renderer/src/shared/FailureToasts.tsx b/src/renderer/src/shared/FailureToasts.tsx index c1d8d97c..76702743 100644 --- a/src/renderer/src/shared/FailureToasts.tsx +++ b/src/renderer/src/shared/FailureToasts.tsx @@ -1,4 +1,5 @@ import { useEffect, useState } from "react"; +import { createPortal } from "react-dom"; import { IconButton } from "./ui"; import { t } from "../i18n/i18n"; @@ -23,7 +24,8 @@ export function FailureToasts() { }; }, []); if (failures.length === 0) return null; - return ( + // Outside the app root, so a failure can be read and dismissed while a dialog makes the app inert. + return createPortal(
{failures.map((failure) => (
@@ -34,6 +36,7 @@ export function FailureToasts() { setFailures((current) => current.filter((item) => item.id !== failure.id))} />
))} -
+
, + document.body, ); } diff --git a/src/renderer/src/shared/Modal.tsx b/src/renderer/src/shared/Modal.tsx new file mode 100644 index 00000000..756c4f21 --- /dev/null +++ b/src/renderer/src/shared/Modal.tsx @@ -0,0 +1,93 @@ +import { useEffect, useRef, useState, type CSSProperties, type ReactNode } from "react"; +import { createPortal } from "react-dom"; + +const FOCUSABLE = "a[href], button:not([disabled]), input:not([disabled]):not([type=hidden]), select:not([disabled]), textarea:not([disabled]), [tabindex]:not([tabindex='-1'])"; + +/** Open modals, innermost last; everything outside the innermost one is inert. */ +const stack: HTMLElement[] = []; + +function syncInert(): void { + const root = document.getElementById("root"); + const top = stack[stack.length - 1] ?? null; + if (root) root.inert = top !== null; + for (const layer of stack) layer.inert = layer !== top; +} + +function focusables(node: HTMLElement): HTMLElement[] { + return [...node.querySelectorAll(FOCUSABLE)].filter((element) => element.offsetParent !== null || element === document.activeElement); +} + +/** A modal layer above an inert app: focus starts and stays inside, Escape asks it to close, focus returns on close. */ +export function Modal({ label, onEscape, onBackdrop, role = "dialog", backdropStyle, children }: { + label: string; + onEscape: () => void; + /** A mouse-down on the backdrop itself, outside the content. */ + onBackdrop?: () => void; + role?: "dialog" | "alertdialog"; + backdropStyle?: CSSProperties; + children: ReactNode; +}) { + const layerRef = useRef(null); + // Read at the first render, before a field inside takes focus with autoFocus. + const [opener] = useState(() => (document.activeElement instanceof HTMLElement ? document.activeElement : null)); + const escapeRef = useRef(onEscape); + useEffect(() => { + escapeRef.current = onEscape; + }, [onEscape]); + + useEffect(() => { + const layer = layerRef.current; + if (!layer) return; + stack.push(layer); + syncInert(); + if (!layer.contains(document.activeElement)) (focusables(layer)[0] ?? layer).focus(); + const onKeyDown = (event: KeyboardEvent) => { + if (event.key === "Escape") { + event.stopPropagation(); + escapeRef.current(); + return; + } + if (event.key !== "Tab") return; + const items = focusables(layer); + if (items.length === 0) { + event.preventDefault(); + return; + } + const first = items[0]!; + const last = items[items.length - 1]!; + if (event.shiftKey && (document.activeElement === first || !layer.contains(document.activeElement))) { + event.preventDefault(); + last.focus(); + } else if (!event.shiftKey && document.activeElement === last) { + event.preventDefault(); + first.focus(); + } + }; + layer.addEventListener("keydown", onKeyDown); + return () => { + layer.removeEventListener("keydown", onKeyDown); + stack.splice(stack.indexOf(layer), 1); + syncInert(); + // After every layer closing in this commit has left the stack, so the opener is no longer inert. + queueMicrotask(() => { + if (opener && document.contains(opener)) opener.focus(); + }); + }; + }, [opener]); + + return createPortal( +
{ if (event.target === event.currentTarget) onBackdrop?.(); }} + > + {children} +
, + document.body, + ); +} diff --git a/src/renderer/src/shared/SearchDialog.tsx b/src/renderer/src/shared/SearchDialog.tsx index c36dad75..175d966b 100644 --- a/src/renderer/src/shared/SearchDialog.tsx +++ b/src/renderer/src/shared/SearchDialog.tsx @@ -1,3 +1,4 @@ +import { Modal } from "./Modal"; import { withNextPage, type SearchAnswer } from "./search-pages"; import { useEffect, useMemo, useState, type CSSProperties, type FormEvent } from "react"; import type { AppSnapshot, SearchEntityType, SearchResultRecord } from "../../../shared/domain"; @@ -87,14 +88,6 @@ export function SearchDialog({ }; }, [employeeId, filter, open, orgId, trimmed]); - useEffect(() => { - if (!open) return; - const handler = (event: KeyboardEvent) => { - if (event.key === "Escape") onClose(); - }; - window.addEventListener("keydown", handler); - return () => window.removeEventListener("keydown", handler); - }, [onClose, open]); if (!open) return null; // The next page joins the shown one only if the words are still the same. @@ -124,13 +117,7 @@ export function SearchDialog({ }; return ( -
{ if (event.target === event.currentTarget) onClose(); }} - > +
@@ -184,7 +171,7 @@ export function SearchDialog({ )}
-
+ ); } diff --git a/tests/e2e/keyboard.spec.ts b/tests/e2e/keyboard.spec.ts new file mode 100644 index 00000000..d21afc7f --- /dev/null +++ b/tests/e2e/keyboard.spec.ts @@ -0,0 +1,61 @@ +import { _electron as electron, expect, test } from "@playwright/test"; +import { mkdirSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join, resolve } from "node:path"; +import { randomUUID } from "node:crypto"; +import { seedBuiltinSkillsFixture } from "./builtin-skills-fixture"; +import { closeApp, onboard } from "./onboard"; + +test("a dialog keeps the keyboard inside it, closes on Escape, and gives focus back", async () => { + const profileDir = join(tmpdir(), `agentcompany-new-e2e-keys-${randomUUID()}`); + mkdirSync(profileDir, { recursive: true }); + const builtinDir = seedBuiltinSkillsFixture(join(tmpdir(), `agentcompany-new-e2e-keys-builtin-${randomUUID()}`)); + const app = await electron.launch({ + args: [resolve("out/main/index.js")], + env: { ...process.env, AGENTCOMPANY_NEW_PROFILE_DIR: profileDir, AGENTCOMPANY_NEW_AUTORUN: "0", AGENTCOMPANY_NEW_BUILTIN_SKILLS_DIR: builtinDir }, + }); + try { + const page = await app.firstWindow(); + await onboard(page, "Keys Co"); + await page.getByRole("button", { name: "Work" }).click(); + const opener = page.getByRole("button", { name: "New task" }); + await opener.click(); + const dialog = page.getByRole("dialog", { name: "New task" }); + await expect(dialog).toBeVisible(); + const focusInDialog = () => page.evaluate(() => Boolean(document.activeElement?.closest("[aria-modal=true]"))); + // Focus starts inside, and the app behind cannot be reached. + expect(await focusInDialog()).toBe(true); + expect(await page.evaluate(() => document.getElementById("root")!.inert)).toBe(true); + for (let press = 0; press < 25; press += 1) { + await page.keyboard.press("Tab"); + expect(await focusInDialog()).toBe(true); + } + for (let press = 0; press < 25; press += 1) { + await page.keyboard.press("Shift+Tab"); + expect(await focusInDialog()).toBe(true); + } + // Escape closes an untouched dialog and focus returns to what opened it. + await page.keyboard.press("Escape"); + await expect(dialog).toHaveCount(0); + await expect(opener).toBeFocused(); + expect(await page.evaluate(() => document.getElementById("root")!.inert)).toBe(false); + // With something typed, Escape asks first. + await opener.click(); + await dialog.getByLabel("Title").fill("Draft"); + await page.keyboard.press("Escape"); + // The question is its own layer: focus moves into it, and Escape there keeps editing. + const confirm = page.getByRole("alertdialog", { name: "Discard your changes?" }); + await expect(confirm).toBeVisible(); + expect(await page.evaluate(() => Boolean(document.activeElement?.closest("[role=alertdialog]")))).toBe(true); + await page.keyboard.press("Escape"); + await expect(confirm).toHaveCount(0); + await expect(dialog.getByLabel("Title")).toBeFocused(); + await page.keyboard.press("Escape"); + await confirm.getByRole("button", { name: "Discard" }).click(); + await expect(dialog).toHaveCount(0); + // Both layers closed at once; focus still returns to what opened the dialog. + await expect(opener).toBeFocused(); + } finally { + await closeApp(app); + } +});