Improve header accessibility and keyboard navigation support. Resolves #4804#4935
Improve header accessibility and keyboard navigation support. Resolves #4804#4935Divyateja2709 wants to merge 2 commits into
Conversation
Summary by CodeRabbit
WalkthroughAdds keyboard accessibility improvements to the frontend: a visually-hidden skip link in the global CSS and layout pointing to ChangesHeader Accessibility Improvements
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Biome (2.5.0)frontend/src/app/globals.cssFile contains syntax errors that prevent linting: Line 3: Tailwind-specific syntax is disabled.; Line 17: Tailwind-specific syntax is disabled.; Line 304: Tailwind-specific syntax is disabled.; Line 481: Tailwind-specific syntax is disabled.; Line 485: Tailwind-specific syntax is disabled.; Line 486: Tailwind-specific syntax is disabled.; Line 487: Tailwind-specific syntax is disabled.; Line 492: Tailwind-specific syntax is disabled. 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 |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@frontend/src/components/Header.tsx`:
- Around line 173-181: Add the id attribute "mobile-menu-toggle" to the Button
component with onPress={toggleMobileMenu}. The focus-restore logic references
this id as a fallback when the previously focused element is no longer available
in the DOM, so without it, focus restoration will fail when the
originally-focused element becomes unavailable.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: a564c413-6e4f-4b30-b885-20542c8efb8f
📒 Files selected for processing (4)
frontend/__tests__/a11y/components/Header.a11y.test.tsxfrontend/src/app/globals.cssfrontend/src/app/layout.tsxfrontend/src/components/Header.tsx
| <Button | ||
| onPress={toggleMobileMenu} | ||
| aria-expanded={mobileMenuOpen} | ||
| aria-controls="mobile-navigation" | ||
| className="flex h-11 w-11 items-center justify-center rounded-lg bg-transparent text-slate-300 hover:bg-transparent hover:text-slate-100 focus-visible:outline focus-visible:outline-2 focus-visible:outline-offset-2 focus-visible:outline-white" | ||
| > | ||
| <span className="sr-only">Open main menu</span> | ||
| <span className="sr-only">{mobileMenuOpen ? 'Close main menu' : 'Open main menu'}</span> | ||
| {mobileMenuOpen ? <FaTimes className="h-6 w-6" /> : <FaBars className="h-6 w-6" />} | ||
| </Button> |
There was a problem hiding this comment.
Add id="mobile-menu-toggle" to enable fallback focus restoration.
The focus-restore logic at line 74 falls back to document.getElementById('mobile-menu-toggle') when previousActiveElement.current is null, but the toggle button lacks this id attribute. If the originally-focused element becomes unavailable (e.g., removed from DOM), focus will be lost.
Proposed fix
<Button
+ id="mobile-menu-toggle"
onPress={toggleMobileMenu}
aria-expanded={mobileMenuOpen}
aria-controls="mobile-navigation"🤖 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/Header.tsx` around lines 173 - 181, Add the id
attribute "mobile-menu-toggle" to the Button component with
onPress={toggleMobileMenu}. The focus-restore logic references this id as a
fallback when the previously focused element is no longer available in the DOM,
so without it, focus restoration will fail when the originally-focused element
becomes unavailable.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 4 files
Confidence score: 3/5
- In
frontend/src/app/layout.tsx, the skip link points to a<main>element that isn’t focusable, so keyboard users may activate “skip to content” without actually landing in main content, weakening core navigation accessibility — make the target programmatically focusable (for example withtabIndex={-1}) and move focus there before merging. - In
frontend/src/components/Header.tsx, the focus-restoration fallback targets a missing#mobile-menu-toggle, so when the menu closes focus can be lost or sent nowhere, creating a confusing keyboard/screen-reader flow — update the selector to an existing control (or use a ref-based fallback) and verify close/restore behavior before merging.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="frontend/src/app/layout.tsx">
<violation number="1" location="frontend/src/app/layout.tsx:83">
P1: Skip-to-content link target `<main>` is not focusable, so keyboard focus may not move to the main content after activation, defeating the skip link's purpose for keyboard users.</violation>
</file>
<file name="frontend/src/components/Header.tsx">
<violation number="1" location="frontend/src/components/Header.tsx:74">
P2: Focus restoration fallback references a non-existent `#mobile-menu-toggle` element</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| <Header isGitHubAuthEnabled={IS_GITHUB_AUTH_ENABLED} /> | ||
| <BreadCrumbsWrapper /> | ||
| <main className="flex flex-1 flex-col justify-center">{children}</main> | ||
| <main id="main" className="flex flex-1 flex-col justify-center">{children}</main> |
There was a problem hiding this comment.
P1: Skip-to-content link target <main> is not focusable, so keyboard focus may not move to the main content after activation, defeating the skip link's purpose for keyboard users.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At frontend/src/app/layout.tsx, line 83:
<comment>Skip-to-content link target `<main>` is not focusable, so keyboard focus may not move to the main content after activation, defeating the skip link's purpose for keyboard users.</comment>
<file context>
@@ -71,13 +71,16 @@ export default function RootLayout({
<Header isGitHubAuthEnabled={IS_GITHUB_AUTH_ENABLED} />
<BreadCrumbsWrapper />
- <main className="flex flex-1 flex-col justify-center">{children}</main>
+ <main id="main" className="flex flex-1 flex-col justify-center">{children}</main>
<Footer />
<ScrollToTop />
</file context>
| <main id="main" className="flex flex-1 flex-col justify-center">{children}</main> | |
| + <main id="main" tabIndex={-1} className="flex flex-1 flex-col justify-center">{children}</main> |
| @@ -3,7 +3,7 @@ import { Button } from '@heroui/button' | |||
| import Image from 'next/image' | |||
There was a problem hiding this comment.
P2: Focus restoration fallback references a non-existent #mobile-menu-toggle element
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At frontend/src/components/Header.tsx, line 74:
<comment>Focus restoration fallback references a non-existent `#mobile-menu-toggle` element</comment>
<file context>
@@ -46,12 +47,44 @@ export default function Header({ isGitHubAuthEnabled }: { readonly isGitHubAuthE
+ if (prev) {
+ prev.focus()
+ } else {
+ const toggle = document.getElementById('mobile-menu-toggle') as HTMLElement | null
+ toggle?.focus()
+ }
</file context>
1 participant
hi @arkid15r and @kasya
Resolves #4804
This PR improves header accessibility and keyboard navigation support for OWASP Nest. The changes include:
Skip-to-main content link (
layout.tsx): Added a keyboard-accessible "Skip to main content" link at the top of the page, visible only on focus. Addedid="main"to the<main>element as the anchor target.Mobile menu ARIA attributes (
Header.tsx): Addedaria-expandedandaria-controls="mobile-navigation"to the menu toggle button. Addedrole="navigation",id="mobile-navigation", andaria-hiddento the mobile navigation panel. The toggle label dynamically changes between "Open main menu" and "Close main menu" based on state.Focus management (
Header.tsx): When the mobile menu opens, focus moves to the first focusable element inside it. When the menu closes (via button, Escape, or outside click), focus is restored to the toggle button.Escape-key support (
Header.tsx): Pressing Escape now closes the mobile menu.Skip link styling (
globals.css): Added.skip-linkstyles — hidden by default, visible on focus with high-contrast colors.Accessibility tests (
Header.a11y.test.tsx): Added jest-axe tests verifying zero WCAG violations and correct ARIA attributes on the mobile menu toggle.Checklist
make check-testlocally: all warnings addressed, tests passed