-
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
Add focus rings to focusable disabled buttons #56383
Merged
andrewhayward
merged 2 commits into
trunk
from
56149/secondary-tertiary-buttons-disabled-focus-rings
Nov 22, 2023
Merged
Add focus rings to focusable disabled buttons #56383
andrewhayward
merged 2 commits into
trunk
from
56149/secondary-tertiary-buttons-disabled-focus-rings
Nov 22, 2023
Conversation
This file contains 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
Secondary and tertiary buttons don't have focus rings when they are disabled but `__experimentalIsFocusable` is true.
andrewhayward
added
[Type] Bug
An existing feature does not function as intended
[Package] Components
/packages/components
labels
Nov 21, 2023
Size Change: +59 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
andrewhayward
added
[Type] Enhancement
A suggestion for improvement.
and removed
[Type] Bug
An existing feature does not function as intended
labels
Nov 21, 2023
jasmussen
approved these changes
Nov 22, 2023
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.
Given the simplicity of the bugfix, this seems like the correct place to start.
andrewhayward
deleted the
56149/secondary-tertiary-buttons-disabled-focus-rings
branch
November 22, 2023 17:14
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
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.
What?
Secondary and tertiary buttons don't have focus rings when they are disabled but
__experimentalIsFocusable
is true. This PR updates the button CSS to ensure that they do.Resolves #56149.
Why?
As per WCAG SC 2.4.7 (Focus visible):
It is ambiguous as to whether a disabled button should be considered "operable" or not, but as the purpose of the success criterion is "to help a person know which element has the keyboard focus", that should be prioritised. As such, even when a button is disabled and not actionable, if it can receive focus this state should be indicated appropriately.
How?
The CSS is altered to remove outlines/borders/etc only when the control does not have focus.
Follow-up
Note that no attempt has been made in this PR to change the colour or other styling of the focus ring in this context; there are too many variables at play to safely make assumptions about what works everywhere that a focus ring might be needed. This should however be investigated separately.
Something to consider...
One potential option would be to desaturate the focus ring for disabled controls – we would need to find the right balance between being different enough to denote diverging states, but similar enough to not be confusing.
This could be achieved by using
color-mix
(which has wide browser support) to desaturate the set accent colour automatically:Testing Instructions
Navigate to the Button component in Storybook.
Button types
Primary buttons
Confirm that primary buttons remain unaffected. They should have blue backgrounds, regardless of disabled or focused state. They should have white text, or desaturated blue text when disabled, focusable or not. They should have a heavy blue outline when focused, disabled or not. There should be no pointer change on disabled buttons.
Secondary buttons
Confirm that regular secondary buttons remain unaffected. They should have no background, regardless of disabled or focused state. They should have blue text, or grey text when disabled, focusable or not. They should have a blue outline as standard, but not when disabled. They should have a heavy blue outline when focused, disabled or not. There should be no pointer change on disabled buttons.
Tertiary buttons
Confirm that regular tertiary buttons remain unaffected. They should have no background, regardless of disabled or focused state. They should not have an outline as standard. They should have blue text, or grey text when disabled, focusable or not. They should have a heavy blue outline when focused, disabled or not. There should be no pointer change on disabled buttons.
Testing Instructions for Keyboard
In all cases:
__experimentalIsFocusable
is set totrue
), should be able to receive focus, and should show a focus ring when navigated to