Skip to content
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

Open
wants to merge 9 commits into
base: trunk
Choose a base branch
from

Conversation

im3dabasia
Copy link
Contributor

What?

Closes #67747

This PR standardizes focus styles for buttons rendered via ItemGroup Button and the Button 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?

  • Updated ItemGroup Button styles to align with the Button component’s focus behavior.
  • Applied :focus:not(:disabled) for a more consistent and accessible focus style.

Screenshots or screencast

  1. The issue: In :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.)
Screenshot 2025-02-24 at 1 44 12 PM
  1. The focus styles are only applied when using :focus-visible, meaning the button does not receive focus styling when clicked with a mouse.
Screenshot 2025-02-24 at 1 48 20 PM
  1. This is the Button component in its :focus state. We aim to replicate this behavior in the ItemGroup button for consistency.
Screenshot 2025-02-24 at 1 31 04 PM
  1. The solution: After adjusting the CSS, we have ensured that both the Button and ItemGroup button now behave consistently. :focus styles have been added to ItemGroup, and :focus-visible styles have been removed.
Screenshot 2025-02-24 at 1 25 58 PM

@im3dabasia im3dabasia marked this pull request as ready for review February 24, 2025 10:53
Copy link

github-actions bot commented Feb 24, 2025

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 props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: im3dabasia <im3dabasia1@git.wordpress.org>
Co-authored-by: afercia <afercia@git.wordpress.org>
Co-authored-by: ciampo <mciampini@git.wordpress.org>
Co-authored-by: mirka <0mirka00@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@im3dabasia
Copy link
Contributor Author

im3dabasia commented Mar 3, 2025

Hey @afercia ,

When you have a moment could you please review my PR.

Thanks

@afercia
Copy link
Contributor

afercia commented Mar 5, 2025

@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.
When you have a chance, could you please add a changelog entry to the components package changelog?

@afercia afercia added [Type] Bug An existing feature does not function as intended [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Package] Components /packages/components labels Mar 5, 2025
outline: 2px solid transparent;
outline-offset: 0;
// Windows High Contrast mode will show this outline, but not the box-shadow.
outline: 3px solid transparent;
}
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

@afercia afercia left a 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.

@im3dabasia im3dabasia force-pushed the fix/add-consistency-in-button-components branch from 39882f5 to 4b850db Compare March 6, 2025 06:04
@im3dabasia im3dabasia requested a review from afercia March 6, 2025 06:41
@im3dabasia
Copy link
Contributor Author

Thank you @afercia , for the correction in the log. I appreciate it !

@afercia
Copy link
Contributor

afercia commented Mar 6, 2025

I thought it was quicker to fix this way 🙂

Copy link
Contributor

@afercia afercia left a 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.

@afercia
Copy link
Contributor

afercia commented Mar 6, 2025

My bad, I think I misunderstood that most of the Item components of the ItemGroup usages receive the as prop with a button value. In fact, with this PR most items now show the focus style also when clicking the button.

For example, all the navigation items in the Site editor navigation panel will now show the focus style on :focus instead of :focus-visible and many Items in the Styles panel as well. This is exactly what this PR is supposed to do, I'm just willing to make clear the extent of this change.

From an accessibility perspective, focus indication should always be visible also when using a pointing device (mouse). Instead, :focus-visible should be used rarely and only for very good reasons. That's why I still think this is a good change but I just want to be sure we are all aligned.

Also, I think the all the base components in the @wordpress/components package should use a consistent focus style otherwise the styling is based on assumptions because we don't know where the base components will be used. If desired, the focus style should be changed in the usage of the base components in the editor or other applications built by other consumers of the package but I don't think that opinionated styling should be built-in. Cc @WordPress/gutenberg-components

One place where this can be tested is in the Site Editor > Styles > Typography > Fonts:

Screenshot 2025-03-06 at 14 11 40

The list of items now shows the focus style also when clicking.
To test the alternative behavior when the as prop is a, change this line:

https://github.com/WordPress/gutenberg/blob/trunk/packages/edit-site/src/components/global-styles/font-family-item.js/#L32

to <Item as="a" href="#" ...

then test navigating and activating the items with mouse and keyboard and observe the focus style is not shown when clicking.

@ciampo
Copy link
Contributor

ciampo commented Mar 6, 2025

Will try to address the points raised:

Re. the goal of this PR:
If I recall, the Item component didn't have styles for button and a by design (the style function is in fact called unstyledButton), I guess with the assumption that a consumer could instead specify as={ Button } in case they wanted Button styles (although I don't have a lot of time to experiment with the code myself).

Re. using :focus instead of :focus-visible, are there official docs about the fact that "focus indication should always be visible also when using a pointing device (mouse)"? I couldn't find any clear mention of that (and I actually found a few sources identifying :focus-visible as an improvement over :focus).
In any case, I guess there are design implications too, which should not be disregarded. I imagine :focus-visible was chosen specifically to avoid showing a focus ring when clicking those buttons, since the focus ring would probably quickly "flash" before animating to the new view.

Re. using consistent focus styles, that is the general intent. In fact, even before this PR, I believe the Item focus styles were using the same var(--wp-admin-border-width-focus), the same COLORS.theme.accent and the same outline settings as other components.

@im3dabasia
Copy link
Contributor Author

Thank you @ciampo for your feedback.

If achieving consistency is our goal, we could also consider the following approach:

We could adjust the original <Button> component to use :focus-visible instead of :focus. This way, we would have consistent styling between both button types - ItemGroup as="button" and the <Button> component - by using the same :focus-visible approach.

What do you think about this alternative change? It would maintain consistency while avoiding the potential design drawbacks of the current approach.

Looking forward to your thoughts, @afercia and @ciampo.

@mirka
Copy link
Member

mirka commented Mar 10, 2025

We've been discussing the focus-visible strategy in #65088, and I'd say we've achieved a consensus that we shouldn't prefer focus-visible globally by default. Rather, the more sensible strategy we've agreed on is to only apply it over the default focus in selective cases, when there is good enough reason to do so.

@im3dabasia
Copy link
Contributor Author

We've been discussing the focus-visible strategy in #65088, and I'd say we've achieved a consensus that we shouldn't prefer focus-visible globally by default. Rather, the more sensible strategy we've agreed on is to only apply it over the default focus in selective cases, when there is good enough reason to do so.

That being said, this PR aims to mimic the :focus being used in the <Button> component for the ItemGroup as "button". So we are aligning with the previous consensus. However, I should add that currently we are adding focus to just the ItemGroup as a "button" only. For ItemGroup as an "a" anchor tag, we have retained the old :focus-visible.

If we agree, we can have the same set of CSS applied to both the ItemGroup as button and as anchor.

cc: @mirka @afercia

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Package] Components /packages/components [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make Button and ItemGroup 'Item as a button` focus indication consistent
4 participants