Skip to content

Docs: Add visible focus outline for keyboard navigation #33177

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

Closed
Kamalllx wants to merge 1 commit into from
+14 −0
Closed

Docs: Add visible focus outline for keyboard navigation #33177

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
14 changes: 14 additions & 0 deletions code/addons/docs/src/blocks/components/DocsPage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,20 @@ export const DocsContent = styled.div(({ theme }) => {
color: theme.color.defaultText,
'& code': code,
},
// Ensure keyboard focus is visible for interactive elements inside docs
[toGlobalSelector('a, button, input, textarea, select')]: {
'&:focus-visible': {
Comment on lines +259 to +260
Copy link
Member

@Sidnioulz Sidnioulz Dec 1, 2025

Choose a reason for hiding this comment

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

I think we'd be better off targeting toGlobalSelector('*:focus-visible'). We're missing things like area, video controls, summary.

Copy link
Contributor

@MichaelArestad MichaelArestad Dec 2, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Member

@Sidnioulz Sidnioulz Dec 5, 2025

Choose a reason for hiding this comment

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

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-visible pseudo-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

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}`,
Comment on lines +261 to +264
Copy link
Member

@Sidnioulz Sidnioulz Dec 1, 2025

Choose a reason for hiding this comment

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

Suggested change
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',
Copy link
Member

@Sidnioulz Sidnioulz Dec 1, 2025

Choose a reason for hiding this comment

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

Suggested change
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.

Copy link
Contributor

@MichaelArestad MichaelArestad Dec 2, 2025

Choose a reason for hiding this comment

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

@Sidnioulz Do you think we should avoid setting the color on the focus outline or am I misreading this comment?

Copy link
Member

@Sidnioulz Sidnioulz Dec 5, 2025

Choose a reason for hiding this comment

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

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.

},
},
},
Comment on lines +258 to +271
Copy link
Contributor

@coderabbitai coderabbitai bot Dec 1, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

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:

  1. Escape hatch and anchor protection are bypassed
    The :not(.sb-anchor, .sb-unstyled, .sb-unstyled <element>) part only applies to the last selector (select), so a, button, input, and textarea are matched even inside .sb-unstyled content and on .sb-anchor elements, breaking the documented “unstyled” and anchor behavior.

  2. select elements are never matched
    Because select is also included inside the :not(...) list, select:not(...select...) cannot match any element, so select elements never receive the new :focus-visible outline. 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.

[toGlobalSelector('pre')]: {
...reset,
// reset
Expand Down
Loading