R8a: Escape scoping and focus-trap hardening (findings #16, #17) - #173
Merged
Conversation
Fixes the overlay system's Escape handling and Tab focus trap, plus two related completeness gaps found in review. - Escape (finding #16): the document-level Escape-to-close listener ran in the capture phase and unconditionally called stopPropagation(), so it fired and closed the surface before any inner field's own Escape handler (rename inputs, inline edit textareas) ever ran. Moved the listener to the bubble phase and gated it on event.defaultPrevented, so an inner handler can claim Escape by calling preventDefault() - the standard DOM convention - without the outer surface closing underneath it. Applied the same preventDefault() fix to NoteNodeView and GroupNodeView's inline-edit Escape handlers, and to PinOverlay's pin editor (which had no Escape handling at all, so Escape fell through and closed the whole Pins popover). - Focus trap (finding #17): the FOCUSABLE selector counted disabled buttons as valid Tab stops, so a trap whose last element was disabled could never detect the Tab-wrap boundary and would leak focus out of the panel. Excluded [disabled] elements from the selector in both overlays.tsx and CodeExecutionApprovalPanel.tsx. Added a Tab-gated focus escape backstop (new shared useFocusEscapeBackstop hook) as defense in depth: on any focusin that lands outside the panel immediately after a Tab keydown, focus is redirected back in. Gated on the preceding key being Tab (not applied unconditionally) so it can't fight a dialog's own legitimate close()-triggered focus restoration, and so two simultaneously open panels (CodeExecutionApprovalPanel supports this) don't redirect focus back and forth into each other indefinitely. CommandPalette's missing focus trap and NodeMenu's untargeted global Escape handler are related but distinct issues, out of scope here, and should be tracked separately. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
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.
Problem
Two related overlay-system bugs from the R8a UI/UX audit:
stopPropagation(), so it fired and closed the surface before any inner field's own Escape handler could run. Rename inputs, inline-edit textareas, and the Pins editor all had Escape silently discard their own local edit and close (or in PinOverlay's case, had no Escape handling at all — Escape closed the whole popover).FOCUSABLEselector counted disabled buttons as valid stops. When the true last focusable element was disabled (e.g. Approve/Deny both disabled whilebusy), the wrap-boundary check never matched the real last element, and Tab walked focus straight out of the panel.Change
overlays.tsx: moved the Escape listener to the bubble phase and gated it onevent.defaultPrevented, so an inner handler can claim Escape viapreventDefault()— the standard DOM convention — without the outer surface closing underneath it.preventDefault()fix toNoteNodeView,GroupNodeView(inline label editors), andPinOverlay(pin title/note editor, which had no Escape handling before this).overlays.tsxandCodeExecutionApprovalPanel.tsx: excluded[disabled]elements from theFOCUSABLEselector.useFocusEscapeBackstop, shared betweenDialogandCodeExecutionApprovalPanel): if focus lands outside the panel immediately after a Tab keydown, it's redirected back in. Gated on the preceding key being Tab so it can't fight a dialog's own legitimateclose()-triggered focus restoration, and so two simultaneously-open panels don't redirect focus into each other indefinitely.Two related-but-distinct issues were flagged during review and are intentionally out of scope here:
CommandPaletteclaims a focus trap it doesn't actually implement, andNodeMenu's global capture-phase Escape handler has no target check and can swallow unrelated inline edits elsewhere on the canvas. Both should be tracked as separate follow-ups.Test plan
npx tsc --noEmitnpx vitest run(full suite)python -m pytest -q(backend, unaffected)npm run lint/npm run buildNew/updated coverage:
overlays.test.tsx(Escape-claim scoping, disabled-button wrap, Tab-gated backstop, close()-restore race, mouse-click-after-Tab reset),NoteNodeView.test.tsx/GroupNodeView.test.tsx(Escape claims the event),ChatLibraryDialog.test.tsx(corrected a stale test that had codified the old buggy behavior as intended),PinOverlay.test.tsx(Escape cancels edit without closing the popover),CodeExecutionApprovalPanel.test.tsx(disabled-button Tab wrap, Tab-gated backstop).Live-browser verification of
PinOverlaywas attempted but blocked by the preview pane's non-composited state, which prevents React Flow's viewport from initializing (a known environment limitation, unrelated to this change); the fix is covered by real-DOM, real-React-reconciliation unit tests instead.