Docs: Add visible focus outline for keyboard navigation #33177
Conversation
…view Closes storybookjs#33025 Added :focus-visible styles for interactive elements (a, button, input, textarea, select) in DocsPage.tsx to ensure keyboard focus indicators are visible. Includes fallback for high-contrast/forced-colors environments.
valentinpalkovic
changed the title
📝 WalkthroughWalkthroughAdds keyboard focus visibility styling to interactive elements in DocsContent. Applies a 2px outline using the theme's secondary color with a 2px offset, and includes a high-contrast forced-colors fallback. Single-file change purely enhancing visual accessibility without altering functionality. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
✨ Finishing touches
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
code/addons/docs/src/blocks/components/DocsPage.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use ESLint and Prettier configurations that are enforced in the codebase
Files:
code/addons/docs/src/blocks/components/DocsPage.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Enable TypeScript strict mode
Files:
code/addons/docs/src/blocks/components/DocsPage.tsx
code/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
code/**/*.{ts,tsx,js,jsx,mjs}: Use server-side logger from 'storybook/internal/node-logger' for Node.js code
Use client-side logger from 'storybook/internal/client-logger' for browser code
Do not use console.log, console.warn, or console.error directly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/addons/docs/src/blocks/components/DocsPage.tsx
code/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Export functions that need to be tested from their modules
Files:
code/addons/docs/src/blocks/components/DocsPage.tsx
code/**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
code/**/*.{js,jsx,json,html,ts,tsx,mjs}: Run Prettier with --write flag to format code before committing
Run ESLint with yarn lint:js:cmd to check for linting issues and fix errors before committing
Files:
code/addons/docs/src/blocks/components/DocsPage.tsx
🧠 Learnings (3)
📓 Common learnings
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Tabs/Tabs.stories.tsx:222-227
Timestamp: 2025-11-05T09:36:55.944Z
Learning: Repo: storybookjs/storybook PR: 32458 — In code/core/src/components/components/Button/Button.tsx (React/TypeScript), ButtonProps includes ariaLabel?: string | false and the component maps it to the DOM aria-label. Convention: ariaLabel is mandatory on all Button usages — provide a descriptive string for icon-only buttons; set ariaLabel=false when the button’s children already serve as the accessible name. Do not suggest using a raw aria-label prop on Button call sites.
📚 Learning: 2025-11-05T09:36:55.944Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Tabs/Tabs.stories.tsx:222-227
Timestamp: 2025-11-05T09:36:55.944Z
Learning: Repo: storybookjs/storybook PR: 32458 — In code/core/src/components/components/Button/Button.tsx (React/TypeScript), ButtonProps includes ariaLabel?: string | false and the component maps it to the DOM aria-label. Convention: ariaLabel is mandatory on all Button usages — provide a descriptive string for icon-only buttons; set ariaLabel=false when the button’s children already serve as the accessible name. Do not suggest using a raw aria-label prop on Button call sites.
Applied to files:
code/addons/docs/src/blocks/components/DocsPage.tsx
📚 Learning: 2025-10-03T07:55:42.639Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/manager/components/preview/Toolbar.tsx:102-105
Timestamp: 2025-10-03T07:55:42.639Z
Learning: In code/core/src/manager/components/preview/Toolbar.tsx, we intentionally do not add aria-label/aria-labelledby to StyledToolbar (AbstractToolbar) because the enclosing section is already labeled via an sr-only heading and the toolbar is the only content. Revisit only if real user testing indicates a need.
Applied to files:
code/addons/docs/src/blocks/components/DocsPage.tsx
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: normal
| // Ensure keyboard focus is visible for interactive elements inside docs | ||
| [toGlobalSelector('a, button, input, textarea, select')]: { | ||
| '&:focus-visible': { | ||
| outline: | ||
| theme.base === 'light' | ||
| ? `2px solid ${theme.color.secondary}` | ||
| : `2px solid ${theme.color.secondary}`, | ||
| outlineOffset: '2px', | ||
| // fallback for high-contrast / forced-colors environments | ||
| '@media (forced-colors: active)': { | ||
| outline: '2px solid Highlight', | ||
| }, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Fix toGlobalSelector usage so focus styles apply correctly and respect .sb-unstyled
toGlobalSelector is designed for a single element selector, but here it’s called with a comma‑separated list:
[toGlobalSelector('a, button, input, textarea, select')]: { ... }That expands to roughly:
:where(a, button, input, textarea, select:not(...))which has two problems:
-
Escape hatch and anchor protection are bypassed
The:not(.sb-anchor, .sb-unstyled, .sb-unstyled <element>)part only applies to the last selector (select), soa,button,input, andtextareaare matched even inside.sb-unstyledcontent and on.sb-anchorelements, breaking the documented “unstyled” and anchor behavior. -
selectelements are never matched
Becauseselectis also included inside the:not(...)list,select:not(...select...)cannot match any element, soselectelements never receive the new:focus-visibleoutline. This fails the PR goal of covering all listed interactive elements.
To keep the intended semantics and support all target elements, define the rule per element (you can refactor to share the style object if you prefer). For example:
- // Ensure keyboard focus is visible for interactive elements inside docs
- [toGlobalSelector('a, button, input, textarea, select')]: {
- '&:focus-visible': {
- outline:
- theme.base === 'light'
- ? `2px solid ${theme.color.secondary}`
- : `2px solid ${theme.color.secondary}`,
- outlineOffset: '2px',
- // fallback for high-contrast / forced-colors environments
- '@media (forced-colors: active)': {
- outline: '2px solid Highlight',
- },
- },
- },
+ // Ensure keyboard focus is visible for interactive elements inside docs
+ const focusVisibleOutline = {
+ '&:focus-visible': {
+ outline: `2px solid ${theme.color.secondary}`,
+ outlineOffset: '2px',
+ // fallback for high-contrast / forced-colors environments
+ '@media (forced-colors: active)': {
+ outline: '2px solid Highlight',
+ },
+ },
+ };
+
+ [toGlobalSelector('a')]: focusVisibleOutline,
+ [toGlobalSelector('button')]: focusVisibleOutline,
+ [toGlobalSelector('input')]: focusVisibleOutline,
+ [toGlobalSelector('textarea')]: focusVisibleOutline,
+ [toGlobalSelector('select')]: focusVisibleOutline,(If you’d rather not introduce a new const, you can inline focusVisibleOutline into five separate blocks; behaviorally it’s the same.)
Also, note that the theme.base === 'light' ? ... : ... ternary was redundant (both branches identical), so the above simplification keeps behavior but removes that duplication.
🤖 Prompt for AI Agents
In code/addons/docs/src/blocks/components/DocsPage.tsx around lines 258 to 271,
the current single call to toGlobalSelector with a comma-separated list causes
the :not(...) protection to only apply to the last selector and prevents select
from ever matching; split the rule into one toGlobalSelector call per element
(a, button, input, textarea, select) so each element gets its own
:not(.sb-anchor, .sb-unstyled, .sb-unstyled <element>) protection and focus
styles; factor the shared focus-visible style into a small const (including the
forced-colors fallback) and apply that const to each [toGlobalSelector('...')]
entry, and remove the redundant theme.base ternary since both branches are
identical.
Sidnioulz
left a comment
Sidnioulz
left a comment
There was a problem hiding this comment.
This PR does not really address the issue. It restyles some focus outlines rather than adding missing outlines.
All focus outlines already exist, to the best of my knowledge: we have default outlines for MDX interactive elements, brand new outlines for component controls, and old style outlines for buttons like copy code.
I'll edit the original issue to clarify what is expected. I suggest holding off until the issue is a bit better defined before giving this another go, so we don't waste more of your time!
| [toGlobalSelector('a, button, input, textarea, select')]: { | ||
| '&:focus-visible': { |
There was a problem hiding this comment.
I think we'd be better off targeting toGlobalSelector('*:focus-visible'). We're missing things like area, video controls, summary.
There was a problem hiding this comment.
An explicit list is definitely going to be more performant than a *, but the downside is the list might be difficult to maintain/remember over time. Just food for thought.
There was a problem hiding this comment.
An explicit list is definitely going to be more performant than a
*, but the downside is the list might be difficult to maintain/remember over time. Just food for thought.
Generally speaking, we're splitting hairs here and we can ignore the performance overhead. What costs is how often you need to re-evaluate selectors rather than how many selectors you need to evaluate, because the eval is heavily optimised.
In this specific instance, I believe *:focus-visible has the better performance. Selectors will be matched right to left and built into a tree-like index, so
- For every element in the DOM, the CSS engine will first check if it matches the
:focus-visiblepseudo-selector - Then, for every matching element (of which there is exactly one hence the hair splitting), the CSS engine will match every follow-up selector:
- either a single match to make against
* - or ~10ish matches to make against individual elements
- either a single match to make against
Which is why * provides the better performance here (fewer checks to make, as we know only focusable elements can get :focus-visible and *:focus-visible will only apply rules when relevant).
| outline: | ||
| theme.base === 'light' | ||
| ? `2px solid ${theme.color.secondary}` | ||
| : `2px solid ${theme.color.secondary}`, |
There was a problem hiding this comment.
| outline: | |
| theme.base === 'light' | |
| ? `2px solid ${theme.color.secondary}` | |
| : `2px solid ${theme.color.secondary}`, | |
| outline: `2px solid ${theme.color.secondary}`, |
| outlineOffset: '2px', | ||
| // fallback for high-contrast / forced-colors environments | ||
| '@media (forced-colors: active)': { | ||
| outline: '2px solid Highlight', |
There was a problem hiding this comment.
| outline: '2px solid Highlight', | |
| outline: '2px solid AccentColor', |
According to https://developer.mozilla.org/en-US/docs/Web/CSS/Reference/Properties/outline-color, the default outline color is either currentColor or accent-color.
Generally speaking I would prefer to not set a forced colour selector until someone reports an actual problem, though. We need to have some trust in the forced color selector applying system colours properly.
There was a problem hiding this comment.
@Sidnioulz Do you think we should avoid setting the color on the focus outline or am I misreading this comment?
There was a problem hiding this comment.
Specifically talking about the forced colors selector which applies to High Contrast Mode.
I prefer if we don't enforce system colours ourselves until someone reports a problem, because browsers already translate CSS colours into system colours themselves and do a pretty good job of it.
Here for instance, I don't believe @Kamalllx chose the right semantic. Even if we set the right colour, there are a lot of deprecated system colours and a lot of not-yet-baseline system colours, so browser defaults will evolve over time, and we'll miss out on those browser changes when we hardcode system colours.
|
@MichaelArestad I think we should close this PR until we get a chance to work on making the remaining buttons more visually consistent across MDX and addon panels. This doesn't increase consistency, it just changes a colour at the end of the day. WDYT? |
@Sidnioulz I don't follow. Are there major discrepancies across buttons there? How do those relate to this PR? |
The fact that the PR does not remove any So I don't find the assertion that making style changes makes focus visible and allows keyboard navigation. It might improve it but it does not resolve a blocking situation. The reason I'm not super keen to handle this topic just in MDX CSS is that the overall keyboard navigation experience outside of MDX is already fairly inconsistent: docs-focus-styles.webmSo if we want to build a consistent keyboard navigation experience (https://www.w3.org/WAI/WCAG21/Understanding/consistent-identification.html), we need to align those styles too, and that requires design input from you, @MichaelArestad. |
|
@Sidnioulz Fair! That video is solid. Thanks! |
|
@Kamalllx thanks for volunteering to help with this issue. I'm gonna go ahead and close the PR because there is a bigger inconsistency we need to fix. If you'd like to contribute a broader fix, please let us know on the issue and Michael will get back to you with designs. Thanks 🙏 |
4 participants
Closes #33025
What I did
Added visible
:focus-visiblestyles for interactive elements (links, buttons, inputs, textareas, and selects) within the Docs view to ensure keyboard focus indicators are visible when navigating with Tab/Shift+Tab.Changes:
DocsPage.tsxthat applies a2px solidoutline using the theme's secondary color to all interactive elements when focused via keyboardoutline-offset: 2pxfor better visual separation@media (forced-colors: active)This fix improves accessibility compliance with WCAG 2.1.1 (Keyboard) and WCAG 2.4.7 (Focus Visible).
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
yarn task --task sandbox --start-from auto --template react-vite/default-tsTabkey to navigate between interactive elements (links, buttons, "Show code", "Copy" buttons)Documentation
MIGRATION.MD
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.