fix(ui): restore Escape dismissal in settings and while tooltips show - #272
Draft
kabarza wants to merge 1 commit into
Draft
fix(ui): restore Escape dismissal in settings and while tooltips show#272kabarza wants to merge 1 commit into
kabarza wants to merge 1 commit into
Conversation
Escape silently stopped closing surfaces in two independent ways.
SettingsModal bound Escape as a React `onKeyDown` on the dialog element,
so it only fired while focus sat inside the dialog subtree. Clicking any
non-focusable spot in the panel (a label, the section header, empty space)
blurs to `<body>` — outside that subtree — after which Escape was dead for
the rest of the session while backdrop click kept working. It was the only
modal built this way; Dialog, ModuleInserterDialog, MediaPickerModal,
StepUpDialog and ConfirmDeleteDialog all bind on document/window. Moved it
onto a document listener to match, gated so only the topmost dialog reacts
(a nested media picker must not collapse the whole stack), and added
tabIndex={-1} to the panel so dead-space clicks keep focus inside and the
Tab trap holds.
Tooltip's window capture-phase handler called preventDefault() +
stopPropagation() on Escape and only hid the tooltip. Tooltips open on
plain hover, so resting the pointer on any trigger swallowed Escape
app-wide — no modal, context menu or spotlight could close. It now hides
the tooltip and lets the key through.
Also fixes the focus-return branch in SettingsModal, which was dead: all
three layouts unmount the modal on close, so `open` never transitioned to
false while mounted and focus was never returned to the trigger (WCAG
2.4.3 / Guideline CoreBunch#225). Restoring in the effect cleanup makes it run.
The gate tests asserted the broken behaviour — one required Tooltip to
consume Escape, three matched the dead triggerRef branch as source
strings. Replaced with behavioural coverage for focus-independent close,
stacked-dialog layering and focus return on unmount.
Verified: bun test (6282 pass), bun run build, bun run lint, plus a live
repro in the running dev admin before and after.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Se93H46Eck37iJBJwjmWV2
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Escape silently stopped dismissing surfaces in two independent ways. Both are reproducible in the running dev admin.
1.
SettingsModal— Escape died as soon as focus left the dialog.The modal bound Escape as a React
onKeyDownon the dialog element, so it only fired while the event target was inside the dialog subtree. Clicking any non-focusable spot in the panel — a label, the section header, empty space — blurs to<body>, which is outside that subtree. From that point Escape did nothing for the rest of the session, while backdrop click kept working (which is why it reads as "only Esc is broken").Live trace, clicking empty panel space and then pressing Escape:
Note the absence of any
#rootstage — the event never enters the React root container, so the delegated handler is structurally unreachable.SettingsModalwas the only modal built this way.Dialog,ModuleInserterDialog,MediaPickerModal,StepUpDialogandConfirmDeleteDialogall bind ondocument/windowand are immune. This moves it onto a document listener to match, with two supporting changes:document.bodyand therefore follow this dialog in document order; without the gate, one Escape collapsed the entire stack at once.tabIndex={-1}on the panel, so a click on dead space lands on the panel instead of<body>. This keeps the Tab trap honest — previously the next Tab could walk the page behind the modal.2.
Tooltip— a visible tooltip swallowed Escape app-wide.The capture-phase
windowhandler calledpreventDefault()+stopPropagation()and only hid the tooltip. Tooltips open on plain hover, so resting the pointer on any trigger consumed the next Escape globally — no modal, context menu or spotlight could close.Live trace, hovering a toolbar button until its tooltip shows, then pressing Escape:
Escape now hides the tooltip and keeps propagating to whatever surface owns the keystroke. Capture is retained so the tooltip still hides if a downstream handler stops propagation.
3. Dead focus-return branch (drive-by, same root cause).
SettingsModalrestored focus to the trigger in anopen === falsebranch, but all three layouts unmount the modal on close, soopennever transitions tofalsewhile mounted. The branch never ran and focus was never returned to the trigger, contrary to WCAG 2.4.3 / Guideline #225 stated in the file's own header. Restoring in the effect cleanup makes it actually run.Note for reviewers
This inverts two existing tests that asserted the broken behaviour, which I'd rather flag than have discovered:
tooltip.test.tsxhadconsumes Escape before a parent panel handler can close, assertingparentEscapeCount === 0. That assertion is what the app-wide Escape blackout looked like from the inside. It is nowhides on Escape without consuming it, asserting the key reaches other handlers and is notdefaultPrevented.toolbar.test.tshad three source-string tests matching the deadtriggerRefbranch. Replaced with one asserting the restore lives in the effect cleanup, plus one asserting Escape is bound ondocumentrather than as a ReactonKeyDown.Tooltipis a shared primitive, so the behaviour change reaches every surface that uses one. The trade-off is deliberate: Escape now dismisses the tooltip and the surface underneath, rather than the tooltip alone. Silently eating a global dismissal key because the pointer happened to rest somewhere is the worse failure.New behavioural coverage: focus-independent close, stacked-dialog layering, and focus return on unmount.
Verification
bun run buildbun test— 6282 pass, 0 failbun run lintAlso verified by hand in the running dev admin, before and after, for each path: Escape after clicking dead panel space, Escape with focus forced to
<body>, Escape with a tooltip visible, and Escape with the media picker stacked on top of settings (closes the picker only, then settings on the second press).Checklist
docs/reference/ui-primitives.md, both theDialogandTooltipsections.