Skip to content

♿️(frontend) use anchor links for table of contents entries #2390

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Ovgodd merged 1 commit into from
Jun 10, 2026
Merged

♿️(frontend) use anchor links for table of contents entries #2390

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ and this project adheres to

- 👷(CI) remove test-e2e-other-browser job #2404
- ♿️(frontend) use heading element for pinned documents section title #2380
- ♿️(frontend) use anchor links for table of contents entries #2390

### Fixed

Expand Down
33 changes: 21 additions & 12 deletions src/frontend/apps/e2e/__tests__/app-impress/doc-table-content.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,34 +32,43 @@ test.describe('Doc Table Content', () => {

const elSidePanel = page.getByLabel('Table of contents side panel');

const level1 = elSidePanel.getByText('Level 1');
const link1 = elSidePanel.getByRole('link', { name: 'Level 1' });
const link2 = elSidePanel.getByRole('link', { name: 'Level 2' });
const link3 = elSidePanel.getByRole('link', { name: 'Level 3' });
const level1 = link1.getByText('Level 1');
const editorLevel1 = editor.getByText('Level 1');
const level2 = elSidePanel.getByText('Level 2');
const level2 = link2.getByText('Level 2');
const editorLevel2 = editor.getByText('Level 2');
const level3 = elSidePanel.getByText('Level 3');
const level3 = link3.getByText('Level 3');

// TOC entries must be <a> links with fragment hrefs
await expect(link1).toBeVisible();
await expect(link1).toHaveAttribute('href', /^#heading-/);
await expect(link2).toHaveAttribute('href', /^#heading-/);
await expect(link3).toHaveAttribute('href', /^#heading-/);

await expect(level1).toBeVisible();
await expect(editorLevel1).not.toBeInViewport();
await expect(level1).toHaveAttribute('aria-selected', 'false');
await expect(link1).not.toHaveAttribute('aria-current');

await expect(level2).toBeVisible();
await expect(level2).toHaveCSS('padding-left', /14.4px/);
await expect(editorLevel2).toBeInViewport();
await expect(level2).toHaveAttribute('aria-selected', 'true');
await expect(link2).toHaveAttribute('aria-current', 'true');

await expect(level3).toBeVisible();
await expect(level3).toHaveCSS('padding-left', /24px/);
await expect(level3).toHaveAttribute('aria-selected', 'false');
await expect(link3).not.toHaveAttribute('aria-current');

await level1.click();
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');
Comment on lines +63 to +72

@coderabbitai coderabbitai Bot Jun 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

});
Comment on lines +63 to 73

@coderabbitai coderabbitai Bot Jun 3, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

});
1 change: 0 additions & 1 deletion src/frontend/apps/impress/src/features/docs/doc-editor/hook/useHeadings.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ export const useHeadings = (editor: DocsBlockNoteEditor) => {
const { setHeadings, resetHeadings } = useHeadingStore();

useEffect(() => {
// Check if editor and its view are mounted before accessing document
if (!editor) {
return;
}
Expand Down
23 changes: 15 additions & 8 deletions src/frontend/apps/impress/src/features/docs/doc-table-content/components/Heading.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { useState } from 'react';
import { css } from 'styled-components';

import { BoxButton, Text } from '@/components';
import { Box, Text } from '@/components';
import { useCunninghamTheme } from '@/cunningham';
import { DocsBlockNoteEditor } from '@/docs/doc-editor/types';
import { useResponsiveStore } from '@/stores';
Expand Down Expand Up @@ -38,15 +38,18 @@ export const Heading = ({
const isActive = isHighlight || isHover;

return (
<BoxButton
id={`heading-${headingId}`}
<Box
as="a"
href={`#heading-${headingId}`}
className="--docs--table-content-heading"
$width="100%"
$minHeight="var(--c--globals--spacings--lg)"
onMouseOver={() => setIsHover(true)}
onMouseLeave={() => setIsHover(false)}
onClick={() => {
onClick={(e: React.MouseEvent<HTMLAnchorElement>) => {
// With mobile the focus open the keyboard and the scroll is not working
Comment thread
Ovgodd marked this conversation as resolved.
e.preventDefault();

if (!isMobile) {
editor.focus();
}
Expand All @@ -68,17 +71,22 @@ export const Heading = ({
: 'none'
}
$justify="center"
$padding="none"
$margin="none"
$hasTransition
$css={css`
text-align: left;
display: flex;
text-decoration: none;
color: inherit;
cursor: pointer;
&:focus-visible {
/* Scoped focus style: same footprint as hover, with theme shadow */
outline: none;
box-shadow: 0 0 0 2px ${colorsTokens['brand-400']};
border-radius: var(--c--globals--spacings--st);
}
`}
aria-label={text}
aria-selected={isHighlight}
aria-current={isHighlight ? 'true' : undefined}

@coderabbitai coderabbitai Bot Jun 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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}.

>
<Text
Expand All @@ -87,10 +95,9 @@ export const Heading = ({
$weight={isHighlight ? '700' : '500'}
$css="overflow-wrap: break-word;"
$hasTransition
aria-selected={isHighlight}
>
{text}
</Text>
</BoxButton>
</Box>
);
};
61 changes: 37 additions & 24 deletions ...ntend/apps/impress/src/features/docs/doc-table-content/components/TableContentSideBar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,13 @@ export const TableContentSideBar = ({ onClose }: TableContentSideBarProps) => {
`}
>
<Box $direction="row" $align="center" $justify="space-between">
<Text as="h2" $weight="bold" $size="16px" $margin="0">
<Text
as="h2"
id="toc-heading"
$weight="bold"
$size="16px"
$margin="0"
>
{t('Table of Contents')}
</Text>
<ButtonCloseModal
Expand All @@ -106,33 +112,40 @@ export const TableContentSideBar = ({ onClose }: TableContentSideBarProps) => {
</Box>
{editor && headings && headings.length > 0 && (
<Box
as="ul"
role="list"
$gap={spacingsTokens['3xs']}
$padding={{
vertical: 'base',
horizontal: 'sm',
}}
as="nav"
aria-labelledby="toc-heading"
$css={css`
overflow-y: auto;
list-style: none;
margin: 0;
`}
>
{headings.map(
(heading) =>
heading.contentText && (
<Box as="li" role="listitem" key={heading.id}>
<Heading
editor={editor}
headingId={heading.id}
level={heading.props.level}
text={heading.contentText}
isHighlight={headingIdHighlight === heading.id}
/>
</Box>
),
)}
<Box
as="ul"
role="list"
$gap={spacingsTokens['3xs']}
$padding={{
vertical: 'base',
horizontal: 'sm',
}}
$css={css`
list-style: none;
margin: 0;
`}
>
{headings.map(
(heading) =>
heading.contentText && (
<Box as="li" role="listitem" key={heading.id}>
<Heading
editor={editor}
headingId={heading.id}
level={heading.props.level}
text={heading.contentText}
isHighlight={headingIdHighlight === heading.id}
/>
</Box>
),
)}
</Box>
</Box>
)}
</Box>
Expand Down
Loading