diff --git a/web_ui/src/app/canvas/CodeExecutionApprovalPanel.test.tsx b/web_ui/src/app/canvas/CodeExecutionApprovalPanel.test.tsx index 12f1e5d..67ce301 100644 --- a/web_ui/src/app/canvas/CodeExecutionApprovalPanel.test.tsx +++ b/web_ui/src/app/canvas/CodeExecutionApprovalPanel.test.tsx @@ -234,4 +234,53 @@ describe("CodeExecutionApprovalPanel", () => { expect(screen.getByRole("button", { name: "Approve" })).toBeEnabled(); expect(screen.getByRole("button", { name: "Deny" })).toBeEnabled(); }); + + // -- R8a finding #17: focus trap must exclude disabled buttons ------------ + + it("R8a finding #17: while busy, Tab from the code display does not leak focus out of the panel", () => { + // Before this fix, FOCUSABLE included disabled buttons: with busy=true + // both Deny and Approve are disabled, so "last" was the disabled + // Approve button. Tab from the code display (first, not last) never + // matched the wrap condition, preventDefault() never fired, and the + // browser's own native Tab order - which correctly skips disabled + // buttons - walked straight out of the panel (its only other two + // stops both being disabled) onto whatever's next in the document. + // This is the one thing this panel exists to make impossible during a + // mandatory security approval. + renderPanel({ busy: true }); + const codeDisplay = document.querySelector(".code-exec-approval-code") as HTMLElement; + const dialog = screen.getByRole("dialog"); + codeDisplay.focus(); + expect(document.activeElement).toBe(codeDisplay); + + fireEvent.keyDown(dialog, { key: "Tab" }); + + expect(dialog.contains(document.activeElement)).toBe(true); + }); + + it("R8a finding #17: a focus escape past the trap is pulled back into the panel (backstop)", () => { + // Simulates a leak the primary trap missed by some cause other than the + // now-fixed disabled-button one. This panel shares overlays.tsx's own + // Tab-gated useFocusEscapeBackstop (see that hook's doc comment) rather + // than a hand-written unconditional listener - an earlier draft of this + // fix used an unconditional one and infinite-looped the instant two + // panels were open at once (see the "FIX B" test below), each treating + // the other's redirect as its own escape. The gating means the backstop + // only acts right after a real Tab keydown - fireEvent.keyDown(document, + // ...), not user.keyboard, for the same reason as overlays.test.tsx's + // twin of this test: dispatching directly at document arms the flag via + // this hook's capture-phase listener without also reaching the panel's + // own bubble-phase trap, isolating "Tab pressed but the primary trap + // missed it" from "Tab was handled correctly." + renderPanel({ busy: false }); + const outside = document.createElement("button"); + document.body.appendChild(outside); + try { + fireEvent.keyDown(document, { key: "Tab" }); + outside.focus(); + expect(screen.getByRole("dialog").contains(document.activeElement)).toBe(true); + } finally { + outside.remove(); + } + }); }); diff --git a/web_ui/src/app/canvas/CodeExecutionApprovalPanel.tsx b/web_ui/src/app/canvas/CodeExecutionApprovalPanel.tsx index 7df23d0..f97ab9b 100644 --- a/web_ui/src/app/canvas/CodeExecutionApprovalPanel.tsx +++ b/web_ui/src/app/canvas/CodeExecutionApprovalPanel.tsx @@ -3,6 +3,7 @@ import { createPortal } from "react-dom"; import ReactMarkdown from "react-markdown"; import remarkGfm from "remark-gfm"; import rehypeHighlight from "rehype-highlight"; +import { useFocusEscapeBackstop } from "../overlays/overlays"; /** * The mandatory human-approval gate (Qt-removal plan R5.4, corrected in the @@ -139,7 +140,16 @@ function toPythonFence(code: string): string { // Deny, Approve) - deliberately narrower than overlays.tsx's own shared // FOCUSABLE selector (which also matches inputs/selects/links/textareas), // since nothing else in this panel's markup is ever meant to receive focus. -const FOCUSABLE = 'button, [tabindex]:not([tabindex="-1"])'; +// +// R8a (UI/UX issue list finding #17): excludes [disabled], the same fix +// overlays.tsx's own Dialog needed and for the identical reason - `busy` +// (below) disables BOTH Deny and Approve simultaneously while a click is +// in flight, and an undisabled trap boundary computed against a disabled +// "last" button never actually receives Tab natively, so the wrap-around +// branch below silently never fires and focus walks out of a panel whose +// entire reason to exist is that focus must NEVER leave it during a +// mandatory code-execution approval. +const FOCUSABLE = 'button:not([disabled]), [tabindex]:not([tabindex="-1"])'; export interface CodeExecutionApprovalPanelProps { /** No longer used to register with any shared overlay registry (there is @@ -219,6 +229,18 @@ export function CodeExecutionApprovalPanel({ return () => panel.removeEventListener("keydown", onKeyDown); }, [awaitingApproval]); + // R8a (finding #17): shares overlays.tsx's own backstop rather than a + // second hand-written copy - see that hook's own doc comment for why an + // unconditional version (which THIS file tried first) is actually wrong + // here specifically: two of these panels can be open at once for + // different nodes (FIX B, this file's own module doc), and an + // unconditional "redirect whenever focus lands outside, full stop" + // listener on each one infinite-loops the instant either redirects, + // since each treats the other's redirect as its own escape and redirects + // back. The shared hook's Tab-gating avoids this generally, not just for + // Dialog's own close()-restores-focus race. + useFocusEscapeBackstop(panelRef, awaitingApproval, FOCUSABLE); + if (!awaitingApproval) return null; const showRequirements = kind === "code_sandbox" && !!requirements?.trim(); diff --git a/web_ui/src/app/canvas/GroupNodeView.test.tsx b/web_ui/src/app/canvas/GroupNodeView.test.tsx index 73c6074..0e94b92 100644 --- a/web_ui/src/app/canvas/GroupNodeView.test.tsx +++ b/web_ui/src/app/canvas/GroupNodeView.test.tsx @@ -82,7 +82,11 @@ describe("GroupNodeView (frame)", () => { fireEvent.doubleClick(container.querySelector(".group-node-header")!); const input = screen.getByRole("textbox") as HTMLInputElement; fireEvent.change(input, { target: { value: "Throwaway" } }); - fireEvent.keyDown(input, { key: "Escape" }); + // R8a finding #16: must claim the event (preventDefault -> dispatchEvent + // returns false) or overlays.tsx's document-level Escape handler would + // also close whatever dialog/popover is open elsewhere - see + // NoteNodeView's identical guard and overlays.tsx's own comment. + expect(fireEvent.keyDown(input, { key: "Escape" })).toBe(false); expect(onSetLabel).not.toHaveBeenCalled(); expect(screen.queryByRole("textbox")).toBeNull(); diff --git a/web_ui/src/app/canvas/GroupNodeView.tsx b/web_ui/src/app/canvas/GroupNodeView.tsx index 539429a..ade5d49 100644 --- a/web_ui/src/app/canvas/GroupNodeView.tsx +++ b/web_ui/src/app/canvas/GroupNodeView.tsx @@ -78,6 +78,11 @@ export function GroupNodeView({ id, data, selected }: NodeProps) function onKeyDown(event: React.KeyboardEvent) { if (event.key === "Escape") { + // R8a (UI/UX issue list finding #16): claims Escape so overlays.tsx's + // own document-level handler doesn't also close an unrelated open + // popover behind this frame/container - see NoteNodeView's identical + // guard for the full reasoning. + event.preventDefault(); skipBlurRef.current = true; setDraft(data.label); setEditing(false); diff --git a/web_ui/src/app/canvas/NoteNodeView.test.tsx b/web_ui/src/app/canvas/NoteNodeView.test.tsx index ee0660b..e8e6064 100644 --- a/web_ui/src/app/canvas/NoteNodeView.test.tsx +++ b/web_ui/src/app/canvas/NoteNodeView.test.tsx @@ -67,7 +67,13 @@ describe("NoteNodeView", () => { const textarea = screen.getByRole("textbox") as HTMLTextAreaElement; fireEvent.change(textarea, { target: { value: "throwaway edit" } }); - fireEvent.keyDown(textarea, { key: "Escape" }); + // fireEvent's own return value is the DOM dispatchEvent() result - false + // means something called preventDefault(). R8a finding #16: this MUST + // be false, or overlays.tsx's document-level Escape handler (which now + // checks event.defaultPrevented before closing anything) would also + // close whatever dialog/popover happens to be open elsewhere the + // instant a note revert fires - see overlays.tsx's own comment. + expect(fireEvent.keyDown(textarea, { key: "Escape" })).toBe(false); expect(onSetContent).not.toHaveBeenCalled(); expect(screen.queryByRole("textbox")).toBeNull(); diff --git a/web_ui/src/app/canvas/NoteNodeView.tsx b/web_ui/src/app/canvas/NoteNodeView.tsx index d6a9692..23a059d 100644 --- a/web_ui/src/app/canvas/NoteNodeView.tsx +++ b/web_ui/src/app/canvas/NoteNodeView.tsx @@ -68,6 +68,13 @@ export function NoteNodeView({ data, selected }: NodeProps) { function onKeyDown(event: React.KeyboardEvent) { if (event.key === "Escape") { + // R8a (UI/UX issue list finding #16): claims Escape so the overlay + // system's own document-level handler (overlays.tsx) - which now + // checks event.defaultPrevented before closing anything - doesn't + // also close an unrelated open popover behind this note. Without + // this, Escape while editing a note with e.g. Pins open would revert + // the note AND close Pins in the same keystroke. + event.preventDefault(); skipBlurRef.current = true; setDraft(data.content); setEditing(false); diff --git a/web_ui/src/app/chrome/ChatLibraryDialog.test.tsx b/web_ui/src/app/chrome/ChatLibraryDialog.test.tsx index b58b446..3fb9483 100644 --- a/web_ui/src/app/chrome/ChatLibraryDialog.test.tsx +++ b/web_ui/src/app/chrome/ChatLibraryDialog.test.tsx @@ -214,7 +214,19 @@ describe("ChatLibraryDialog", () => { expect(screen.getByText("First Chat")).toBeInTheDocument(); }); - it("Escape while renaming closes the whole dialog (the app's global Escape-to-close policy) without committing a rename", async () => { + it("Escape while renaming cancels the rename WITHOUT closing the dialog (R8a finding #16)", async () => { + // This test used to assert the opposite - Escape closing the whole + // dialog mid-rename, mislabeled as "the app's global Escape-to-close + // policy" - which was exactly the bug the UI/UX issue list's finding + // #16 named this component as its own worked example of: the rename + // input's own onKeyDown already called event.preventDefault() on + // Escape (see onRenameKeyDown below), but overlays.tsx's document-level + // handler ran in capture phase with an unconditional stopPropagation(), + // so that preventDefault() could never matter - the global handler + // always won and closed the dialog out from under an in-progress + // rename. overlays.tsx now runs on bubble phase and checks + // event.defaultPrevented before closing anything, so this component's + // own already-correct handler finally has a chance to run. const { user, intents } = setup(); await user.click(screen.getByText("open library")); @@ -222,7 +234,15 @@ describe("ChatLibraryDialog", () => { await user.keyboard("{Escape}"); expect(intents.filter((i) => i[1] === "renameChat")).toEqual([]); - expect(screen.queryByRole("dialog")).toBeNull(); + // Reverts exactly like the Cancel (x) button does, not "the dialog is + // now gone" - the rename INPUT is gone (cancelled) and the original + // title is back, but Chat Library stays open underneath it. Checked by + // role, not just the accessible name: the pencil button that RESTARTS + // a rename carries the identical "Rename ..." label, so a label-only + // query would find that instead and pass even if cancelling had failed. + expect(screen.queryByRole("textbox", { name: 'Rename "First Chat"' })).toBeNull(); + expect(screen.getByText("First Chat")).toBeInTheDocument(); + expect(screen.getByRole("dialog")).toBeInTheDocument(); }); it("deleting a row: trash opens an inline scoped confirm, confirming dispatches deleteChat for THAT row only", async () => { diff --git a/web_ui/src/app/chrome/PinOverlay.test.tsx b/web_ui/src/app/chrome/PinOverlay.test.tsx index a9eaebd..b7583a9 100644 --- a/web_ui/src/app/chrome/PinOverlay.test.tsx +++ b/web_ui/src/app/chrome/PinOverlay.test.tsx @@ -82,6 +82,45 @@ describe("PinOverlay", () => { expect(updatePin).toHaveBeenCalledWith("p1", "Renamed", "n"); }); + it("R8a finding #16: Escape while editing a pin cancels the edit WITHOUT closing the whole Pins popover", async () => { + // Neither the title input nor the note textarea had ANY onKeyDown + // before this fix - Escape fell through untouched to overlays.tsx's + // own document-level handler, which closed the entire Popover (the + // same bug class already fixed for Chat Library's rename input, just + // missed here since this file had no existing "Escape" handler to + // find via a grep for one - there was nothing to find, only something + // to add). + const user = userEvent.setup(); + const { store, updatePin } = makeStore([{ id: "p1", title: "Origin", note: "n", x: 0, y: 0 }]); + renderOpen(store); + await user.click(screen.getByLabelText("Edit Origin")); + await user.clear(screen.getByLabelText("Pin title")); + await user.type(screen.getByLabelText("Pin title"), "Throwaway"); + + await user.keyboard("{Escape}"); + + expect(updatePin).not.toHaveBeenCalled(); + // The edit reverted (back to the read-only row, original title intact)... + expect(screen.queryByLabelText("Pin title")).toBeNull(); + expect(screen.getByText("Origin")).toBeInTheDocument(); + // ...and the popover itself is still open, unlike before this fix. + expect(screen.getByText("PINS")).toBeInTheDocument(); + }); + + it("R8a finding #16: Escape in the note textarea also cancels the edit without closing the popover", async () => { + const user = userEvent.setup(); + const { store, updatePin } = makeStore([{ id: "p1", title: "Origin", note: "n", x: 0, y: 0 }]); + renderOpen(store); + await user.click(screen.getByLabelText("Edit Origin")); + + await user.click(screen.getByLabelText("Pin note")); + await user.keyboard("{Escape}"); + + expect(updatePin).not.toHaveBeenCalled(); + expect(screen.queryByLabelText("Pin note")).toBeNull(); + expect(screen.getByText("PINS")).toBeInTheDocument(); + }); + it("remove calls the intent", async () => { const user = userEvent.setup(); const { store, removePin } = makeStore([{ id: "p1", title: "Origin", note: "", x: 0, y: 0 }]); diff --git a/web_ui/src/app/chrome/PinOverlay.tsx b/web_ui/src/app/chrome/PinOverlay.tsx index 1b4fe35..b2b5163 100644 --- a/web_ui/src/app/chrome/PinOverlay.tsx +++ b/web_ui/src/app/chrome/PinOverlay.tsx @@ -58,6 +58,22 @@ export function PinOverlay({ store }: { store: SceneStore }) { setDraftError(null); } + // R8a (UI/UX issue list finding #16): neither the title input nor the + // note textarea below had ANY onKeyDown, so Escape while editing a pin + // fell straight through to overlays.tsx's own document-level handler, + // which closed the whole Pins popover - discarding the entire panel, + // not just cancelling the one pin's edit. Same bug class already fixed + // for Chat Library's rename input and the Note/Frame inline label + // editors, missed here originally since this file has no `key === + // "Escape"` literal to grep for at all - there was no handler to find, + // only one to add. + function onEditKeyDown(event: React.KeyboardEvent) { + if (event.key === "Escape") { + event.preventDefault(); + cancelEditing(); + } + } + function saveEditing() { const trimmedTitle = draftTitle.trim(); if (!trimmedTitle) { @@ -111,6 +127,7 @@ export function PinOverlay({ store }: { store: SceneStore }) { className="pins-edit-title" value={draftTitle} onChange={(e) => setDraftTitle(e.target.value)} + onKeyDown={onEditKeyDown} aria-label="Pin title" autoFocus /> @@ -118,6 +135,7 @@ export function PinOverlay({ store }: { store: SceneStore }) { className="pins-edit-note" value={draftNote} onChange={(e) => setDraftNote(e.target.value)} + onKeyDown={onEditKeyDown} aria-label="Pin note" rows={2} /> diff --git a/web_ui/src/app/overlays/overlays.test.tsx b/web_ui/src/app/overlays/overlays.test.tsx index 9438ce9..b1b3735 100644 --- a/web_ui/src/app/overlays/overlays.test.tsx +++ b/web_ui/src/app/overlays/overlays.test.tsx @@ -1,10 +1,13 @@ -import { render, screen } from "@testing-library/react"; +import { fireEvent, render, screen } from "@testing-library/react"; import userEvent from "@testing-library/user-event"; +import { useState } from "react"; import { describe, expect, it } from "vitest"; import { Dialog, OverlayProvider, Popover, useOverlays } from "./overlays"; function Chrome() { const overlays = useOverlays(); + const [renameValue, setRenameValue] = useState(""); + const [renameCancelled, setRenameCancelled] = useState(false); return (
+ {/* R8a finding #17: a primary action disabled in its default state - + mirrors Settings' own IntegrationsPage/ApiProviderPage/OllamaPage + "Save" buttons, each gated on an empty draft field. Trailing the + real "save" button so it - not this one - must be where Tab + wraps back to "first field". */} +
); @@ -97,6 +123,98 @@ describe("overlay system (the OverlayManager contract)", () => { ); }); + it("R8a finding #17: Tab from the real last-focusable wraps to the first, skipping a disabled trailing button", async () => { + const user = setup(); + await user.click(screen.getByRole("button", { name: /^Settings/ })); + // "disabled action" trails "save" in DOM order - if the trap still + // counted it as focusable, activeElement === lastEl would never be + // true for the real last element (save), the wrap branch would never + // fire, and Tab would leak focus onto whatever's next in the whole + // document outside the panel. + screen.getByRole("button", { name: "save" }).focus(); + await user.keyboard("{Tab}"); + expect(document.activeElement).toBe(screen.getByRole("button", { name: "Close Settings" })); + }); + + it("R8a finding #17: a focus escape past the trap (Tab-driven) is pulled back into the panel", async () => { + // Simulates a leak the trap's own boundary computation missed by some + // OTHER cause than the disabled-button one already fixed above - the + // belt-and-suspenders backstop must not depend on knowing why the + // escape happened, only that a Tab press left focus outside the panel. + // + // fireEvent.keyDown(document, ...), not user.keyboard("{Tab}") - the + // panel's own primary trap is now correct, so a realistic simulated Tab + // would be caught (and consume the backstop's own "was Tab" flag) before + // ever reaching this scenario. Dispatching the keydown directly at + // document reaches this component's document-level capture listener + // (arming the flag) WITHOUT reaching panel's own bubble-phase handler + // (out of the event's path when document itself is the target) - i.e. + // exactly "Tab was pressed, but the primary trap did not handle it", + // the counterfactual this backstop exists for. + const user = setup(); + await user.click(screen.getByRole("button", { name: /^Settings/ })); + const outsideButton = screen.getByRole("button", { name: "elsewhere" }); + + fireEvent.keyDown(document, { key: "Tab" }); + outsideButton.focus(); // what a missed trap boundary would let happen + + expect(screen.getByRole("dialog", { name: "Settings" }).contains(document.activeElement)).toBe( + true, + ); + }); + + it("R8a finding #17: closing a dialog restores focus to the opener even right after a Tab press (no fight with the backstop)", async () => { + // The backstop above must not intercept close()'s own legitimate + // focus-restoration just because the last key happened to be Tab. + // Starts from "first field" (not the dialog's default initial focus, + // the close button) specifically to land the Tab on "save" - a plain + // button with no Escape handling of its own - rather than on "rename + // field", which correctly (finding #16) claims Escape and would leave + // the dialog open, an entirely different, already-covered scenario. + const user = setup(); + const opener = screen.getByRole("button", { name: /^Settings/ }); + await user.click(opener); + await user.click(screen.getByLabelText("first field")); + await user.keyboard("{Tab}"); + expect(document.activeElement).toBe(screen.getByRole("button", { name: "save" })); + await user.keyboard("{Escape}"); + expect(document.activeElement).toBe(opener); + }); + + it("R8a finding #17: closing via a MOUSE click right after a Tab keydown does not fight the backstop either", async () => { + // The scenario the pointerdown reset specifically exists for: a Tab + // keydown with no OTHER intervening key (which would otherwise clear + // the "was Tab" flag on its own, per onKeyDownCapture's own logic - + // see the Escape case above), immediately followed by a mouse click + // on the dialog's own close button, not a keypress. fireEvent, not + // user.keyboard, for the same reason as the escape-past-the-trap test + // above: a realistic Tab is handled correctly now and would consume + // the flag itself, defeating the point of testing the reset. + const user = setup(); + const opener = screen.getByRole("button", { name: /^Settings/ }); + await user.click(opener); + + fireEvent.keyDown(document, { key: "Tab" }); + await user.click(screen.getByRole("button", { name: "Close Settings" })); + + expect(document.activeElement).toBe(opener); + }); + + it("R8a finding #16: Escape does not close the dialog when an inner field claims it first", async () => { + const user = setup(); + await user.click(screen.getByRole("button", { name: /^Settings/ })); + await user.click(screen.getByLabelText("rename field")); + await user.keyboard("{Escape}"); + + // The field's own handler ran (preventDefault claimed it)... + expect(screen.getByText("rename cancelled")).toBeInTheDocument(); + // ...and the dialog is deliberately still open, unlike the existing + // "Escape closes whatever is open" test above (a field with no + // preventDefault of its own, "first field" - the ordinary case, which + // must still work exactly as before). + expect(screen.getByRole("dialog", { name: "Settings" })).toBeInTheDocument(); + }); + it("closing restores focus to the opener", async () => { const user = setup(); const opener = screen.getByRole("button", { name: /^Settings/ }); diff --git a/web_ui/src/app/overlays/overlays.tsx b/web_ui/src/app/overlays/overlays.tsx index 140223c..2436ecc 100644 --- a/web_ui/src/app/overlays/overlays.tsx +++ b/web_ui/src/app/overlays/overlays.tsx @@ -7,6 +7,7 @@ import { useRef, useState, type ReactNode, + type RefObject, } from "react"; /** @@ -17,13 +18,18 @@ import { * and DIALOG (centered, scrimmed); * - single-open across BOTH tiers - opening anything closes whatever else * is open (the audit-B1 policy, carried over verbatim); - * - Escape closes the open surface, wherever focus lives - one document - * listener, no titleChanged relays, no ShortcutOverride interception: - * the entire class of Qt/Chromium keyboard-arbitration workarounds - * (page-side sentinel scripts, app-level event filters) ceases to exist; + * - Escape closes the open surface, wherever focus lives, UNLESS something + * more specific already claimed it (event.preventDefault(), the standard + * "I'm handling this" signal - an inline editor reverting its own edit, + * say) - one document listener, no titleChanged relays, no + * ShortcutOverride interception: the entire class of Qt/Chromium + * keyboard-arbitration workarounds (page-side sentinel scripts, app-level + * event filters) ceases to exist; * - outside-click dismisses popovers (the click still lands - light-dismiss * contract); dialogs dismiss via scrim click, close button, or Escape; - * - dialogs get a focus trap (Tab cycles inside, focus restored on close); + * - dialogs get a focus trap (Tab cycles inside, focus restored on close), + * hardened against a single leak permanently defeating it (R8a finding + * #17 - see Dialog's own comment); * - chip active-state is a context read of REAL open state, never latched * click state (audit B6). */ @@ -83,18 +89,40 @@ export function OverlayProvider({ children }: { children: ReactNode }) { else surfaceElements.current.delete(name); }, []); - // Escape closes the open surface - document capture phase so it wins even - // with focus inside inputs. + // Escape closes the open surface. R8a (UI/UX issue list finding #16): + // this used to run on document's CAPTURE phase and call + // stopPropagation() unconditionally - which wins even with focus inside + // an input, but wins TOO completely: capture fires before the event ever + // reaches React's own bubble-phase dispatch (React delegates listeners to + // the root container, itself a document descendant), so stopping it here + // meant an inline editor's own Escape handler - Chat Library's rename + // input, a Note's or a Frame/Container's inline label editor - could + // never run at all while any overlay was open, even one unrelated to it + // (a note being edited on the canvas behind an open Pins popover, say). + // Escape looked like it did nothing, or worse, closed the wrong thing. + // + // Now on the BUBBLE phase instead, and gated on event.defaultPrevented: + // an inner handler that wants to claim Escape calls + // event.preventDefault() (the standard way to say "I'm handling this"), + // which this listener checks for since it fires last, after the event + // has already bubbled through every more-specific handler beneath it. + // Handlers that don't call preventDefault (there's no browser default + // action to prevent for a bare Escape - it's purely a convention this + // provider now depends on) are unaffected: Escape still closes the + // overlay exactly as before. NodeMenu.tsx's own Escape handling is a + // separate, deliberately different pattern (capture phase + an + // unconditional stopPropagation of its own) and is unaffected by this + // change - it already wins the race against this listener regardless of + // which phase this one runs on, since capture always fires first. useEffect(() => { if (openSurface === null) return; const onKeyDown = (event: KeyboardEvent) => { - if (event.key === "Escape") { - event.stopPropagation(); + if (event.key === "Escape" && !event.defaultPrevented) { close(); } }; - document.addEventListener("keydown", onKeyDown, true); - return () => document.removeEventListener("keydown", onKeyDown, true); + document.addEventListener("keydown", onKeyDown); + return () => document.removeEventListener("keydown", onKeyDown); }, [openSurface, close]); // Outside-click light-dismiss for POPOVERS only. pointerdown, not click: @@ -150,8 +178,107 @@ export function Popover({ ); } +// R8a (UI/UX issue list finding #17): the original selector didn't exclude +// [disabled] - a disabled button/input/select/textarea is real DOM content +// that querySelectorAll happily returns, but the BROWSER skips it on Tab +// (disabled elements are never part of the native tab order). Three of +// Settings' five sections end in a primary button disabled in its default +// state (IntegrationsPage/ApiProviderPage/OllamaPage - each gated on an +// empty draft field), so "the last focusable element" as this trap +// computed it was frequently a button Tab could never actually land on - +// activeElement === lastEl was never true, the wrap-around branch below +// never ran, and focus walked straight out of the dialog onto the app +// behind the scrim. const FOCUSABLE = - 'button, [href], input, select, textarea, [tabindex]:not([tabindex="-1"])'; + 'button:not([disabled]), [href], input:not([disabled]), select:not([disabled]), textarea:not([disabled]), [tabindex]:not([tabindex="-1"])'; + +// Deliberately NOT also filtering on el.offsetParent !== null (an +// "is this actually rendered, not display:none'd by some ancestor" check +// the audit's own fuller fix suggests, for a hidden tab section / a step +// not yet reached). Two reasons: this app's own dialogs don't have that +// shape today - every multi-section dialog audited (Settings included) +// switches sections by conditionally RENDERING the active one, not by +// CSS-hiding inactive ones while leaving them mounted, so there is no +// concrete instance of this ever mattering to check against; and jsdom +// (this file's own test environment) does not implement layout at all, +// so offsetParent reads null unconditionally regardless of real +// visibility - filtering on it would make every focusable in every test +// vanish, not just genuinely hidden ones, which is worse than not +// checking it. The Tab-gated focusin backstop below still catches a +// focus escape from THIS cause exactly the same as any other - it does +// not depend on knowing why the trap's own boundary was wrong, only that +// a Tab press left focus outside the panel. + +/** + * R8a (finding #17): a belt-and-suspenders backstop for a Tab-cycling focus + * trap. The primary trap (Dialog's own effect below, or a standalone one + * like CodeExecutionApprovalPanel.tsx's) is scoped to the panel itself - it + * only ever runs for a keydown that reaches an element ALREADY inside the + * panel, so it has no way to pull focus back once it has already escaped + * (a disabled trailing button proved reachable; some future change to a + * panel's own content - a hidden tab section, say - could reopen it a + * different way). A `focusin` listener at `document` sees focus AFTER it + * lands anywhere, regardless of how it got there or why the primary trap's + * boundary was wrong, and is the one place that can catch an escape the + * trap above missed. + * + * Exported so this exact, easy-to-get-wrong logic is never hand-written a + * third time - it already has two real bugs behind it, both found the hard + * way (a failing test / a live stack overflow), not by inspection: + * + * - An UNGATED version (redirect whenever focus lands outside the panel, + * full stop) fights close()'s own legitimate focus restoration: + * overlays.tsx's close() restores focus to the ORIGINAL trigger element - + * by definition outside the panel - SYNCHRONOUSLY, in the same tick that + * sets openSurface to null, before React has re-rendered and before this + * effect's own cleanup has run. An ungated listener is still attached at + * that exact moment and yanks focus back into a dialog mid-close. + * - The SAME ungated version also infinite-loops the moment more than one + * trapped panel is open at once (CodeExecutionApprovalPanel.tsx + * deliberately supports this - two pending approvals on different + * nodes): each panel's own unconditional listener sees the OTHER panel's + * redirect as ITS OWN escape and redirects back, forever. + * + * Gating on "was Tab literally the immediately-preceding key" (reset by any + * OTHER key, including Escape, and by any pointerdown - closes the gap a + * mouse click on a close button would otherwise leave) fixes both: it + * excludes every programmatic .focus() call by construction (close()'s + * restore included), and each panel's own flag is consumed by its own + * FIRST redirect, bounding any cross-panel ping-pong instead of looping. + */ +export function useFocusEscapeBackstop( + panelRef: RefObject, + active: boolean, + focusableSelector: string, +) { + useEffect(() => { + if (!active) return; + const panel = panelRef.current; + if (!panel) return; + let lastKeyWasTab = false; + const onKeyDownCapture = (event: KeyboardEvent) => { + lastKeyWasTab = event.key === "Tab"; + }; + const onPointerDown = () => { + lastKeyWasTab = false; + }; + const onFocusIn = (event: FocusEvent) => { + if (!lastKeyWasTab) return; + lastKeyWasTab = false; + if (panel.contains(event.target as Node)) return; + const focusables = [...panel.querySelectorAll(focusableSelector)]; + (focusables[0] ?? panel).focus(); + }; + document.addEventListener("keydown", onKeyDownCapture, true); + document.addEventListener("pointerdown", onPointerDown, true); + document.addEventListener("focusin", onFocusIn); + return () => { + document.removeEventListener("keydown", onKeyDownCapture, true); + document.removeEventListener("pointerdown", onPointerDown, true); + document.removeEventListener("focusin", onFocusIn); + }; + }, [active, panelRef, focusableSelector]); +} /** Centered modal surface with scrim + focus trap + mandatory titled header * and close button (the audit-B5 rule: every dialog is closeable on sight). */ @@ -180,12 +307,13 @@ export function Dialog({ if (!isOpen) return; const panel = panelRef.current; if (!panel) return; - const first = panel.querySelector(FOCUSABLE); + const focusableIn = () => [...panel.querySelectorAll(FOCUSABLE)]; + const first = focusableIn()[0]; (first ?? panel).focus(); const onKeyDown = (event: KeyboardEvent) => { if (event.key !== "Tab") return; - const focusables = [...panel.querySelectorAll(FOCUSABLE)]; + const focusables = focusableIn(); if (focusables.length === 0) return; const firstEl = focusables[0]; const lastEl = focusables[focusables.length - 1]; @@ -201,6 +329,8 @@ export function Dialog({ return () => panel.removeEventListener("keydown", onKeyDown); }, [isOpen]); + useFocusEscapeBackstop(panelRef, isOpen, FOCUSABLE); + if (!isOpen) return null; return (
e.target === e.currentTarget && overlays.close()}>