Phase 5: Accessibility — ARIA landmarks, keyboard support, skip-nav#223
Phase 5: Accessibility — ARIA landmarks, keyboard support, skip-nav#223ComBba wants to merge 2 commits intophase-4/production-infrafrom
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Code Review
This pull request focuses on enhancing the accessibility of the application by introducing a 'Skip to main content' link, defining semantic landmarks like
and ARIA regions, and improving the labeling of interactive elements. The review feedback highlights opportunities to further refine these improvements by removing redundant ARIA labels, ensuring disabled buttons remain focusable for keyboard users to access descriptive tooltips, and utilizing native button elements via the asChild pattern to simplify keyboard interaction logic.| <Input | ||
| id="youtube-url-input" | ||
| ref={inputRef} | ||
| value={youtubeUrl} | ||
| onChange={(e) => onYoutubeUrlChange(e.target.value)} | ||
| placeholder="https://www.youtube.com/watch?v=..." | ||
| className="font-mono text-sm" | ||
| aria-label="YouTube video URL" | ||
| /> |
There was a problem hiding this comment.
The aria-label is redundant here because the input is already correctly associated with a visible (though visually hidden via sr-only) label on line 155 using the htmlFor attribute. Redundant labeling can lead to double announcements in some screen readers.
| <Input | |
| id="youtube-url-input" | |
| ref={inputRef} | |
| value={youtubeUrl} | |
| onChange={(e) => onYoutubeUrlChange(e.target.value)} | |
| placeholder="https://www.youtube.com/watch?v=..." | |
| className="font-mono text-sm" | |
| aria-label="YouTube video URL" | |
| /> | |
| <Input | |
| id="youtube-url-input" | |
| ref={inputRef} | |
| value={youtubeUrl} | |
| onChange={(e) => onYoutubeUrlChange(e.target.value)} | |
| placeholder="https://www.youtube.com/watch?v=..." | |
| className="font-mono text-sm" | |
| /> |
| <Button | ||
| className="shrink-0 gap-2 bg-gradient-to-r from-red-600 to-red-500 hover:from-red-500 hover:to-red-400" | ||
| onClick={() => alert("This feature is restricted to admin users and authorized IPs only.")} | ||
| disabled | ||
| className="shrink-0 gap-2 opacity-50 cursor-not-allowed" | ||
| title="Admin access required — use Zero-Prompt Start for the full pipeline" | ||
| > | ||
| <Lock className="w-4 h-4" /> | ||
| Start | ||
| Admin Only | ||
| </Button> |
There was a problem hiding this comment.
Using the disabled attribute removes the button from the tab order, which prevents keyboard users from focusing it and discovering the information provided in the title attribute. Additionally, title is not consistently accessible across different assistive technologies and devices. Consider using aria-disabled="true" to keep the button focusable while indicating its state, and ensure the "Admin Only" restriction is communicated clearly (e.g., via a proper tooltip component or visible text).
<Button
type="button"
aria-disabled="true"
className="shrink-0 gap-2 opacity-50 cursor-not-allowed"
title="Admin access required — use Zero-Prompt Start for the full pipeline"
>
<Lock className="w-4 h-4" />
Admin Only
</Button>
| <Badge | ||
| role="button" | ||
| tabIndex={0} | ||
| aria-pressed={typeFilter === null} | ||
| variant={typeFilter === null ? "default" : "outline"} | ||
| className="cursor-pointer text-[10px] whitespace-nowrap" | ||
| onClick={() => setTypeFilter(null)} | ||
| onKeyDown={(e) => { if (e.key === "Enter" || e.key === " ") { e.preventDefault(); setTypeFilter(null); } }} | ||
| > | ||
| All | ||
| </Badge> |
There was a problem hiding this comment.
To adhere to the first rule of ARIA, it is better to use a native <button> element instead of a span with role="button". The Badge component supports the asChild pattern, which allows you to wrap a native button. This provides built-in keyboard support (Enter and Space) and native focus management without needing manual tabIndex or onKeyDown handlers.
<Badge
asChild
variant={typeFilter === null ? "default" : "outline"}
className="cursor-pointer text-[10px] whitespace-nowrap"
>
<button
type="button"
aria-pressed={typeFilter === null}
onClick={() => setTypeFilter(null)}
>
All
</button>
</Badge>
| <Badge | ||
| key={type} | ||
| role="button" | ||
| tabIndex={0} | ||
| aria-pressed={typeFilter === type} | ||
| variant={typeFilter === type ? "default" : "outline"} | ||
| className="cursor-pointer text-[10px] whitespace-nowrap" | ||
| onClick={() => setTypeFilter(type === typeFilter ? null : type)} | ||
| onKeyDown={(e) => { if (e.key === "Enter" || e.key === " ") { e.preventDefault(); setTypeFilter(type === typeFilter ? null : type); } }} | ||
| > | ||
| {type} | ||
| </Badge> |
There was a problem hiding this comment.
Similar to the "All" badge, using asChild with a native <button> is preferred for better accessibility and to avoid manual keyboard event handling.
<Badge
key={type}
asChild
variant={typeFilter === type ? "default" : "outline"}
className="cursor-pointer text-[10px] whitespace-nowrap"
>
<button
type="button"
aria-pressed={typeFilter === type}
onClick={() => setTypeFilter(type === typeFilter ? null : type)}
>
{type}
</button>
</Badge>
cbbfba5 to
efc3ff5
Compare
1. layout.tsx: add skip-to-main-content link for keyboard/screen reader users 2. zero-prompt-landing.tsx: add <label> for YouTube URL input; replace alert() Start button with disabled "Admin Only" state + tooltip 3. kanban-board.tsx: add role="region" + aria-label on board container 4. kanban-column.tsx: add role="group" + aria-label with column name and item count for screen readers 5. action-feed.tsx: add role="button", tabIndex, aria-pressed, and keyboard handlers (Enter/Space) to all filter badges 6. demo/page.tsx: add aria-hidden="true" to decorative cursor animation Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
efc3ff5 to
3f996ba
Compare
e09da87 to
3495679
Compare
New Frontend Tests (30 unit + 9 E2E):
- use-demo-zero-prompt.test.ts (11): session lifecycle, card mutations, timer cleanup
- use-dashboard.test.ts (6): polling, score distribution, verdict breakdown
- dashboard-api.test.ts (13): health check, stats, results, deployments with auth
- navigation.spec.ts (5): landing, demo, dashboard page loads + navigation
- demo-flow.spec.ts (4): auto-type, kanban board, cards, completion
New Backend Tests (52):
- test_zp_store.py (23): column allowlist validation, JSONB serialization, row parsing
- test_connection_pool.py (15): pool creation, env overrides, lock safety, close behavior
- test_pipeline_runtime_score.py (14): score=0 preserved, None fallback, verdict mapping
Fixed Stale Tests (6):
- test_runtime_config: pool defaults 1/1 → 2/10
- test_server_coverage: health endpoint {"status":"ok"} only
- test_tools_integration (4): GENERATED_APP_DATABASE_URL/INFERENCE_KEY env vars
- digitalocean.py: apply URL scheme conversion to generated_db_url
E2E Infrastructure:
- Playwright config (chromium, webServer: npm run dev, retries: 1)
- vitest.config.ts: exclude e2e/ from unit test runs
Totals: 8 web test files (61 tests), 85 agent test files (~1438 tests)
Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Summary
<label>for YouTube input; replacealert()with disabled "Admin Only" buttonrole="region"+aria-labelon board containerrole="group"+aria-labelwith column name and countrole="button",tabIndex,aria-pressed, keyboard handlers on filter badgesaria-hidden="true"on decorative cursorChanges
Test plan
🤖 Generated with Claude Code