Repository navigation
Make overlays, status messages and the wheel reachable by screen readers - #87
Merged
Merged
Conversation
Screen-reader and reduced-motion pass over the existing UI, plus a few layout fixes the pass turned up. - Both overlay cards become real dialogs (role/aria-modal/aria-labelledby /tabindex="-1") and take focus on open, so VoiceOver announces what was opened instead of leaving the user swiping through the board behind the scrim. Settings gains a visible close button and an Escape path, both routed through one closeSettings() that hands focus back to the gear. - The dictionary card is moved into whichever dialog is currently modal when it pops up; left outside, aria-modal hides it from assistive tech and the level-clear card's word chips would open something unreachable. - #preview stops being an aria-live region (it rewrites once per letter during a drag) and result messages go to a new sr-only #status instead. Invalid words announce "not a word" rather than the bare letters, which would otherwise read the same as a correct hit. - Wheel letters become divs: they are pointer-events: none and never got their own click, so button semantics promised an interaction that does not exist and put every letter in the tab order. - Reduced-motion switches from two hand-maintained selector lists to a global 0.01ms rule, so animationend still fires and the classes that playOnce()/grid.js remove on it do not get stuck. - Cards scroll when taller than the viewport (expanded About used to be clipped, taking the tap-to-dismiss scrim with it); the hint button moves to the wheel's bottom-left corner where there is actual room; the rotate mask now requires pointer: coarse so a short desktop window is not told to rotate. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
PR Summary by QodoImprove screen-reader and reduced-motion accessibility for overlays and wheel
AI Description
Diagram
High-Level Assessment
Files changed (10)
|
Code Review by Qodo
1. strings.js imported outside main.js
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| @@ -1,4 +1,5 @@ | |||
| // 查詞卡片（設計文件 §6）：釋義 + 喇叭發音。只有已找到的字能查（由呼叫端把關）。 | |||
| import { strings } from './strings.js'; | |||
There was a problem hiding this comment.
1. strings.js imported outside main.js 📘 Rule violation ⌂ Architecture
src/dictionary-card.js and src/wheel.js now import ./strings.js, creating inter-module imports between src/ game modules outside the allowed src/main.js coordinator. This violates the module-boundary rule and makes cross-module coupling harder to control.
Agent Prompt
## Issue description
Non-`main.js` modules under `src/` are importing another `src/` module (`./strings.js`), which violates the rule that only `src/main.js` may wire modules together.
## Issue Context
New imports were added to support localized `aria-label`s, but they introduce prohibited inter-module dependencies.
## Fix Focus Areas
- src/dictionary-card.js[1-5]
- src/wheel.js[1-5]
- src/main.js[1-120]
- src/strings.js[1-80]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+91
to
+93
| <!-- 拼字串顯示區刻意**不是** aria-live 區塊：showPreview 每滑過一個字母就重寫一次， | ||
| 掛上去等於在整段拖曳期間逐字播報 C→CA→CAT。真正該播的只有 flashPreview 那幾則結果 | ||
| 訊息，那些走下面的 #status（UI 文件 §4-F 第三段）--> |
There was a problem hiding this comment.
2. Non-english comments added 📘 Rule violation ⚙ Maintainability
Multiple added/modified comments contain non-English natural language (e.g., Chinese) in source files. This violates the requirement that source code comments be written in English, reducing maintainability for English-speaking reviewers/auditors.
Agent Prompt
## Issue description
New/modified comments in source files are written in a non-English natural language, which violates the English-only comment requirement.
## Issue Context
This PR adds extensive explanatory comments for accessibility behaviors; they should be translated to English (or reduced/removed if redundant).
## Fix Focus Areas
- index.html[91-93]
- src/main.js[15-23]
- src/strings.js[13-15]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+18
to
+23
| function playOnce(el, cls) { | ||
| el.classList.remove(cls); | ||
| void el.offsetWidth; | ||
| el.classList.add(cls); | ||
| el.addEventListener('animationend', () => el.classList.remove(cls), { once: true }); | ||
| } |
There was a problem hiding this comment.
3. Playonce listener buildup 🐞 Bug ☼ Reliability
playOnce() adds a fresh anonymous "animationend" listener every call; when the animation is restarted before it ends, the previous run is canceled (no animationend), leaving stale listeners that can accumulate during rapid repeated interactions. This can cause unbounded temporary listener growth and multiple redundant callbacks firing on the next completed animation.
Agent Prompt
### Issue description
`playOnce(el, cls)` attaches a new anonymous `animationend` listener on every invocation. If the animation is restarted before it finishes (class removed/re-added), the previous animation run can be canceled without emitting `animationend`, so the `{ once: true }` listener never self-removes. Rapid repeated calls can therefore accumulate stale listeners until a later animation finally completes.
### Issue Context
This helper is used on user-click paths (e.g. hint/claim shake). The `shake` animation duration is 0.35s, so rapid clicking can easily restart the animation before the prior run ends.
### Fix Focus Areas
- src/main.js[15-23]
- src/main.js[439-446]
- src/style.css[313-319]
### Suggested fix
- Use a stable handler (not an inline anonymous closure) and register it for both `animationend` *and* `animationcancel` with `{ once: true }`, so canceled runs don’t leave listeners behind.
- Optionally guard against bubbled events by checking `e.target === el` and, if relevant, `e.animationName`.
- Alternative robust approach: keep a per-element `AbortController`/listener cleanup in a `WeakMap` so each new `playOnce()` aborts prior listeners before starting a new run.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+592
to
+598
| // 三條關閉路徑（點卡片外、右上角 ✕、Escape）共用這支，才不會有一條忘了把焦點還回去。 | ||
| function closeSettings() { | ||
| $('overlay-settings').hidden = true; | ||
| // 焦點交還給開卡的那顆齒輪：不還的話焦點落回 body，讀螢幕的人得從畫面最上面重新滑一次 | ||
| // 才找得回自己剛才在哪裡。 | ||
| $('btn-settings').focus(); | ||
| } |
There was a problem hiding this comment.
4. Focus returns to hidden 🐞 Bug ≡ Correctness
closeSettings() always focuses #btn-settings, but #btn-settings exists only inside #screen-game, which may be hidden if the user keyboard-navigates to and activates #btn-level behind the untrapped modal. Closing settings after switching screens can leave focus on a hidden element or fail to restore focus to any visible control.
Agent Prompt
### Issue description
`closeSettings()` unconditionally focuses `#btn-settings`, but that button lives inside `#screen-game`. Because there is intentionally no JS focus trap for the settings dialog, keyboard users can Tab to underlying controls (like `#btn-level`) and switch to another screen while the settings overlay remains open. When the overlay is closed, focusing a control in a hidden section can strand focus.
### Issue Context
- Settings dialog explicitly does not implement a focus trap.
- `showLevels()` calls `showScreen('levels')`, which hides `#screen-game`.
### Fix Focus Areas
- src/main.js[592-607]
- src/main.js[565-572]
- src/main.js[196-209]
- index.html[81-88]
- index.html[140-148]
### Suggested fix
Pick one (or combine):
1) **Safer focus restore:** store the opener element when opening settings (e.g., `lastSettingsOpener = document.activeElement` or explicitly the gear button), and in `closeSettings()` only focus it if it’s still connected and visible (not in a hidden screen). Otherwise, focus a visible fallback on the current screen (e.g., first focusable in the visible section).
2) **Prevent background activation:** add a minimal keyboard focus trap or make background inert while the modal is open (even if you keep no trap for touch AT).
3) **Close-on-screen-change:** when `showScreen(name !== 'game')`, also close `overlay-settings` (and pick an appropriate focus target for that screen).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
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.
Screen-reader and reduced-motion pass over the existing UI, plus a few layout fixes the pass turned up. No gameplay, economy or level-data changes.
Dialogs
#overlay-clearand#overlay-settingscards getrole="dialog" aria-modal="true" aria-labelledby tabindex="-1"and receive focus on open — focus lands on the card, not the first button, so VoiceOver reads "過關！ +10 金幣" / "設定" instead of "下一關，按鈕" / "關閉，按鈕".closeSettings(), which hands focus back to the gear button.aria-modalalready is one for touch AT, and the only way out is desktop Tab, which in this project only serves dev iteration (UI doc §4-E).bodychild it is outside the modal subtree, so the level-clear card's word chips — the card's main entry point — would open something a screen-reader user can never reach.position: fixedignores DOM position, so nothing moves visually.Status announcements
#previewlosesaria-live:showPreviewrewrites it on every letter, which announced C→CA→CAT for the whole drag..sr-only#status(role="status"), parked outside every screen so it never leaves the a11y tree. Both are cleared after 900ms so a repeated message announces again.notAWord("{字} 不是單字") rather than the bare word — the "invalid" part lives entirely in the shake animation, and a correct hit speaks the same word through TTS.Wheel
<div>s. They arepointer-events: noneand hit-testing is coordinate-based, so they never received their ownclick: button semantics promised "double-tap to activate" for something that does nothing, and put every letter in the tab order. A div still reads out the letter.shuffle/pronounce {word}aria-labels move intostrings.js— they were the only English strings a Chinese-language screen reader would hit.#redeem-inputgets a realaria-label(a placeholder stops being the accessible name once you start typing).Reduced motion
Two hand-maintained selector lists become one global rule at
0.01ms.animation: nonesuppressesanimationend, and bothgrid.jsandmain.js's newplayOnce()remove their animation class on that event — the list version left those classes stuck forever. It also removes the "remember to add your new component to the list" failure mode, which fails silently on the users who need it most.Layout fixes found along the way
.cardscrolls when taller than the viewport. Expanding About (six lines of license text) on a small screen used to be clipped byalign-items: center, which pushed the scrim off-screen too and effectively killed tap-outside-to-close.pointer: coarse: a short desktop window matchedlandscape+max-height: 500pxand covered the whole game with "please rotate", which a desktop user cannot do.bun run checkpasses.