fix(a11y): shared focus trap + focus restore for modal overlays - #6685
Merged
Conversation
aria-modal="true" promises assistive tech that content behind the dialog
is inert, but ~12 overlays neither trapped Tab nor restored focus on
close - keyboard focus walked straight into the dashboard behind an
'inert' overlay, and closing a dialog dropped focus to <body>.
- New src/utils/focus-trap.ts: createFocusTrap(container, { onEscape?,
initialFocus? }) with activate/deactivate, modeled on the existing
per-surface implementations (confirm-dialog, market-chart-interactions,
CountryDeepDivePanel). Captures the opener on activate and restores it
on deactivate; ignores Tab while focus is inside a stacked dialog (e.g.
confirm-dialog over the settings modal) so nested overlays keep their
own trap.
- Adopted in: SearchModal, SignalModal, StoryModal, watchlist modal,
UnifiedSettings (also gains aria-modal), MobileWarningModal (also gains
Escape - it had no keyboard dismiss at all), WidgetChatModal and
McpConnectModal (both also gain role=dialog/aria-modal/aria-label;
McpConnectModal had no Escape either), LiveNewsPanel channel manager
(gains role=dialog), and the mobile menu / region bottom sheet.
- Mission preset popover now returns focus to its trigger on close
instead of dropping it to <body>.
The three existing hand-rolled traps are intentionally left untouched;
consolidating them onto the shared utility can follow separately.
|
@ayobamiseun is attempting to deploy a commit to the World Monitor Team on Vercel. A member of the Team first needs to authorize it. |
koala73
previously requested changes
Aug 17, 2026
koala73
left a comment
Owner
There was a problem hiding this comment.
Review (not ready)
The shared trap itself is the right direction, but the required unit gate fails on this head. Existing harnesses do not know about createFocusTrap.
1. Search modal first-paint harness — ReferenceError: createFocusTrap is not defined
tests/search-mobile-first-paint.test.mts evals SearchModal source into a harness. The new createFocusTrap(...) call in SearchModal.open is not in that eval scope:
SearchModalHarness.open (eval at tests/search-mobile-first-paint.test.mts:50)
ReferenceError: createFocusTrap is not defined
Fix: inject/mock createFocusTrap in that harness the same way other @/utils/* symbols are provided, or import the real helper into the eval bindings.
2. UnifiedSettings tests — this.focusTrap.activate is undefined
TypeError: Cannot read properties of undefined (reading 'activate')
on UnifiedSettings.open / MCP Clients tab tests. The test double does not run the constructor path that now assigns this.focusTrap = createFocusTrap(...).
Fix: initialize focusTrap on the test double (or guard activate until constructed).
Please re-run:
npm run test:data -- tests/search-mobile-first-paint.test.mts- the UnifiedSettings.open / MCP Clients suite
and push when those are green. Vercel “authorization required” on the fork is not a required gate.
Contributor
Author
|
@koala73 I addressed both requested unit-harness fixes and synced the branch with current |
Contributor
Author
|
@koala73 CI update: the required |
Three problems in the shared trap's document capture handler, all cases of a trap acting for a container it no longer owns: - Every active trap ran its Tab branch whenever focus sat on <body>, so with two overlays open the earliest-registered (bottom-most) one won and pulled focus into the overlay behind the visible dialog. Traps now register in a module-level stack and only the most recent one acts. - The Escape branch ran before the containment check and called stopPropagation(), so it could close a background dialog and swallow the key before the front dialog saw it. It now takes the same ownership guard as Tab. - A container with no visible focusable (hidden or detached by a teardown path) preventDefault()ed every Tab, leaving the whole page with no reachable focus target. It now lets Tab through, matching market-chart-interactions.ts. Also folds the trap's private focusable-element filter into the canonical getFocusableElements() in dom-utils, whose docstring already claimed the job. The two had drifted: the canonical one omitted form controls, so the selector is widened to keep SearchModal's input and MobileWarningModal's checkbox in the cycle. That also brings ProActivationInterstitial's brief-hour <select>, which uses the canonical helper, into its own trap's cycle for the first time. Co-authored-by: Elie Habib <koala73@users.noreply.github.com>
The trap ten overlays now depend on had no test, and both suites this PR touched replace it with a stub -- so a regression in Tab wrap, focus restore, the stacking guard, or the zero-focusable branch would have shipped green. Adds 21 cases over the real utility: initial-focus resolution and its fallbacks, Tab and Shift+Tab wrap at both ends, recovery when focus has fallen to <body>, deferral to a container the trap does not manage, the empty-container branch, Escape with and without a handler, stack ownership across two traps and the handback when the top one closes, and every deactivate() path including the isConnected-gated restore and restoreFocus: false. Uses the mini-dom harness rather than vitest/jsdom, which reports offsetParent === null for every element and would make the focusable filter return an empty list -- passing every assertion vacuously. mini-dom gains contains(), which the trap's containment guard needs. The search harness keeps its stub, which the prior review round accepted: its overlay is a plain object with no querySelectorAll, so the real trap cannot run against it. A comment now points at the suite that pins the real contract. Co-authored-by: Elie Habib <koala73@users.noreply.github.com>
UnifiedSettings.destroy() and MobilePrimaryNav.destroy() tore down every other listener they had installed but left their traps active. Destroying either while its overlay was open left the trap's document capture listener alive over a detached or hidden container, so Tab kept being intercepted for the rest of the page's life. Both now deactivate with restoreFocus: false, since teardown should not hand focus back to a control that is going away too. teardownSettings() and closeMenu()/closeRegion() keep the restoring default for user-initiated closes. The UnifiedSettings test double gains deactivate() so extending that harness past open() fails on the behavior under test rather than a missing method. Co-authored-by: Elie Habib <koala73@users.noreply.github.com>
SignalModal.show() is reached only from the data loader's periodic correlation and military-surge analysis (data-loader.ts:3599, :3605, :4079) -- a background refresh, not a user gesture. Wiring the trap into activateEsc() meant those pushed focus to the modal's close button and confined Tab, so a user typing in the search box or a settings field lost the caret mid-keystroke. show() now takes Escape handling without focus containment. The badge-click paths, showSignal() and showAlert(), keep the trap: there the user asked for the dialog, so moving focus into it is the correct behavior. Co-authored-by: Elie Habib <koala73@users.noreply.github.com>
Closing the popover always restored focus to #missionPresetBtn, but that button sits in .mission-preset-mount, which main.css:1093 hides with display: none !important below 1075px. focus() on an element in a display:none subtree is a no-op, so on mobile -- where the popover opens from #mobileMenuMission instead -- focus still dropped to <body>, the exact behavior this restore was added to fix. openMissionPresetPopover() now records the anchor it was opened from and the close path restores to it, falling back to the desktop trigger when the recorded opener is gone. Co-authored-by: Elie Habib <koala73@users.noreply.github.com>
The new aria-label was the literal 'Manage channels', so screen-reader users on every non-English build heard this dialog announced in English while every other control inside it was translated. components.liveNews.manage already exists in each locale file and live-channels-window.ts:451 -- the content this modal loads -- already uses it. Co-authored-by: Elie Habib <koala73@users.noreply.github.com>
koala73
enabled auto-merge (squash)
August 18, 2026 04:31
This was referenced Aug 19, 2026
koala73
added a commit
that referenced
this pull request
Aug 20, 2026
…ter-only widgets (#6964) * fix(a11y): keyboard paths for panel reorder, resize handles, and pointer-only widgets Layout customization was entirely pointer-only: panel drag-reorder, panel row/column resize, and map height/width resize had no keyboard equivalent, so keyboard users could never arrange their dashboard. - Panel reorder: makeDraggable now also injects a visually-hidden-until- focused move button into each panel header; arrow keys move the panel one slot and persist through the same savePanelOrder() path as a completed drag. Focus rides along with the panel so repeated presses keep moving it. Within-grid only; grid-to-grid moves still need a pointer. - Resize handles (WAI-ARIA window-splitter): the panel row/column strips and the map height/width strips are now focusable role=separator elements with aria-orientation/valuemin/valuemax/valuenow; arrow keys step the span (panels) or px/percent (map) and persist like a finished drag. Focus rings added for all four strips. - RouteExplorer: alternatives cards switch to a roving tabindex (one tab stop per listbox, matching the existing ArrowUp/Down handler) instead of every card being tabbable; chokepoint and product rows accept Space as well as Enter. - AviationCommandBar (Ctrl+J palette) and the IntelligenceGapBadge findings modal - the two overlays deferred from #6685 - gain role=dialog/aria-modal/aria-label and the shared focus trap with restore; findings items become keyboard-activatable (role=button, tabindex, Enter/Space). - Download dropdown trigger exposes aria-haspopup/aria-expanded, kept in sync across toggle, outside-click, and Escape closes. * fix(a11y): keep listbox tab stops and stacked-dialog Escape working Roving tabindex on shared RouteCards left Land corridors and unselected alternatives with tabindex=-1, so those listboxes had no tab stop. Opt into roving only from AlternativesTab and keep a fallback tab stop until an option is selected. RouteExplorer's capture Escape handler also closed the explorer while Aviation Command or the findings modal held focus. Yield when focus is inside a stacked aria-modal, focus the aviation input on trap activate, and let the findings trap own Escape. Co-authored-by: Elie Habib <koala73@users.noreply.github.com> * fix(a11y): complete keyboard interaction contracts * test(a11y): correct splitter pointer expectation --------- Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Elie Habib <koala73@users.noreply.github.com> Co-authored-by: Elie Habib <elie.habib@gmail.com>
7 tasks
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
3 participants
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.
Part of #6573 (accessibility remediation, PR 3 of 7): focus management for modal overlays.
Problem
~12 overlays declare
aria-modal="true"(or function as modals) but neither trap Tab nor restore focus on close.aria-modaltells assistive tech the content behind the dialog is inert — but keyboard focus could Tab straight out of the dialog into the "inert" dashboard, and closing any of these dropped focus to<body>, stranding keyboard users at the top of the page. Two modals (MobileWarningModal,McpConnectModal) had no Escape handling at all.The repo already contains three working hand-rolled traps (
confirm-dialog.ts,market-chart-interactions.ts,CountryDeepDivePanel.ts) — the fix was to extract that pattern once and adopt it where it's missing.What this does
New
src/utils/focus-trap.ts—createFocusTrap(container, { onEscape?, initialFocus? })returningactivate()/deactivate():activate()capturesdocument.activeElementas the return target and moves focus into the dialog.deactivate()restores focus to the opener if it's still connected.onEscapeis optional so surfaces with existing Escape handlers keep them unchanged.Adopted in ten surfaces:
SearchModal(desktop + mobile sheet) — was focusing its input on open but never restoring on close, with Tab escaping thearia-modaloverlay.SignalModal,StoryModal, watchlist modal — Escape-only before; now trap + restore.UnifiedSettings— also gains the missingaria-modal="true"; focus returns to the settings trigger on close.MobileWarningModal— declaredaria-modalbut had no keyboard dismiss at all; now Escape closes it, and it traps/restores.WidgetChatModal,McpConnectModal— full-screen overlays with no dialog semantics; both gainrole="dialog"+aria-modal+aria-label, trap, and restore (McpConnectModalalso gains Escape).LiveNewsPanelchannel manager — hadaria-modalwithoutrole="dialog"; both fixed, plus trap/restore.mobile-primary-nav.ts) — previously a screen-reader user could Tab into the scroll-locked dashboard behind the sheet; traps activate after the opening animation frame so initial focus lands on a visible control.<body>.The three existing hand-rolled traps are deliberately left untouched to keep this PR additive; consolidating them onto the shared utility can follow separately.
Verification
npm run typecheck— cleanbiome linton all 12 touched files — cleannode --test tests/a11y-issue-4373-invariants.test.mjs tests/a11y-issue-5059-invariants.test.mjs— 43/43 pass (settings tab keyboard model unchanged)