Editor: Fix regression and restore the back button focus ring#80029
Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Size Change: -79 B (0%) Total Size: 7.68 MB 📦 View Changed
|
|
Flaky tests detected in bf58e9c. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/29000155915
|
mirka
left a comment
There was a problem hiding this comment.
Given the stronger push for design system adherence now, it might be a good time to remove any ad hoc focus-visibles and see what happens, especially in places like this where it isn't that critical.
| &:focus-visible { | ||
| transform: translateZ(0); | ||
| // TODO: remove once the <Item> component has migrated to the | ||
| // design-system focus ring (`outset-ring__focus`), as <Button> did in #78646. | ||
| // | ||
| // This override is necessary now so the focus ring matches with | ||
| // the editor back button. | ||
| box-shadow: none; | ||
| @include mixins.outset-ring__focus(); |
There was a problem hiding this comment.
Possibly cleaner/safer in the short term?
| &:focus-visible { | |
| transform: translateZ(0); | |
| // TODO: remove once the <Item> component has migrated to the | |
| // design-system focus ring (`outset-ring__focus`), as <Button> did in #78646. | |
| // | |
| // This override is necessary now so the focus ring matches with | |
| // the editor back button. | |
| box-shadow: none; | |
| @include mixins.outset-ring__focus(); | |
| &:focus-visible { | |
| // TODO: remove once the <Item> component has migrated to the WPDS focus color token. | |
| --wp-components-color-accent: var(--wpds-color-stroke-focus); | |
| transform: translateZ(0); |
| // Focus (resets default button focus and use focus-visible). | ||
| &:focus:not(:disabled) { | ||
| box-shadow: none; | ||
| outline: none; | ||
| } | ||
| &:focus-visible:not(:disabled) { | ||
| box-shadow: | ||
| 0 0 0 var(--wp-admin-border-width-focus) | ||
| var(--wp-admin-theme-color); | ||
| outline: 3px solid transparent; | ||
| } | ||
|
|
There was a problem hiding this comment.
There was a problem hiding this comment.
Thanks for the context!
the switch to focus-visible was done intentionally in #48472
I checked and it looks like this PR does not re-introduce the "bug" that the PR was fixing. The behavior is similar to the editor in GB 23.5. So it seems it's fine to keep this removal. (And I agree with your point that we should push for the built-in DS stylings).
There was a problem hiding this comment.
I checked and it looks like this PR does not re-introduce the "bug" that the PR was fixing. The behavior is similar to the editor in GB 23.5.
I'm really sorry, I misunderstood that PR! I agree with the PR that showing focus ring on back button during navigation is distracting. Fixing here: #80114
| // TODO: remove once the <Item> component has migrated to the | ||
| // design-system focus ring (`outset-ring__focus`), as <Button> did in #78646. |
| // TODO: remove once the <Item> component has migrated to the | ||
| // design-system focus ring (`outset-ring__focus`), as <Button> did in #78646. | ||
| // | ||
| // This override is necessary now so the focus ring matches with | ||
| // the editor back button. | ||
| box-shadow: none; | ||
| @include mixins.outset-ring__focus(); |
There was a problem hiding this comment.
However, it means that the focus ring color for the sidebar menu items (like Navigation, Pages, Templates) will be different from the back button. In this PR, we "fix" it by overriding it locally.
I think this seems like an issue that should be addressed separately from this PR, as the color mismatch is already occurring in trunk.
There was a problem hiding this comment.
Thanks, I think you're right. It seems it's better to "fix" this at the source instead of adding more chaos to the CSS 😅 reverted for now!
Co-authored-by: fushar <fushar@git.wordpress.org> Co-authored-by: mirka <0mirka00@git.wordpress.org> Co-authored-by: t-hamano <wildworks@git.wordpress.org>
What?
Fixes a "regression" introduced by #79837.
The sidebar back button in the Site Editor (
.edit-site-sidebar-button) is overriding the default focus styles. It now conflicts with the linked PR because that added this rule:The correct fix here is to just follow what WP DS provides for the focus ring styles, so this PR removes the overrides.
However, it means that the focus ring color for the sidebar menu items (like Navigation, Pages, Templates) will be different from the back button. In this PR, we "fix" it by overriding it locally. The proper fix is to update the<Item>component so that it uses WP DS styles, as done in #78646. I left a// TODOcomment on that.Testing Instructions
<back button in< DesignVerify the focus state for all sidebar items are consistent with the<.Screenshots or screencast
Use of AI Tools
Used Opus 4.8 to investigate the issue.