Uh oh!
There was an error while loading. Please reload this page.
Wave 2 Phase 2 — accessibility: contrast (#107) + focus/labels (#108) - #214
Merged
Conversation
Darken four contrast failures that measured below the 4.5:1 AA minimum for normal text: - --text-muted #8a8372 (~224 uses) -> #6f6857: now 5.0-5.5:1 on white / --bg / --bg-inset (was 3.4-3.8:1). - .btn--primary background --accent #8a9a5b (white label 3.06:1) -> --brand-forest #1B6C42 (white label 6.37:1), tying the primary action to the established brand green. - .chip--accent text --accent on --accent-soft (2.80:1) -> --sap-600 (5.84:1). - ModelToggle active labels: Fast #3B82F6 (2.87:1) -> #1D4ED8 (5.23:1), Smart #8A63D2 (3.30:1) -> #6D28D9 (5.36:1), on their soft pills. Ratios computed with the WCAG relative-luminance formula. The new #112 state cues were verified too: the error "!" is 4.70:1 (--err on --err-soft) and the rarity status dot clears the 3:1 non-text bar for all five tiers.
#108) Focus visibility (WCAG 2.4.7): remove the inline outline:"none" on the 13 inputs/textareas (ChatPanel, Social, notetaker x6, Learn x2, ParsedCardsTable x2, page.tsx) so each falls back to the existing global :focus-visible accent ring instead of having no focus indicator. Accessible names (4.1.2 / 3.3.2): aria-label on the ChatPanel tutor textarea ("Message"), the ManageCoursesModal search input, and the Calendar select-all + per-row checkboxes. Modal focus (2.4.3): SignInModal now moves focus into the panel on open, traps Tab within it, and restores focus to the trigger on close. Applied the Dialog.tsx pattern in place (rather than reparenting into Dialog) to preserve SignInModal's custom panel and close animation. Non-text content (1.1.1): role="img" + a source-derived aria-label on FunctionPlot and MermaidBlock; KnowledgeGraph2D gains role="img"/label plus a visually-hidden node <ul> (mirroring KnowledgeGraph3D) so the pointer-only graph is keyboard/AT navigable; the landing logo <div onClick> becomes a <button> with its icon marked decorative.
Deploying with |
| Status | Name | Latest Commit | Updated (UTC) |
|---|---|---|---|
| ❌ Deployment failed View logs | frontend | 1a34b4f | Jun 12 2026, 02:30 PM |
📝 WalkthroughWalkthroughThis PR improves application accessibility and visual consistency across the frontend. It restores browser focus outlines on form inputs, adds descriptive labels and semantic roles to interactive elements and visualizations, implements keyboard focus management in the sign-in modal, and updates design system tokens for better contrast compliance and visual harmony. ChangesAccessibility and Design System Refinement
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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.
Actionable comments posted: 2
🤖 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 `@frontend/src/components/KnowledgeGraph2D.tsx`:
- Around line 648-655: The SR_ONLY hidden node list in KnowledgeGraph2D.tsx
currently renders focusable <button> elements inside nodes.map when onNodeClick
is provided (symbols: SR_ONLY, nodes.map, onNodeClick), which can receive
keyboard focus while visually hidden; change the hidden list to be
non-interactive by replacing the focusable controls with plain, non-focusable
elements (e.g., render a <span> or similar static element for each node) or
remove onClick handlers from the SR_ONLY list so no element inside the list can
receive focus, and if you must keep interactive behavior move the clickable
<button> out of the SR_ONLY container or explicitly set tabIndex={-1} and
aria-hidden to prevent focus but prefer the non-interactive approach.
In `@frontend/src/components/SignInModal.tsx`:
- Around line 79-93: The onKey handler for the SignInModal’s focus trap
(function onKey, using panelRef) only checks equality with first/last elements
and fails when document.activeElement is outside the focusable list (e.g., the
dialog container with tabIndex=-1); update onKey so that after building
focusable (from panelRef.current.querySelectorAll) you check whether active
(document.activeElement) is contained in that focusable set and, if not, always
preventDefault and move focus to last when Shift+Tab or to first when Tab;
ensure the checks reference the symbols focusable, first, last, active and
preserve the existing Escape handling (close()).
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9a5b2643-fbc9-4186-9857-602f32e7c140
📒 Files selected for processing (15)
frontend/src/app/(shell)/notetaker/page.tsxfrontend/src/app/globals.cssfrontend/src/app/page.tsxfrontend/src/components/AchievementUnlockToast.tsxfrontend/src/components/ChatPanel.tsxfrontend/src/components/FunctionPlot.tsxfrontend/src/components/KnowledgeGraph2D.tsxfrontend/src/components/ManageCoursesModal.tsxfrontend/src/components/MermaidBlock.tsxfrontend/src/components/ModelToggle.tsxfrontend/src/components/SignInModal.tsxfrontend/src/components/flashcards/ParsedCardsTable.tsxfrontend/src/components/screens/Calendar.tsxfrontend/src/components/screens/Learn.tsxfrontend/src/components/screens/Social.tsx
💤 Files with no reviewable changes (2)
- frontend/src/components/screens/Learn.tsx
- frontend/src/app/(shell)/notetaker/page.tsx
Comment on lines
+648
to
+655
| <ul style={SR_ONLY} aria-label="Knowledge graph nodes"> | ||
| {nodes.map((n) => | ||
| onNodeClick ? ( | ||
| <li key={n.id}> | ||
| <button type="button" onClick={() => onNodeClick(n)}> | ||
| {n.name} | ||
| </button> | ||
| </li> |
There was a problem hiding this comment.
Avoid focusable controls inside the visually-hidden node list.
At Line 650, the SR-only list renders hidden <button> elements. Those controls can receive keyboard focus while remaining visually clipped, which breaks visible focus expectations and can strand keyboard users. Keep this list non-interactive, or add a focus style that makes focused items visible.
Suggested fix
- <ul style={SR_ONLY} aria-label="Knowledge graph nodes">- {nodes.map((n) =>- onNodeClick ? (- <li key={n.id}>- <button type="button" onClick={() => onNodeClick(n)}>- {n.name}- </button>- </li>- ) : (- <li key={n.id}>{n.name}</li>- ),- )}- </ul>+ <ul style={SR_ONLY} aria-label="Knowledge graph nodes">+ {nodes.map((n) => (+ <li key={n.id}>{n.name}</li>+ ))}+ </ul>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <ulstyle={SR_ONLY}aria-label="Knowledge graph nodes"> | |
| {nodes.map((n)=> | |
| onNodeClick ? ( | |
| <likey={n.id}> | |
| <buttontype="button"onClick={()=>onNodeClick(n)}> | |
| {n.name} | |
| </button> | |
| </li> | |
| <ulstyle={SR_ONLY}aria-label="Knowledge graph nodes"> | |
| {nodes.map((n)=>( | |
| <likey={n.id}>{n.name}</li> | |
| ))} | |
| </ul> |
🤖 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 `@frontend/src/components/KnowledgeGraph2D.tsx` around lines 648 - 655, The
SR_ONLY hidden node list in KnowledgeGraph2D.tsx currently renders focusable
<button> elements inside nodes.map when onNodeClick is provided (symbols:
SR_ONLY, nodes.map, onNodeClick), which can receive keyboard focus while
visually hidden; change the hidden list to be non-interactive by replacing the
focusable controls with plain, non-focusable elements (e.g., render a <span> or
similar static element for each node) or remove onClick handlers from the
SR_ONLY list so no element inside the list can receive focus, and if you must
keep interactive behavior move the clickable <button> out of the SR_ONLY
container or explicitly set tabIndex={-1} and aria-hidden to prevent focus but
prefer the non-interactive approach.
Comment on lines
+79
to
+93
| const onKey = (e: KeyboardEvent) => { | ||
| if (e.key === "Escape") { close(); return; } | ||
| if (e.key === "Tab" && panelRef.current) { | ||
| const focusable = Array.from( | ||
| panelRef.current.querySelectorAll<HTMLElement>( | ||
| 'button:not([disabled]), [href], input:not([disabled]), select:not([disabled]), textarea:not([disabled]), [tabindex]:not([tabindex="-1"])' | ||
| ) | ||
| ); | ||
| if (focusable.length === 0) { e.preventDefault(); return; } | ||
| const first = focusable[0]; | ||
| const last = focusable[focusable.length - 1]; | ||
| const active = document.activeElement as HTMLElement | null; | ||
| if (e.shiftKey && active === first) { e.preventDefault(); last.focus(); } | ||
| else if (!e.shiftKey && active === last) { e.preventDefault(); first.focus(); } | ||
| } |
There was a problem hiding this comment.
Harden Tab trap when focus is not on boundary elements.
On Line 91–Line 92, wrapping only handles active === first/last. If focus lands on the dialog container (tabIndex={-1}) or otherwise outside focusable, Shift+Tab can escape the modal. Force-focus first/last whenever active is outside the trapped set.
Suggested patch
if (e.key === "Tab" && panelRef.current) {
const focusable = Array.from(
panelRef.current.querySelectorAll<HTMLElement>(
'button:not([disabled]), [href], input:not([disabled]), select:not([disabled]), textarea:not([disabled]), [tabindex]:not([tabindex="-1"])'
)
);
if (focusable.length === 0) { e.preventDefault(); return; }
const first = focusable[0];
const last = focusable[focusable.length - 1];
const active = document.activeElement as HTMLElement | null;
- if (e.shiftKey && active === first) { e.preventDefault(); last.focus(); }- else if (!e.shiftKey && active === last) { e.preventDefault(); first.focus(); }+ const inTrap = !!active && focusable.includes(active);+ if (!inTrap) {+ e.preventDefault();+ (e.shiftKey ? last : first).focus();+ } else if (e.shiftKey && active === first) {+ e.preventDefault(); last.focus();+ } else if (!e.shiftKey && active === last) {+ e.preventDefault(); first.focus();+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| constonKey=(e: KeyboardEvent)=>{ | |
| if(e.key==="Escape"){close();return;} | |
| if(e.key==="Tab"&&panelRef.current){ | |
| constfocusable=Array.from( | |
| panelRef.current.querySelectorAll<HTMLElement>( | |
| 'button:not([disabled]), [href], input:not([disabled]), select:not([disabled]), textarea:not([disabled]), [tabindex]:not([tabindex="-1"])' | |
| ) | |
| ); | |
| if(focusable.length===0){e.preventDefault();return;} | |
| constfirst=focusable[0]; | |
| constlast=focusable[focusable.length-1]; | |
| constactive=document.activeElementasHTMLElement|null; | |
| if(e.shiftKey&&active===first){e.preventDefault();last.focus();} | |
| elseif(!e.shiftKey&&active===last){e.preventDefault();first.focus();} | |
| } | |
| constonKey=(e: KeyboardEvent)=>{ | |
| if(e.key==="Escape"){close();return;} | |
| if(e.key==="Tab"&&panelRef.current){ | |
| constfocusable=Array.from( | |
| panelRef.current.querySelectorAll<HTMLElement>( | |
| 'button:not([disabled]), [href], input:not([disabled]), select:not([disabled]), textarea:not([disabled]), [tabindex]:not([tabindex="-1"])' | |
| ) | |
| ); | |
| if(focusable.length===0){e.preventDefault();return;} | |
| constfirst=focusable[0]; | |
| constlast=focusable[focusable.length-1]; | |
| constactive=document.activeElementasHTMLElement|null; | |
| constinTrap=!!active&&focusable.includes(active); | |
| if(!inTrap){ | |
| e.preventDefault(); | |
| (e.shiftKey ? last : first).focus(); | |
| }elseif(e.shiftKey&&active===first){ | |
| e.preventDefault();last.focus(); | |
| }elseif(!e.shiftKey&&active===last){ | |
| e.preventDefault();first.focus(); | |
| } | |
| } |
🤖 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 `@frontend/src/components/SignInModal.tsx` around lines 79 - 93, The onKey
handler for the SignInModal’s focus trap (function onKey, using panelRef) only
checks equality with first/last elements and fails when document.activeElement
is outside the focusable list (e.g., the dialog container with tabIndex=-1);
update onKey so that after building focusable (from
panelRef.current.querySelectorAll) you check whether active
(document.activeElement) is contained in that focusable set and, if not, always
preventDefault and move focus to last when Shift+Tab or to first when Tab;
ensure the checks reference the symbols focusable, first, last, active and
preserve the existing Escape handling (close()).
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 freeto 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.
Wave 2 — Phase 2: accessibility (WCAG AA)
Two scoped, single-issue commits. Each verified by a fresh-context scope check; builds green (tsc + eslint + vitest 37/37). Not for merge yet — awaiting spot-check of the contrast list.
#107 — design-token contrast (WCAG 1.4.3)
Darken four shared tokens/controls that measured below 4.5:1 (ratios computed with the WCAG relative-luminance formula):
--text-muted(~224 uses)#8a8372#6f6857.btn--primarybg (white label)--accent#8a9a5b--brand-forest#1B6C42.chip--accenttext--accenton--accent-soft--sap-600#3a6a2c#3B82F6#1D4ED8#8A63D2#6D28D9Also re-verified the #112 state cues: error
!= 4.70:1 (text); rarity status dot ≥3:1 for all five tiers (non-text). Pre-existing/out-of-scope finding flagged: the rarity text label (common/uncommon/legendary) is 3.2–4.2:1.#108 — focus + a11y labels (2.4.7 / 4.1.2 / 2.4.3 / 1.1.1)
outline:"none"on 13 inputs/textareas → global:focus-visiblering restored.aria-labels on the ChatPanel textarea, ManageCourses search, Calendar checkboxes.role="img"+label on FunctionPlot / MermaidBlock; KnowledgeGraph2D gets a label + visually-hidden node list; landing logo<div onClick>→<button>.Summary by CodeRabbit
Accessibility Improvements
Style Updates