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

Secondary and tertiary buttons have no focus ring when not fully disabled #56149

Closed
andrewhayward opened this issue Nov 15, 2023 · 3 comments · Fixed by #56383
Closed

Secondary and tertiary buttons have no focus ring when not fully disabled #56149

andrewhayward opened this issue Nov 15, 2023 · 3 comments · Fixed by #56383
Assignees
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). Needs Accessibility Feedback Need input from accessibility Needs Design Feedback Needs general design feedback. [Package] Components /packages/components [Status] In Progress Tracking issues with work in progress [Type] Enhancement A suggestion for improvement.

Comments

@andrewhayward
Copy link
Contributor

What problem does this address?

Buttons support the ability to retain focus while being disabled, via the __experimentalIsFocusable prop. However, when this is enabled on secondary and tertiary buttons, they do not have a focus ring. For example, the "Reset filters" button here is focused and only mostly disabled, but that is not visibly evident.

A toolbar with a search field, two buttons and a dropdown toggle. One button (circled in red) is disabled and focused, but has no focus ring.

This is caused by an issue of specificity in the CSS, with .is-secondary and .is-tertiary overriding previously established focus styling for buttons using box-shadow and outline:

&:disabled,
&[aria-disabled="true"],
&[aria-disabled="true"]:hover {
color: $gray-600;
background: transparent;
transform: none;
opacity: 1;
box-shadow: none;
outline: none;
}

&:focus:not(:disabled) {
box-shadow: 0 0 0 var(--wp-admin-border-width-focus) $components-color-accent;
// Windows High Contrast mode will show this outline, but not the box-shadow.
outline: 3px solid transparent;
}

What is your proposed solution?

The fix could be as straightforward as changing the scope or specificity of one of the other rules, to allow for the focus styling to propagate. This is probably most easily achieved by moving the box-shadow and outline declarations into a sub-block:

diff --git a/packages/components/src/button/style.scss b/packages/components/src/button/style.scss
index 03273056cf..4b11d9169a 100644
--- a/packages/components/src/button/style.scss
+++ b/packages/components/src/button/style.scss
@@ -129,8 +129,11 @@
                        background: transparent;
                        transform: none;
                        opacity: 1;
-                       box-shadow: none;
-                       outline: none;
+
+                       &:not(:focus) {
+                               box-shadow: none;
+                               outline: none;
+                       }
                }
        }

This will at least allow mostly-disabled buttons to pick up the focus styling applied to other controls:

A toolbar with a search field, two buttons and a dropdown toggle. One button is disabled and focused, and has a focus ring.

@andrewhayward andrewhayward added [Type] Enhancement A suggestion for improvement. [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). Needs Design Feedback Needs general design feedback. [Package] Components /packages/components Needs Accessibility Feedback Need input from accessibility labels Nov 15, 2023
@Mamaduka
Copy link
Member

This came up in #54722. cc @jameskoster, @jasmussen

@jasmussen
Copy link
Contributor

Seems like a bug, yes.

@joedolson
Copy link
Contributor

I feel like a disabled control should have a different focus outline, to provide visual affordance that even though you can focus the control, it's not available to use.

This is a fairly ambiguous situation, accessibility-wise; combining the needs of focus visibility with the affordance of a disabled control. I think we should de-saturate the focus ring when disabled controls have focus; possibly even add an additional icon indicator on the control to help indicate that it's disabled. Setting focus on a control usually implies that you can use the control, so we need to provide strong signals visually that this is disabled.

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). Needs Accessibility Feedback Need input from accessibility Needs Design Feedback Needs general design feedback. [Package] Components /packages/components [Status] In Progress Tracking issues with work in progress [Type] Enhancement A suggestion for improvement.
Projects
Status: Done 🎉
Development

Successfully merging a pull request may close this issue.

4 participants