-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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.
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
Components: Ensure Consistent Button Focus Styling Across ItemGroup Button
and Button
#69289
base: trunk
Are you sure you want to change the base?
Components: Ensure Consistent Button Focus Styling Across ItemGroup Button
and Button
#69289
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. |
Hey @afercia , When you have a moment could you please review my PR. Thanks |
@im3dabasia thanks for your PR. We need to find another example for the testing instructions because #68672 changed the Duotone button and now it's not an ItemGroup button any longer. Changes look good to me, will test shortly. |
outline: 2px solid transparent; | ||
outline-offset: 0; | ||
// Windows High Contrast mode will show this outline, but not the box-shadow. | ||
outline: 3px solid transparent; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I wasn't clear. What I meant in #67747 is that the focus should be consistent only when the Item is rendered as a Button components, that is for example: <Item as="button" { ...toggleProps }>
. The as
prop is passed here so that it could be used to distinguish the focus style.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have corrected it. Previously, the style was applied to Items as 'button' or as 'a'.
After 0355271 the focus styles will be applied only on the button and otherwise it would default to the old style ie focus-visible for the other ItemGroup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So far this PR changes the focus style unconditionally. All ItemGroup <Item>
components would use the new focus style. As an accessibility specialist I would have no objections and I'd recommend it but I'm afraid it would be too much for the design team.
I'd suggest to use :focus
only when the Item has the as="button"
prop and use :focus-visible
otherwise.
39882f5
to
4b850db
Compare
Thank you @afercia , for the correction in the log. I appreciate it ! |
I thought it was quicker to fix this way 🙂 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR works as intended. However, I would like to get some more feedback before merging. See following comment.
My bad, I think I misunderstood that most of the For example, all the navigation items in the Site editor navigation panel will now show the focus style on From an accessibility perspective, focus indication should always be visible also when using a pointing device (mouse). Instead, Also, I think the all the base components in the One place where this can be tested is in the Site Editor > Styles > Typography > Fonts: The list of items now shows the focus style also when clicking. to then test navigating and activating the items with mouse and keyboard and observe the focus style is not shown when clicking. |
Will try to address the points raised: Re. the goal of this PR: Re. using Re. using consistent focus styles, that is the general intent. In fact, even before this PR, I believe the |
Thank you @ciampo for your feedback. If achieving consistency is our goal, we could also consider the following approach: We could adjust the original What do you think about this alternative change? It would maintain consistency while avoiding the potential design drawbacks of the current approach. |
We've been discussing the |
That being said, this PR aims to mimic the If we agree, we can have the same set of CSS applied to both the ItemGroup as button and as anchor. |
What?
Closes #67747
This PR standardizes focus styles for buttons rendered via
ItemGroup
Button and theButton
component, ensuring visual consistency.Why?
Currently, buttons in the repository use different approaches for focus styles—some rely on
:focus
, while others use:focus-visible
. This inconsistency leads to uneven focus behavior, impacting both UI consistency and accessibility.A similar issue was highlighted in this PR comment, where the focus styles in the trunk were deemed superior to the newer changes. Additionally, after the merge of this PR, ItemGroup was replaced with a Button, resolving part of the inconsistency.
How?
Screenshots or screencast
:focus
, the CSS is set to none, as shown here. This is inconsistent with the Button component, which has a box-shadow and outline applied in the:focus
pseudo-state. (See the 3rd screenshot for reference.):focus-visible
, meaning the button does not receive focus styling when clicked with a mouse.:focus
state. We aim to replicate this behavior in the ItemGroup button for consistency.