♿️(frontend) use anchor links for table of contents entries#2390
Conversation
Replace TOC buttons with anchor links and heading ids for a11y navigation.
Ovgodd
force-pushed
the
fix/a11y-2345-toc-links
branch
from
2f43dd1 to
f1fe46a
Compare
WalkthroughThis PR converts the table-of-contents sidebar from custom button-based navigation to semantic anchor links. Changes include adding DOM synchronization in Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 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)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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: 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 `@src/frontend/apps/e2e/__tests__/app-impress/doc-table-content.spec.ts`:
- Around line 64-74: The test clicks on TOC entries but doesn't assert the new
focus contract; after clicking link1 and link2 add explicit focus assertions
using toBeFocused() on the corresponding heading anchors (i.e., assert
link1.toBeFocused() after the first click and link2.toBeFocused() after the
second click) in the block around the existing viewport/aria-current checks so
the test verifies both visual visibility and keyboard focus transfer for
editorLevel1/editorLevel2 navigation.
In `@src/frontend/apps/impress/src/features/docs/doc-editor/hook/useHeadings.tsx`:
- Around line 37-39: In useHeadings (useHeadings.tsx) the logic only checks for
existence of the tabindex attribute; update it to enforce tabindex="-1" whenever
the current value !== "-1" — i.e., read el.getAttribute('tabindex') and call
el.setAttribute('tabindex', '-1') if the value is absent or not equal to '-1' so
TOC anchor focus behavior is normalized; ensure you still use the same el
identifier and setAttribute method used in the diff.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 92b7510b-ac11-4bc9-aabc-ee9a438ac8b1
📒 Files selected for processing (5)
CHANGELOG.mdsrc/frontend/apps/e2e/__tests__/app-impress/doc-table-content.spec.tssrc/frontend/apps/impress/src/features/docs/doc-editor/hook/useHeadings.tsxsrc/frontend/apps/impress/src/features/docs/doc-table-content/components/Heading.tsxsrc/frontend/apps/impress/src/features/docs/doc-table-content/components/TableContentSideBar.tsx
| await link1.click(); | ||
| await expect(editorLevel1).toBeInViewport(); | ||
| await expect(level1).toHaveAttribute('aria-selected', 'true'); | ||
| await expect(level2).toHaveAttribute('aria-selected', 'false'); | ||
| await expect(link1).toHaveAttribute('aria-current', 'true'); | ||
| await expect(link2).not.toHaveAttribute('aria-current'); | ||
|
|
||
| await level2.click(); | ||
| await link2.click(); | ||
| await expect(editorLevel1).not.toBeInViewport(); | ||
| await expect(editorLevel2).toBeInViewport(); | ||
| await expect(level2).toHaveAttribute('aria-selected', 'true'); | ||
| await expect(level1).toHaveAttribute('aria-selected', 'false'); | ||
| await expect(link2).toHaveAttribute('aria-current', 'true'); | ||
| await expect(link1).not.toHaveAttribute('aria-current'); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Assert focus transfer after TOC activation.
On Line 64 and Line 69, clicks currently validate viewport and aria-current, but not the new focus-move contract. Add explicit toBeFocused() assertions on the heading anchors to lock in the accessibility fix.
Suggested test diff
@@
await expect(link1).toHaveAttribute('href', /^`#heading-/`);
await expect(link2).toHaveAttribute('href', /^`#heading-/`);
await expect(link3).toHaveAttribute('href', /^`#heading-/`);
+ const heading1Anchor = page.locator((await link1.getAttribute('href'))!);
+ const heading2Anchor = page.locator((await link2.getAttribute('href'))!);
@@
await link1.click();
await expect(editorLevel1).toBeInViewport();
+ await expect(heading1Anchor).toBeFocused();
await expect(link1).toHaveAttribute('aria-current', 'true');
await expect(link2).not.toHaveAttribute('aria-current');
@@
await link2.click();
await expect(editorLevel1).not.toBeInViewport();
await expect(editorLevel2).toBeInViewport();
+ await expect(heading2Anchor).toBeFocused();
await expect(link2).toHaveAttribute('aria-current', 'true');
await expect(link1).not.toHaveAttribute('aria-current');🤖 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 `@src/frontend/apps/e2e/__tests__/app-impress/doc-table-content.spec.ts` around
lines 64 - 74, The test clicks on TOC entries but doesn't assert the new focus
contract; after clicking link1 and link2 add explicit focus assertions using
toBeFocused() on the corresponding heading anchors (i.e., assert
link1.toBeFocused() after the first click and link2.toBeFocused() after the
second click) in the block around the existing viewport/aria-current checks so
the test verifies both visual visibility and keyboard focus transfer for
editorLevel1/editorLevel2 navigation.
| if (!el.hasAttribute('tabindex')) { | ||
| el.setAttribute('tabindex', '-1'); | ||
| } |
There was a problem hiding this comment.
Enforce tabindex="-1" value, not only attribute presence.
Line 37 only checks whether tabindex exists. If it exists with a different value, this won’t normalize focus behavior for TOC anchors. Set it whenever the value is not -1.
Suggested patch
- if (!el.hasAttribute('tabindex')) {
+ if (el.getAttribute('tabindex') !== '-1') {
el.setAttribute('tabindex', '-1');
}📝 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.
| if (!el.hasAttribute('tabindex')) { | |
| el.setAttribute('tabindex', '-1'); | |
| } | |
| if (el.getAttribute('tabindex') !== '-1') { | |
| el.setAttribute('tabindex', '-1'); | |
| } |
🤖 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 `@src/frontend/apps/impress/src/features/docs/doc-editor/hook/useHeadings.tsx`
around lines 37 - 39, In useHeadings (useHeadings.tsx) the logic only checks for
existence of the tabindex attribute; update it to enforce tabindex="-1" whenever
the current value !== "-1" — i.e., read el.getAttribute('tabindex') and call
el.setAttribute('tabindex', '-1') if the value is absent or not equal to '-1' so
TOC anchor focus behavior is normalized; ensure you still use the same el
identifier and setAttribute method used in the diff.
|
Size Change: +1 B (0%) Total Size: 4.33 MB 📦 View Changed
|
1 participant
Purpose
Fix table of contents accessibility: TOC entries were
<button>elements without fragment targets, so they were invisible in screen reader link navigation and did not move keyboard focus to the corresponding heading.Proposal
<a href="#heading-{blockId}">linksidandtabindex="-1"on heading blocks in the document (useHeadings)<nav>landmark with an accessible labelaria-currenton the active TOC link (instead of invalidaria-selectedon links)[data-id]and focus via heading anchors on clickdoc-table-contente2e tests for link role andhref/aria-current