♿️(frontend) use anchor links for table of contents entries#2390
Conversation
2f43dd1 to
f1fe46a
Compare
WalkthroughThis PR converts TOC entries to semantic anchor links, wraps the headings list in a nav labeled by a stable toc-heading id, moves scrolling to the nav, replaces BoxButton with a Box rendered as an anchor (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 |
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: 0 B Total Size: 4.33 MB 📦 View Changed
|
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
8a1192c to
b7b1750
Compare
Replace TOC buttons with anchor links and heading ids for a11y navigation.
b7b1750 to
ad33e9e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/frontend/apps/impress/src/features/docs/doc-table-content/components/Heading.tsx (1)
49-66:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
preventDefault()breaks URL hash updates and deep linking.Calling
preventDefault()on the anchor click blocks the browser from updating the URL hash, which defeats core anchor link functionality:
- Users cannot share links to specific TOC sections
- Browser back/forward navigation won't work with TOC clicks
- Bookmarking a specific section is impossible
If custom scroll behavior is needed (to avoid keyboard issues on mobile), consider removing
preventDefault()and letting the browser update the URL, or usehistory.pushState()/history.replaceState()to manually update the hash without triggering navigation.🔗 Proposed fix to preserve URL hash updates
Option 1: Remove preventDefault and let browser handle navigation (simplest):
- onClick={(e: React.MouseEvent<HTMLAnchorElement>) => { - // With mobile the focus open the keyboard and the scroll is not working - e.preventDefault(); - + onClick={(e: React.MouseEvent<HTMLAnchorElement>) => { if (!isMobile) { editor.focus(); }Option 2: Manually update URL hash if you must preventDefault:
onClick={(e: React.MouseEvent<HTMLAnchorElement>) => { // With mobile the focus open the keyboard and the scroll is not working e.preventDefault(); + window.history.pushState(null, '', `#heading-${headingId}`); if (!isMobile) {🤖 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 - 66, The current onClick handler calls e.preventDefault(), which blocks the browser from updating the URL hash and breaks deep links; update the handler so it no longer prevents the default for non-mobile interactions (remove e.preventDefault() or only call it when isMobile is true), and if you must preventDefault on mobile keep URL semantics by calling history.pushState(null, '', `#${headingId}`) or history.replaceState(null, '', `#${headingId}`) after editor.setTextCursorPosition(headingId, 'end'); keep the existing editor.focus() conditional and the scrollIntoView call intact so behavior remains consistent across devices.
🤖 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 63-72: The test in doc-table-content.spec.ts should assert that
clicking TOC links updates the URL hash: after await link1.click() add
expect(page.url()).toContain('`#heading-`') (or the exact slug for link1) and
after await link2.click() add expect(page.url()).toContain('`#heading-`') for
link2; if the app prevents default hash navigation due to preventDefault() in
Heading.tsx, remove or adjust that preventDefault() in the Heading component so
clicks allow the browser to update location.hash (or explicitly call
history.pushState/location.hash update in the Heading click handler) so the test
can pass.
In
`@src/frontend/apps/impress/src/features/docs/doc-table-content/components/Heading.tsx`:
- Line 90: The aria-current usage in Heading.tsx sets aria-current to "true" for
highlighted TOC items; change it to the more specific token "location" instead.
Update the JSX where aria-current is set (the attribute in the Heading component
rendering, controlled by isHighlight) to use 'location' when isHighlight is true
and undefined otherwise, so aria-current reads aria-current={isHighlight ?
'location' : undefined}.
---
Outside diff comments:
In
`@src/frontend/apps/impress/src/features/docs/doc-table-content/components/Heading.tsx`:
- Around line 49-66: The current onClick handler calls e.preventDefault(), which
blocks the browser from updating the URL hash and breaks deep links; update the
handler so it no longer prevents the default for non-mobile interactions (remove
e.preventDefault() or only call it when isMobile is true), and if you must
preventDefault on mobile keep URL semantics by calling history.pushState(null,
'', `#${headingId}`) or history.replaceState(null, '', `#${headingId}`) after
editor.setTextCursorPosition(headingId, 'end'); keep the existing editor.focus()
conditional and the scrollIntoView call intact so behavior remains consistent
across devices.
🪄 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: b83c6086-b413-4bc3-b33f-0ab4ee767245
📒 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
💤 Files with no reviewable changes (1)
- src/frontend/apps/impress/src/features/docs/doc-editor/hook/useHeadings.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
Add URL hash verification to anchor link tests.
The test verifies viewport scrolling and aria-current state, but doesn't verify that clicking TOC links updates the URL hash (e.g., expect(page.url()).toContain('#heading-')). This is a core anchor link behavior and should be tested.
🧪 Proposed test enhancement
await link1.click();
await expect(editorLevel1).toBeInViewport();
+ await expect(page).toHaveURL(/`#heading-`/);
await expect(link1).toHaveAttribute('aria-current', 'location');
await expect(link2).not.toHaveAttribute('aria-current');
await link2.click();
await expect(editorLevel1).not.toBeInViewport();
await expect(editorLevel2).toBeInViewport();
+ await expect(page).toHaveURL(/`#heading-`/);
await expect(link2).toHaveAttribute('aria-current', 'location');
await expect(link1).not.toHaveAttribute('aria-current');Note: This test would currently fail due to the preventDefault() call in Heading.tsx that blocks URL hash updates.
🤖 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 63 - 72, The test in doc-table-content.spec.ts should assert that clicking
TOC links updates the URL hash: after await link1.click() add
expect(page.url()).toContain('`#heading-`') (or the exact slug for link1) and
after await link2.click() add expect(page.url()).toContain('`#heading-`') for
link2; if the app prevents default hash navigation due to preventDefault() in
Heading.tsx, remove or adjust that preventDefault() in the Heading component so
clicks allow the browser to update location.hash (or explicitly call
history.pushState/location.hash update in the Heading click handler) so the test
can pass.
| `} | ||
| aria-label={text} | ||
| aria-selected={isHighlight} | ||
| aria-current={isHighlight ? 'true' : undefined} |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Prefer aria-current="location" over "true" for navigation links.
Per the ARIA specification, aria-current accepts specific token values: page | step | location | date | time | true. For table-of-contents navigation, "location" is more semantically accurate than the generic "true".
♻️ Proposed fix
- aria-current={isHighlight ? 'true' : undefined}
+ aria-current={isHighlight ? 'location' : undefined}🤖 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`
at line 90, The aria-current usage in Heading.tsx sets aria-current to "true"
for highlighted TOC items; change it to the more specific token "location"
instead. Update the JSX where aria-current is set (the attribute in the Heading
component rendering, controlled by isHighlight) to use 'location' when
isHighlight is true and undefined otherwise, so aria-current reads
aria-current={isHighlight ? 'location' : undefined}.
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.