♿️(frontend) use anchor links for table of contents entries#2390
Open
Ovgodd wants to merge 4 commits into
Open
♿️(frontend) use anchor links for table of contents entries#2390Ovgodd wants to merge 4 commits into
Ovgodd wants to merge 4 commits into
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. It adds DOM synchronization in useHeadings to create/maintain heading IDs and tabindex, wraps the TOC list in a semantic nav, replaces BoxButton with anchor-like Box links that use href=" 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)
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
Comment on lines
+64
to
74
| 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.
Comment on lines
+37
to
+39
| 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.
Suggested change
| 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.
Contributor
|
Size Change: 0 B Total Size: 4.33 MB 📦 View Changed
|
AntoLC
reviewed
Jun 8, 2026
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/frontend/apps/impress/src/features/docs/doc-editor/hook/useHeadings.tsx (1)
14-32:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftRestore heading anchor synchronization before updating TOC state.
setHeadings(editor)runs, but nothing now guarantees heading DOM nodes haveid="heading-{blockId}"andtabindex="-1". Since TOC links are#heading-*, this breaks native fragment targeting/focus behavior and undermines the accessibility objective.Suggested direction
+const syncHeadingAnchors = (editor: DocsBlockNoteEditor) => { + const headingBlocks = editor.document.filter( + (block) => + block.type === 'heading' && + block.props.level >= 1 && + block.props.level <= 3, + ); + + headingBlocks.forEach((block) => { + const el = document.body.querySelector<HTMLElement>( + `.bn-block-outer[data-id="${block.id}"] [data-content-type="heading"]:first-child`, + ); + if (!el) return; + const expectedId = `heading-${block.id}`; + if (el.id !== expectedId) el.id = expectedId; + if (el.getAttribute('tabindex') !== '-1') el.setAttribute('tabindex', '-1'); + }); +}; + +const updateHeadings = (editor: DocsBlockNoteEditor) => { + setHeadings(editor); + requestAnimationFrame(() => syncHeadingAnchors(editor)); +}; - - setHeadings(editor); + updateHeadings(editor); ... - setHeadings(editor); + updateHeadings(editor);🤖 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 14 - 32, The TOC update happens before heading DOM anchors are guaranteed to exist; modify the onChange debounce handler so that before calling setHeadings(editor) you first ensure heading DOM nodes have the expected id="heading-{blockId}" and tabindex="-1" (e.g. call or implement a helper like syncHeadingAnchors(editor) right before setHeadings); locate the debounce block around editor.onChange and context.getChanges and insert the anchor synchronization step to run after changes are detected and before setHeadings(editor) so fragment links and focus behavior remain consistent.src/frontend/apps/impress/src/features/docs/doc-table-content/components/Heading.tsx (1)
49-65:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not block native anchor navigation/focus for TOC links.
e.preventDefault()pluseditor.focus()prevents the fragment link from performing native hash navigation and moving focus to the heading target, which is the behavior this migration is meant to restore.Suggested fix
onClick={(e: React.MouseEvent<HTMLAnchorElement>) => { - e.preventDefault(); - - if (!isMobile) { - editor.focus(); - } - - editor.setTextCursorPosition(headingId, 'end'); - - document - .querySelector<HTMLElement>(`[data-id="${headingId}"]`) - ?.scrollIntoView({ - behavior: 'smooth', - inline: 'start', - block: 'start', - }); + if (isMobile) { + // keep optional mobile-specific behavior only if needed + return; + } }}🤖 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-table-content/components/Heading.tsx` around lines 49 - 65, The click handler currently calls e.preventDefault() and editor.focus(), which blocks native hash navigation and focus; remove the e.preventDefault() call and the editor.focus() invocation so the browser can perform native fragment navigation and move focus to the heading, and instead (if needed) set the editor cursor after navigation by calling editor.setTextCursorPosition(headingId, 'end') without stealing focus (or defer that call via setTimeout/nextTick if you must run it after the browser scrolls); update the onClick handler around the headingId usage accordingly so the anchor behaves like a normal TOC link.
🤖 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.
Outside diff comments:
In `@src/frontend/apps/impress/src/features/docs/doc-editor/hook/useHeadings.tsx`:
- Around line 14-32: The TOC update happens before heading DOM anchors are
guaranteed to exist; modify the onChange debounce handler so that before calling
setHeadings(editor) you first ensure heading DOM nodes have the expected
id="heading-{blockId}" and tabindex="-1" (e.g. call or implement a helper like
syncHeadingAnchors(editor) right before setHeadings); locate the debounce block
around editor.onChange and context.getChanges and insert the anchor
synchronization step to run after changes are detected and before
setHeadings(editor) so fragment links and focus behavior remain consistent.
In
`@src/frontend/apps/impress/src/features/docs/doc-table-content/components/Heading.tsx`:
- Around line 49-65: The click handler currently calls e.preventDefault() and
editor.focus(), which blocks native hash navigation and focus; remove the
e.preventDefault() call and the editor.focus() invocation so the browser can
perform native fragment navigation and move focus to the heading, and instead
(if needed) set the editor cursor after navigation by calling
editor.setTextCursorPosition(headingId, 'end') without stealing focus (or defer
that call via setTimeout/nextTick if you must run it after the browser scrolls);
update the onClick handler around the headingId usage accordingly so the anchor
behaves like a normal TOC link.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 789f5427-228c-48a7-b409-280abccb37c2
📒 Files selected for processing (3)
src/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
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 free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Purpose
Fix table of contents accessibility: TOC entries were
<button>elements, so they were invisible in screen reader link navigation (RGAA criterion 6.1).Proposal
<a href="#heading-{blockId}">links<nav>landmark witharia-labelledbyreferencing the existingh2aria-currenton the active TOC link (instead of invalidaria-selectedon links)[data-id]+setTextCursorPosition(same behavior as before)doc-table-contente2e tests for link role andhref/aria-currentNote
Focus on the target heading is not yet implemented: BlockNote does not expose
idattributes on heading DOM elements, and manual DOM manipulation is wiped by ProseMirror re-renders. An upstream feature request has been opened on BlockNote to support native headingids.