Skip to content

[#1248] Scrollspy guide - #1365

Open
jamesnw wants to merge 6 commits into
mainfrom
scroll-spy-guide
Open

[#1248] Scrollspy guide#1365
jamesnw wants to merge 6 commits into
mainfrom
scroll-spy-guide

Conversation

@jamesnw

@jamesnw jamesnw commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Fixes #1248

@github-actions
github-actions Bot requested review from LeaVerou and bramus August 21, 2026 02:24
Comment thread guides/ui-components/scrollspy/expectations.md Outdated
Comment thread guides/ui-components/scrollspy/guide.md

@bramus bramus left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the changes!

@LeaVerou LeaVerou left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Left a couple minor comments but LGTM overall!

Comment thread guides/ui-components/scrollspy/guide.md Outdated
});
};
// Update the aria on initial page load.
setTimeout(syncAriaCurrent, 10);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

(P2, as I don't think it makes a difference in most cases)
This is a bit arbitrary — many things may have not ended loading by then.
I'd suggest using type=module which automatically defers to DOMContantLoaded. Alternatively, attach to DOMContantLoaded and remove that listener if scrollend fires before then.

Another idea could also be to add an animation via :target-current and monitor animationstart — then it's guaranteed to fire whenever :target-current changes, and by in sync with it without us having to do much.

Comment thread guides/ui-components/scrollspy/guide.md Outdated
- **DO** ensure targets have unique `id` attributes matching the links' `href` attributes.
- **DO** provide enough vertical space for sections: `scroll-target-group` most clearly identifies the visible target when sections are large enough that only one or two are visible at a time.
- **MANDATORY**: For accessibility, the visual state must be mirrored with `aria-current`. While `:target-current` handles the visual aspect, this is not exposed to screen readers.
- **DO NOT** rely solely on color for the active state: Include other visual cues like font weight, an underline, or an indicator dot to ensure the state is accessible to users with color vision deficiencies.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Is this needed? I'm thinking in most cases when using color for highlighting there is a very clear difference in lightness, which people with atypical color vision can generally perceive. Have you seen any inaccessible examples in the wild?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

An example would be the Bluesky tabbar (different pattern, similar design)

image

Without the bottom blue bar, this would fail SC 1.4.1, specifically the note that as long as there is a contrast between states of 3:1, color alone is fine. The demo colors are sufficient, but I do think it's at least worth mentioning. I made it less strong of a requirement though.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Yes, but that's not an example of an inaccessible pattern, it's an example of a pattern that would be inaccessible if we were to remove parts of it. I was curious if you had seen any actively inaccessible cases in the wild. If these don't exist, that means LLMs haven't been trained on them!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Create guide and evals for the scrollspy use case

3 participants