Standardize component focus states on :focus-visible - #1141
Merged
Merged
Conversation
stephaniehobson
force-pushed
the
v23/focus-visible-components
branch
from
September 11, 2026 21:42
40e037a to
e9b1aa0
Compare
stephaniehobson
added this pull request to stack #1150
September 11, 2026 21:42
stephaniehobson
marked this pull request as ready for review
September 12, 2026 04:03
There was a problem hiding this comment.
🟡 Changes recommended
Address the documented browser-compatibility and documentation concerns before approval.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Standardizes component focus styling by replacing 29 bare :focus selectors with :focus-visible.
Changes:
- Updates focus states across 12 component Sass partials.
- Preserves form-control focus styles and existing
:focus-within. - Documents the change in
CHANGELOG.md.
File summaries
| File | Summary |
|---|---|
CHANGELOG.md |
Documents the focus-state changes. |
assets/sass/protocol/components/_sticky-promo.scss |
Updates promo focus styling. |
assets/sass/protocol/components/_sidebar-menu.scss |
Updates sidebar focus styling. |
assets/sass/protocol/components/_notification-bar.scss |
Updates notification focus styling. |
assets/sass/protocol/components/_navigation.scss |
Updates navigation focus styling. |
assets/sass/protocol/components/_modal.scss |
Updates modal focus styling. |
assets/sass/protocol/components/_menu.scss |
Updates menu focus styling. |
assets/sass/protocol/components/_menu-list.scss |
Updates menu-list focus styling. |
assets/sass/protocol/components/_menu-item.scss |
Updates menu-item focus styling. |
assets/sass/protocol/components/_footer.scss |
Updates footer focus styling. |
assets/sass/protocol/components/_card.scss |
Updates card focus styling. |
assets/sass/protocol/components/_button.scss |
Updates button focus styling. |
assets/sass/protocol/components/_breadcrumb.scss |
Updates breadcrumb focus styling. |
Review details
Suppressed comments (3)
CHANGELOG.md:54
- The PR checklist says this change is documented in the design system, but this diff only adds a changelog entry and the
docs/tree has no guidance for:focus-visibleor the form-input exception. Please add the accessibility guidance to the design-system documentation, or correct the checklist before merging this breaking behavior change.
* (breaking) Standardized interactive component states on `:focus-visible` instead of `:focus`, so the focus ring only shows for keyboard/assistive-tech focus rather than every mouse click. Affects Breadcrumb, Button, Card, Footer, Menu, Menu Item, Menu List, Modal, Navigation, Notification Bar, Sidebar Menu, and Sticky Promo. Text inputs keep `:focus` -- see the next entry.
assets/sass/protocol/components/_card.scss:105
- This selector is shipped while
package.json:60-63still declaresdefaultsplus IE 8. IE 8 does not recognize:focus-visible; in grouped rules like this one, the invalid pseudo-class can discard the entire hover/active rule, and the standalone focus rule gives no custom focus state. Either update the supported-browser target as part of this breaking release or retain a fallback for browsers without:focus-visible.
&:focus-visible {
assets/sass/protocol/components/_navigation.scss:203
- Putting
:focus-visiblein this comma-separated selector list makes the entire rule invalid in browsers that do not recognize that pseudo-class; those browsers then lose the existing hover/active background as well as the focus state. This pattern is repeated throughout the changed component partials, and the release browser baseline is not defined yet. Split the hover/active selectors from the:focus-visiblerule (or explicitly gate the new selector with a supported-browser baseline) in every occurrence.
&:focus-visible,
- Files reviewed: 13/13 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
|
||
| ### Accessibility | ||
|
|
||
| * (breaking) Standardized interactive component states on `:focus-visible` instead of `:focus`, so the focus ring only shows for keyboard/assistive-tech focus rather than every mouse click. Affects Breadcrumb, Button, Card, Footer, Menu, Menu Item, Menu List, Modal, Navigation, Notification Bar, Sidebar Menu, and Sticky Promo. Text inputs keep `:focus` -- see the next entry. |
Contributor
Author
There was a problem hiding this comment.
Will be addressed in next PR in stack.
stephaniehobson
commented
Sep 17, 2026
stephaniehobson
force-pushed
the
v23/focus-visible-components
branch
from
September 17, 2026 23:05
5adc9b6 to
f8c59f7
Compare
29 bare :focus selectors across 12 component partials, converted to
:focus-visible so the focus ring only shows for keyboard/assistive-tech
focus rather than every mouse click. Left the single pre-existing
:focus-within in _menu.scss alone -- different pseudo-class, not in
scope.
Breadcrumb, Button, Card, Footer, Menu, Menu Item, Menu List,
Modal, Navigation, Notification Bar, Sidebar Menu, Sticky Promo
Mechanical rename -- all 29 occurrences were the simple &:focus {
or &:focus, shape, no ambiguity with other selectors. Verified with
a targeted grep across the 12 files before and after (0 bare :focus
remaining, 29 :focus-visible present) and a direct sass compile.
Form text inputs/textareas are a separate, deliberately excluded case
(D2, not this commit) -- :focus-visible doesn't reliably match
mouse-focused text fields across engines, so losing the ring on click
there would be a real accessibility regression, not an improvement.
Stacked on v23/desc-to-body (branched here per the plan's documented
fallback, since that stack hasn't merged yet -- will rebase onto main
once it does). Part of #1084.
Verified: npm run lint, npm test (47 specs, Firefox + Chrome), and a
direct sass --verbose compile confirming 18 :focus-visible selectors
in the compiled protocol-components.css with no new warnings.
stephaniehobson
force-pushed
the
v23/focus-visible-components
branch
from
September 21, 2026 18:38
f8c59f7 to
33c7ca4
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
:focusselectors across 12 component partials, converted to:focus-visible.:focus-withinin_menu.scssalone.CHANGELOG.md.Issue
Part of #1084
Testing
npm run lint,npm test(47 specs, Firefox + Chrome), and a direct sass compile all pass.