Button: Improve the aria-disabled focus style #62480
Conversation
afercia
added
[Type] Bug
|
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. |
afercia
changed the title
|
Size Change: +59 B (0%) Total Size: 1.76 MB
ℹ️ View Unchanged
|
afercia
force-pushed
the
fix/disabled-button-focus-style
branch
from
389ffbd
to
7f87109
Compare
|
Flaky tests detected in 496e303. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/10185809296
|
afercia
force-pushed
the
fix/disabled-button-focus-style
branch
2 times, most recently
from
02dd448
to
80f92ea
Compare
afercia
force-pushed
the
fix/disabled-button-focus-style
branch
from
e38fa2f
to
99d40f6
Compare
afercia
force-pushed
the
fix/disabled-button-focus-style
branch
from
99d40f6
to
7ecad37
Compare
| color: $components-color-accent; | ||
| } | ||
|
|
||
| // Unset some hovers, instead of adding :not specificity. |
There was a problem hiding this comment.
Reading this comment, there could be some unintended consequences across the editor in terms of rule specificity with these changes
There was a problem hiding this comment.
The CSS that was replaced by this was &:not(:disabled):not([aria-disabled="true"]):not(.is-secondary):not(.is-primary):not(.is-tertiary):not(.is-link):hover; I suspect that this was just intended for CSS simplification. It's always possible there's some unexpected consequences, but I don't think it's a significant concern.
There was a problem hiding this comment.
|
|
||
| &:disabled, | ||
| &[aria-disabled="true"] { | ||
| color: $gray-600; |
There was a problem hiding this comment.
If we wanted to retain the original design, we could also use color-mix() to add an exact amount of opacity to the default text color
There was a problem hiding this comment.
Also, looking at other parts in the repo, $gray-700 could be a better value here, should we not go for the color-mix() and opacity route.
But I'll let design folks comment with better context
There was a problem hiding this comment.
It's not entirely clear to me why the default and secondary variant use inconsistent color methods (opacity vs specific value). The opacity method seems to pre-date the specific value, so it's probably fine to change this up.
There was a problem hiding this comment.
$gray-700 would raise the color contrast to be more visible than I think it should be for a disabled control; we still need to pretty clearly convey that this control is disabled.
There was a problem hiding this comment.
If I understand correctly, then, you suggest using solid color? Which one would be the correct one, the 600 or 700 gray variant? (or any other ?)
There was a problem hiding this comment.
I don't have a preference on the opacity vs solid color question; but for a solid color, I'd say 600.
There was a problem hiding this comment.
One of the reasons why I would prefer solid colors over colors with opacity or alpha transparency is that measuring the color contrast ratio is way easier with solid colors. @jameskoster I understand some blocks e.g. the cover text need opacity/alpha but for user interface controls colors I'd prefer to always use solid colors. Any reason, from a design perspective, to not do that?
There was a problem hiding this comment.
A solid color seems more intentional, but might not work as well when the button appears on a non-white background. However opacity isn't perfect in that regard either, so changing this is probably okay. gray-600 seems a good fit.
There was a problem hiding this comment.
As we start adopting more structured theming systems (not necessarily the Theme component, but any system that would allow arbitrary background colors), it would definitely be easier to programatically ensure sufficient contrast if we used solid colors. So if contrast is ever a concern, I would recommend avoiding opacity.
There was a problem hiding this comment.
Thanks @mirka, that's a very good point I'd support it totally.
| svg { | ||
| opacity: 0.66; | ||
| } |
There was a problem hiding this comment.
This update from 0.3 to 0.6 opacity for SVG in disabled options should also probably be reviewed by the design team
There was a problem hiding this comment.
Any reason for the svg not to use the same color as the button text? That will eliminate any inconsistencies between text + icon when both are visible.
There was a problem hiding this comment.
I assume that this is intended to shift the SVG to have a similar rendering to the text color? This change isn't reflected in the screenshot shared in the PR; I think that this is too dark, and results in insufficient difference between active and disabled buttons.
Image shows the undo/redo buttons; the redo button has the opacity of .66 applied, and the difference is barely noticeable.
There was a problem hiding this comment.
the redo button has the opacity of .66 applied, and the difference is barely noticeable.
@joedolson I'm not sure I'm seeing the same thing. Will comment on the PR
| color: $components-color-accent; | ||
| } | ||
|
|
||
| // Unset some hovers, instead of adding :not specificity. |
There was a problem hiding this comment.
The CSS that was replaced by this was &:not(:disabled):not([aria-disabled="true"]):not(.is-secondary):not(.is-primary):not(.is-tertiary):not(.is-link):hover; I suspect that this was just intended for CSS simplification. It's always possible there's some unexpected consequences, but I don't think it's a significant concern.
| color: $components-color-accent; | ||
| } | ||
|
|
||
| // Unset some hovers, instead of adding :not specificity. |
There was a problem hiding this comment.
|
|
||
| &:disabled, | ||
| &[aria-disabled="true"] { | ||
| color: $gray-600; |
There was a problem hiding this comment.
$gray-700 would raise the color contrast to be more visible than I think it should be for a disabled control; we still need to pretty clearly convey that this control is disabled.
| svg { | ||
| opacity: 0.66; | ||
| } |
There was a problem hiding this comment.
I assume that this is intended to shift the SVG to have a similar rendering to the text color? This change isn't reflected in the screenshot shared in the PR; I think that this is too dark, and results in insufficient difference between active and disabled buttons.
Image shows the undo/redo buttons; the redo button has the opacity of .66 applied, and the difference is barely noticeable.
|
I can also confirm (via the manual visual regression tests) that, with the current changes in this PR, the differences with respect to
|
|
Realted to https://github.com/WordPress/gutenberg/pull/62480/files#r1684570141 I'm not sure we're seeing the same color while testing this PR. In the screenshot below:
The change to the SVG opacity which is now Rather, the text color is different and I'd agree is too dark as pointed out. I'll look into it.
|
afercia
force-pushed
the
fix/disabled-button-focus-style
branch
from
7ecad37
to
8d8e966
Compare
|
In the last commits I'm introducing a new color Screenshots where I added buttons text to better illustrate: Before:
After:
I'm using the new gray for all the Button variants, including Screenshot: Disabled:
Enabled:
|
|
A new variable should probably be discussed separately. Any reason not to use gray-600 for now? |
I wanted to use the exact, same color that is used on trunk now. The new
I thought it is too dark. See screenshot, the second 'Redo' is too dark to be clearly distinguished from the enabled 'Undo':
|
No problem. Indeed it's quite strange that the 500 value is missing from the scale. It just feels like a detail to discuss on it's own. That said it is easy to adjust later if required, so I won't object if Marco and Lena are happy :) |
|
Thanks everyone for your feedback and thoughts. I no objections, I'd appreciate a final review by testing all the Button variants 🙏🏽 |
|
I believe @afercia is currently AFK, and therefore I'm happy to take over. @joedolson I think that your blocking feedback has been addressed, but wanted to double-check.
I'd also ask for folks to do one last round of smoke testing around the editor. I'll also do one more round of visual regression on the |
|
Yep. I'm good. |
|
I did a visual snapshot test of the Button variant matrix, and there is a regression in the
(I will be adding a few more to the Button matrix, since in trunk it's currently missing the |
ciampo
force-pushed
the
fix/disabled-button-focus-style
branch
from
9157453
to
84417c3
Compare
|
@mirka I pushed an update which should fix the regression that you noticed on pressed + disabled buttons. Let's wait on final feedback from design folks on the overall changes (and on using a new shade of |
|
I reverted disabled text to use This is how all
The focus outline is still fully visible (ie. not transparent) even for disabled (but keyboard accessible) buttons. I'd be in favour of merging this PR as-is, as it represents an improvement over what's on I will also be AFK for a week starting from tomorrow, so please feel free to merge this PR if it gets approved. |
ciampo
dismissed
joedolson’s stale review
Joe mentioned he's ok with the latest changes, #62480 (comment)
|
I'm going to go ahead and merge to unblock, since it seems like @afercia is on vacation 😄 |
Fixes #62278
What?
The focus style for
aria-disabledbuttons is inconsistent across the button variants. Also, for some variants, it doesn't meet the WCAG contrast guidelines. For screenshots of the inconsistencies and insufficient color contrast of the focus style, please see the issue #62278.Why?
For better accessibility, while the button itself may have some styling to visually indicate it's 'disabled', the focus style needs to be fully visible. The focus outline is a positional marker to allow a user to identify their current position in the page, and that needs to meet contrast guidelines.
How?
opacityproperty for the button element styling. The only opacity value is now used for the SVG icon, when disabled.$gray-600: #949494except for theis-destructivebutton.There are two visible changes:
$gray-600 #949494instead of opacity so it's slightly darker but it's consistent with the gray used for other variants.is-linkbutton, when disabled, is now gray instead of blue. I'm not too concerned about this change because, first of all, links can't be really disabled. They should not have a disabled styling to start with. This is under discussion in Button component: link buttons should not use aria-disabled #62281. Anyways, I don't think it should stay blue (although dimmed) when 'disabled' because, historically, in WordPress the color blue indicates an enabled interactive control.Testing Instructions
npm run storybook:devhttps://wordpress.github.io/gutenberg/
disabledprop set totrue.disabledand__experimentalIsFocusableprops set totrue.Testing Instructions for Keyboard
Screenshots or screencast
Example screenshots to illustrate:
Before:
After: also arira-disabled buttons have a fully visible, consistent, focus outline: