fix(frontend): add skip-to-content link for keyboard users (closes #381) - #452
fix(frontend): add skip-to-content link for keyboard users (closes #381)#452Olajcodes wants to merge 6 commits into
Conversation
…ellar-Ecosystem#381) Add WCAG 2.4.1 bypass-blocks compliance: - Add visually-hidden skip-to-content anchor as first focusable element - Link becomes visible on keyboard focus via CSS transition - Target <main> element with id='main' and tabIndex={-1} for cross-browser focus compatibility (Safari, Chrome, Firefox) - CSS uses absolute positioning with top: -100% to hide off-screen, sliding into view on :focus
|
@Olajcodes Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
|
Warning Review limit reached
Next review available in: 51 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (9)
📝 WalkthroughWalkthroughThe frontend adds a focus-visible skip link targeting a focusable main region. Form controls gain label associations and updated price input handling. ThemeProvider rendering and related tests are updated. ChangesFrontend accessibility, forms, and provider updates
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant KeyboardUser
participant RootLayout
participant SkipLink
participant MainRegion
KeyboardUser->>RootLayout: Tab to first focusable element
RootLayout->>SkipLink: Reveal skip link on focus
KeyboardUser->>SkipLink: Activate link
SkipLink->>MainRegion: Navigate to `#main`
MainRegion->>MainRegion: Receive programmatic focus
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
|
Hi @Olajcodes, This PR could not be merged because it has merge conflicts with the target branch. Please resolve the merge conflicts, push the updated changes, and the PR can be reviewed and merged. Thank you! |
|
@ritik4ever I have resolved the conflicts. Kindly check through and merge |
![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/app/layout.tsx`:
- Around line 56-65: Remove the duplicate provider/layout subtree containing the
extra Navbar, main, and footer. Update the existing ThemeProvider/WalletProvider
tree to include the skip link, set the existing main element to id="main" with
tabIndex={-1}, and preserve a single shared provider context and rendered page
structure.
🪄 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: 8ac6d335-d7a9-4481-934a-59f6a0eb1139
📒 Files selected for processing (2)
frontend/app/globals.cssfrontend/app/layout.tsx
…tem#381) Address review feedback on PR Stellar-Ecosystem#452: collapse the duplicate provider/layout subtree that survived the main merge (two Navbars, two main, two footer, two WalletProvider contexts). Keep a single ThemeProvider > WalletProvider tree with the skip link and main id=main tabIndex=-1. Also fix tests broken by the merge: mock useTheme in Navbar tests, fix RegisterForm test JSX syntax.
All five PR checks were red because the branch inherited broken upstream main state after merging main. These fixes are pre-existing main bugs (verified identical to upstream/main), not regressions from Stellar-Ecosystem#381. Frontend: ThemeProvider always renders its context so useTheme no longer crashes during prerender; RegisterForm labels now use htmlFor/id, price input uses type=text + inputMode=decimal so string validation works, submit disabled via live validation; drop unsupported Tailwind focus:ring-primary/30 opacity modifier (CSS-var color). Backend: add missing pino-http and rate-limit-redis deps; remove duplicate annotateTtlWarning declaration; demo tests mock services.js (real x402 server module-load side effect) and stub global fetch for the AbortError path. Contract: bump ethnum 1.5.2 to 1.5.3 in Cargo.lock because current stable Rust cannot compile ethnum 1.5.2 (E0512 transmute). Validated locally: backend 310/310, frontend build + tsc + 112/112 jest, contract and agents cargo test plus wasm32v1-none release builds all pass.
The previous lock regeneration used --legacy-peer-deps, which dropped the @types/ioredis-mock peer dependency of ioredis-mock, causing backend npm ci to fail with EUSAGE. Regenerate with plain npm install --package-lock-only and verify npm ci + npm test pass.
|
@ritik4ever I have resolved the conflicts. Kindly check through and merge |
c7bf847 to
abbd82c
Compare
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/__tests__/RootLayout.test.tsx`:
- Around line 74-77: Update the RootLayout test’s ordering assertion to verify
the first keyboard-focusable element rather than the first element returned by
getAllByRole('link'). Use focusable-element detection or keyboard tab traversal,
and assert that the skip link receives focus before navigation or any other
focusable control.
- Around line 87-90: Update the main landmark assertion in the RootLayout test
to retrieve the element with screen.getByRole('main') instead of
document.getElementById('main'). Assert that the returned landmark has id="main"
and tabindex="-1", while preserving the existing Page content assertion.
🪄 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: b349b31d-2818-44e0-943a-bf4ca1c6fbe1
📒 Files selected for processing (7)
frontend/__tests__/Navbar.test.tsxfrontend/__tests__/RegisterForm.test.tsxfrontend/__tests__/RootLayout.test.tsxfrontend/app/globals.cssfrontend/app/layout.tsxfrontend/components/RegisterForm.tsxfrontend/components/ThemeProvider.tsx
💤 Files with no reviewable changes (1)
- frontend/components/ThemeProvider.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- frontend/tests/RegisterForm.test.tsx
- frontend/app/globals.css
- frontend/app/layout.tsx
- frontend/tests/Navbar.test.tsx
| // The skip link must precede the navigation so keyboard users can bypass | ||
| // the nav without tabbing through every link. | ||
| const allLinks = screen.getAllByRole('link'); | ||
| expect(allLinks[0]).toBe(skipLink); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Test the first focusable element, not the first link.
getAllByRole('link') excludes buttons, inputs, and elements with tabIndex. The test passes if a focusable element is inserted before the skip link. Assert the first keyboard-focusable element, or tab through the rendered page and assert that the skip link receives focus first.
🤖 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/__tests__/RootLayout.test.tsx` around lines 74 - 77, Update the
RootLayout test’s ordering assertion to verify the first keyboard-focusable
element rather than the first element returned by getAllByRole('link'). Use
focusable-element detection or keyboard tab traversal, and assert that the skip
link receives focus before navigation or any other focusable control.
| const main = document.getElementById('main'); | ||
| expect(main).not.toBeNull(); | ||
| expect(main as HTMLElement).toHaveAttribute('tabindex', '-1'); | ||
| expect(screen.getByText('Page content')).toBeInTheDocument(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the main landmark.
document.getElementById('main') passes for a non-landmark element. Use screen.getByRole('main'), then assert id="main" and tabindex="-1". This prevents a regression from <main> to a generic element.
🤖 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/__tests__/RootLayout.test.tsx` around lines 87 - 90, Update the main
landmark assertion in the RootLayout test to retrieve the element with
screen.getByRole('main') instead of document.getElementById('main'). Assert that
the returned landmark has id="main" and tabindex="-1", while preserving the
existing Page content assertion.
abbd82c to
c7bf847
Compare
2 participants
Summary
Closes #381
Adds a skip-to-content link for keyboard and screen-reader users, satisfying WCAG 2.4.1 (Bypass Blocks) requirement.
Changes
app/layout.tsx<a href="#main" className="skip-link">Skip to content</a>as the first focusable element in the DOMid="main"andtabIndex={-1}to the<main>element for cross-browser focus compatibility (Safari, Chrome, Firefox)app/globals.css.skip-linkstyles using absolute positioning off-screen (top: -100%) to visually hide the link.skip-link:focusstyles that slide the link into view (top: 16px) with an accent-colored outlineAcceptance Criteria
tabIndex={-1}ensures focus lands correctly in all browsers including SafariTesting
tsc --noEmit)Summary by CodeRabbit
Accessibility Improvements
Bug Fixes