fix(core/ui): add aria-label and arrow key menu navigation#5243
fix(core/ui): add aria-label and arrow key menu navigation#5243marcoscaceres wants to merge 3 commits intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR aims to improve the accessibility and keyboard usability of the ReSpec UI, specifically the pill/menu interactions tied to issues #884 and #1064. It updates core/ui so the modal close control is labeled for assistive tech and the menu can move focus with keyboard navigation keys.
Changes:
- Add an accessible name to the modal close button.
- Make opening the ReSpec menu safely focus the first command only when one exists.
- Add ArrowUp/ArrowDown/Home/End keyboard navigation, plus Escape handling, for the ReSpec menu.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| respecPill.setAttribute("aria-expanded", String(menu.hidden)); | ||
| toggleMenu(); | ||
| respecPill.focus(); | ||
| return; |
There was a problem hiding this comment.
Already handled at line 91: the document-level click handler unconditionally sets aria-expanded to "false". See the test at line 34 of ui-spec.js ("resets aria-expanded when menu is closed by outside click").
| const items = /** @type {HTMLElement[]} */ ([ | ||
| ...menu.querySelectorAll("button:not([disabled])"), | ||
| ]); | ||
| const currentIndex = items.indexOf( | ||
| /** @type {HTMLElement} */ (document.activeElement) | ||
| ); | ||
| switch (e.key) { | ||
| case "ArrowDown": { | ||
| e.preventDefault(); | ||
| const next = items[(currentIndex + 1) % items.length]; | ||
| next?.focus(); | ||
| break; | ||
| } | ||
| case "ArrowUp": { | ||
| e.preventDefault(); | ||
| const prev = items[(currentIndex - 1 + items.length) % items.length]; | ||
| prev?.focus(); | ||
| break; | ||
| } | ||
| case "Home": | ||
| e.preventDefault(); | ||
| items[0]?.focus(); | ||
| break; | ||
| case "End": | ||
| e.preventDefault(); | ||
| items[items.length - 1]?.focus(); |
There was a problem hiding this comment.
Already covered: see the describe("keyboard navigation") block starting at line 87 of ui-spec.js. Tests for ArrowDown, ArrowUp, Home, End, and Escape are all there.
| const closeButton = html`<button | ||
| class="close-button" | ||
| aria-label="Close" | ||
| onclick=${() => ui.closeModal()} |
There was a problem hiding this comment.
The currentModalOwner variable (line 64) is set in freshModal() (line 313) when a modal opens. closeModal() at line 283 uses owner || currentModalOwner as the effective owner, so calling closeModal() without an argument correctly resets the opener's aria-expanded via the fallback.
marcoscaceres
force-pushed
the
fix/884-close-a11y
branch
2 times, most recently
from
d62ff67 to
07e3515
Compare
|
@copilot Can you confirm all your feedback has been addressed? Can you fix anything that was not addressed? |
All three review issues have been addressed in commit f684d20:
|
The documentElement click handler checked menu.hidden before setting aria-expanded to false, but something was hiding the menu before the handler ran (race with toggleMenu). Move the setAttribute outside the if-guard so it always resets regardless of menu state. Verified in Safari: aria-expanded correctly goes from "true" to "false" on outside click.
When freshModal replaces an existing modal (e.g., opening warnings while errors modal is open), the previous owner's aria-expanded was never reset to "false". Now resets it before setting the new owner. Found by Opus semantic gate spot-check.
marcoscaceres
force-pushed
the
fix/884-close-a11y
branch
from
08122cb to
a5e5158
Compare
Closes #884
Closes #1064