fix(site): WCAG accessibility pass across all pages#155
Open
whoknowsla wants to merge 3 commits into
Open
Conversation
Adds keyboard support, dialog semantics, form labels, contrast bumps, focus indicators, and live regions so the site meets WCAG 2.1 AA on the static pages. Highlights: - Phase cards on index now keyboard-activatable (role/tabindex + Enter/Space) - Phase modal gets role="dialog", focus trap, focus restore, initial focus - Quiz options in the stage-based renderer switched from <div onclick> to <button> - Catalog/glossary inputs get sr-only labels; catalog headers get aria-sort - Live regions on result counts, quiz score, quiz explanations, lesson loader - GitHub header link gets a real aria-label on every page - Lesson sidebar toggle now syncs aria-expanded/aria-controls - --ink-mute darkened to 4.5:1 (both themes); new --rule-strong (3:1) for component borders on theme-toggle/search-toggle/header-github - Global :focus-visible ring; sr-only utility added - Empty <a> for unreleased lessons replaced with a <span> - Skip-link target (<main>) made programmatically focusable on lesson page
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAccessibility updates across the site: CSS utilities and contrast, consistent header/theme ARIA, catalog button-based sorting, modal focus trapping/restoration and keyboard activation, glossary/live-region search, lesson sidebar and quiz ARIA wiring, and index modal/progress adjustments. ChangesAccessibility & Focus Management
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Improves site-wide accessibility and keyboard navigation, with emphasis on WCAG contrast and better screen-reader semantics for interactive UI.
Changes:
- Adjusted theme tokens and component borders to improve contrast (incl. new
--rule-strong). - Added global focus-visible ring styling and introduced an
.sr-onlyutility class. - Enhanced ARIA semantics and keyboard support across pages (modal focus management, sortable table headers, quiz options as buttons).
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| site/style.css | Adds .sr-only, global focus rings, stronger border token, and contrast tweaks. |
| site/prereqs.html | Adds ARIA labeling and button types for header controls. |
| site/lesson.html | Improves navigation semantics (sidebar toggle state), loading announcements, and quiz option accessibility. |
| site/index.html | Improves document structure (headings/labels) and modal dialog semantics. |
| site/glossary.html | Adds accessible search labeling and live count announcements. |
| site/catalog.html | Makes sortable headers real buttons and improves filter/search labeling and announcements. |
| site/app.js | Adds keyboard activation for phase rows and modal focus management (focus return + tab trapping). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
site/style.css (1)
140-140: ⚡ Quick winReplace deprecated
clipproperty with modernclip-path.The
clipproperty is deprecated. Since the codebase already usesclip-pathelsewhere (lines 880, 884), update the.sr-onlyutility to use the modern equivalent for consistency and standards compliance.♻️ Proposed fix using clip-path
- clip: rect(0, 0, 0, 0); + clip-path: inset(50%);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@site/style.css` at line 140, The `.sr-only` utility uses the deprecated CSS property `clip`; replace it with the modern equivalent by removing `clip: rect(0, 0, 0, 0);` and adding a matching `clip-path: inset(50%);` (or `inset(0 0 0 0)` combined with appropriate positioning) to achieve the same visually-hidden behavior; update the rule in the `.sr-only` selector so it matches the project's existing `clip-path` usage and accessibility intent (see other uses around `clip-path` on lines ~880/884 for style consistency).site/lesson.html (1)
2700-2719: ⚡ Quick winConsider updating quiz explanation content when showing for better screen reader support.
The quiz explanation live region (line 2713) contains static text rendered initially and is only made visible on line 2753 by changing
displayto'block'. Some screen readers may not announcerole="status"regions when only visibility changes without content updates.For better cross-AT compatibility, consider one of these patterns:
- Render explanation empty initially, then insert text when showing
- Clear and re-insert text to force a content change event
Compare to the quiz score pattern (lines 2774-2776), which correctly updates
textContentbefore showing—that will work reliably across screen readers.Suggested refactor
Change line 2713 to render empty:
if (q.explanation) { - html += '<div class="quiz-explanation" id="' + qid + '-exp" role="status" aria-live="polite">' + escapeHtml(q.explanation) + '</div>'; + html += '<div class="quiz-explanation" id="' + qid + '-exp" role="status" aria-live="polite" data-explanation="' + escapeAttr(q.explanation) + '"></div>'; }Then update line 2753 to insert content:
var exp = questionDiv.querySelector('.quiz-explanation'); -if (exp) exp.style.display = 'block'; +if (exp) { + exp.textContent = exp.getAttribute('data-explanation'); + exp.style.display = 'block'; +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@site/lesson.html` around lines 2700 - 2719, The quiz explanation div currently renders the explanation text upfront (id constructed as qid + '-exp') and later only toggles visibility, which may not trigger screen readers; change the initial render in the quiz builder so the element with id qid + '-exp' is created empty (no explanation text), and then in the code path that shows the explanation (the place that sets display = 'block') first set the element's textContent (or innerText) to the escaped explanation (use escapeHtml(q.explanation)) so the live region receives a content update, then make it visible—this mirrors how the quiz score is updated and ensures better AT announcement behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@site/lesson.html`:
- Around line 2700-2719: The quiz explanation div currently renders the
explanation text upfront (id constructed as qid + '-exp') and later only toggles
visibility, which may not trigger screen readers; change the initial render in
the quiz builder so the element with id qid + '-exp' is created empty (no
explanation text), and then in the code path that shows the explanation (the
place that sets display = 'block') first set the element's textContent (or
innerText) to the escaped explanation (use escapeHtml(q.explanation)) so the
live region receives a content update, then make it visible—this mirrors how the
quiz score is updated and ensures better AT announcement behavior.
In `@site/style.css`:
- Line 140: The `.sr-only` utility uses the deprecated CSS property `clip`;
replace it with the modern equivalent by removing `clip: rect(0, 0, 0, 0);` and
adding a matching `clip-path: inset(50%);` (or `inset(0 0 0 0)` combined with
appropriate positioning) to achieve the same visually-hidden behavior; update
the rule in the `.sr-only` selector so it matches the project's existing
`clip-path` usage and accessibility intent (see other uses around `clip-path` on
lines ~880/884 for style consistency).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1204ed92-92bb-4f96-a9e9-317cef0bd12c
📒 Files selected for processing (7)
site/app.jssite/catalog.htmlsite/glossary.htmlsite/index.htmlsite/lesson.htmlsite/prereqs.htmlsite/style.css
- app.js: switch focus-trap visibility check from offsetParent to getClientRects(); offsetParent is null for position:fixed elements even when they are visible and focusable - index.html / app.js: drop ineffective aria-live on the copy button (its aria-label overrides textContent so AT never heard "Copied!"); announce via a dedicated sr-only role=status region instead - style.css: add modern clip-path: inset(50%) to .sr-only, keep clip as a legacy WebKit fallback - lesson.html: render quiz-explanation empty and inject text on reveal so the live region fires a content-change announcement, not just a visibility flip
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@site/app.js`:
- Around line 381-388: The live-region text isn't being retriggered on rapid
repeated copies because the status element keeps the same text; update the copy
handler (the block that references status, revertTimer, btn, originalLabel) to
force a re-announcement by clearing status.textContent before setting it to
'Command copied to clipboard' (e.g., set status.textContent = '' then in the
next tick set the message, via requestAnimationFrame or setTimeout 0), ensure
you still clear any existing revertTimer and reset it to restore btn.textContent
and clear status after 1500ms so repeated clicks always produce a new AT
announcement.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 13a2bce7-cf89-4a37-b0f8-afb47f93162e
📒 Files selected for processing (4)
site/app.jssite/index.htmlsite/lesson.htmlsite/style.css
Clear the status region before setting the message on the next animation frame so screen readers announce each successful copy, not just the first. Identical textContent updates are de-duped by AT otherwise.
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.
Summary
Brings the static site (
site/) up to WCAG 2.1 AA across all five HTML pages (index,catalog,lesson,glossary,prereqs). Visible changes are limited to slightly darker muted text and stronger borders on the header buttons; no layout or behavior regressions.What changed, by WCAG criterion
Level A
role="button" tabindex="0"with Enter/Space activation. Quiz options in the stage-based renderer switched from<div onclick>to real<button>s.role="dialog",aria-modal="true",aria-labelledby, a Tab focus trap, initial focus on the close button, and focus restore to the trigger on close.sr-onlylabels added to the catalog search/phase/status controls and the glossary search.aria-labelon every page (was previously announced as just "…" while the star count loaded).aria-expanded/aria-controls. Unreleased lessons in the modal no longer render as empty<a>(now a labeled<span>).Level AA
--ink-mutedarkened on both themes to clear 4.5:1 against the body background.--rule-strongtoken (~3:1) used for borders on.theme-toggle,.search-toggle,.header-github.:focus-visibleoutline so every link / button / role=button shows a focus ring.<button>s witharia-sortcyclingnone → ascending → descending.role="status" aria-live="polite"on catalog count, glossary count, lesson loader, quiz score, and quiz explanations.Polish
<aside>regions on the lesson page getaria-labels.<div class="toc-title">on the home page promoted to<h2>for proper heading order.aria-hiddenso AT no longer reads "Toggle theme, N".aria-hidden.<main>on the lesson page getstabindex="-1"so the skip-link target reliably receives focus.Files
Verified
aria-sort.