fix(a11y): expose modals/sheets as dialogs with focus + Escape - #33
Merged
Merged
Conversation
The two bottom sheets had no role/aria-modal, no focus management, and no Escape-to-close, and the two modals duplicated a window-level Escape listener (which would double-close a modal stacked over a sheet). Add a shared useDialog hook: marks the element as a dialog (caller adds role="dialog" aria-modal="true"), moves focus into it on open, and closes on Escape via an element-level onKeyDown — so a modal opened over the sheet closes only itself, not both. `active` keys the focus for the always-mounted AddToPlaylistSheet host (focus each time it opens, not once at startup). All four overlays now use it; the modals' hand-rolled Escape effects are removed. (Background scroll bleed is already prevented by the global body lock from the bottom-nav fix, so this pass is the a11y/focus/Escape half of the issue.) Verified in-browser: each overlay is role=dialog + aria-modal, receives focus on open, closes on Escape, and a confirm modal over the sheet closes only the modal. Fixes #15. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FkDKh1Uo1a4D7n5wCKdcKF
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
2 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.
Fixes #15.
Problem
The two bottom sheets had no
role/aria-modal, no focus management, and no Escape-to-close; the two modals duplicated a window-level Escape listener, which would double-close a modal stacked over a sheet.(The scroll-bleed part of #15 is already prevented by the global
body { position: fixed; overflow: hidden }lock added in the bottom-nav fix #21, so this pass covers the a11y/focus/Escape half.)Fix
New shared
useDialoghook:role="dialog" aria-modal="true").onKeyDown— so a modal opened over the sheet closes only itself, not both.activekeys the focus for the always-mountedAddToPlaylistSheethost (focus each time it opens, not once at startup).All four overlays adopt it; the modals' hand-rolled Escape effects are removed (net de-duplication).
Verification
Built clean; behavioral test:
role=dialog+aria-modal, receives focusrole=dialog, focusedNote: this focuses the dialog container but doesn't implement a full Tab focus-trap — a reasonable follow-up, but out of scope here.
🤖 Generated with Claude Code
Generated by Claude Code