-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
use theme color for all shadows for focus state in primary button #16275
use theme color for all shadows for focus state in primary button #16275
Conversation
Yep, this seems good to me designwise, so I would suggest it needs an accessibility insight (focus style/contrast) moreso than design insights! Thanks for the focused PR! Do you think you could provide screenshots for the other themes? That would make a review much easier. |
@@ -90,7 +90,7 @@ | |||
box-shadow: | |||
inset 0 -1px 0 color(theme(button) shade(50%)), | |||
0 0 0 1px $white, | |||
0 0 0 3px $blue-medium-focus; | |||
0 0 0 3px color(theme(button) shade(50%)); |
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.
Would it be expected that the value produced by this will be different for the default value? In other words, is color(#0085ba shade(50%))
the same as what was previously $blue-medium-focus
? And if it isn't, do we care to consider it a regression?
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.
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.
My first inclination was to say that this should be color(theme(button))
, but I dug into it a bit more and realized why our current focus color doesn't match the default blue color already. Looks like the default blue color is #0085ba
, and $blue-medium-focus
works out to #007cba
. These two colors are really close, but the main difference is that the focus color passes WCAG AA contrast tests, whereas the default blue color doesn't. I think that's why our focus color is not currently the same as color(theme(button))
.
For this PR, I think we should use color(theme(button) shade(5%))
. This works out to #007eb1
, which passes the WCAG AA contrast test, is really close to our current color, and is also used elsewhere in the file.
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.
Awesome, thanks – will make that change asap :)
Have updated based on @kjellr feedback - ready for re-review/next step. Is there something I should do about the CI test failure? |
Looks good, thanks @haszari — We should probably get an a11y check on the new focus colors (though I don't believe all of the alternate color schemes pass WCAG AA contrast tests in the first place. 🤔) Here are screenshots of the change in the alternate color schemes: |
I rebased anyway, and it's still looking good :) |
Can't review in depth as I'm pressed for time, so reviewing based on the screenshots. And overall, this looks good. But I'm missing the darker theme color from the initial PR: Take the following, for example: The focus outline is yellow on white, and whiel I haven't actually tested the contrast ratio, I would expect it to not pass AA. While there has historically been an exception to strict accessibility/contrast rules for the non-default themes, it doesn't seem like theres' a good reason to not do it, since it the fix could potentially be as simple as bumping the percentage for the Nice work. |
Yeah, I tested a few of those button colors, and the buttons themselves don't pass AA. 😐 Still, I definitely get your drift. Since our current blue does pass though, I wonder if we could just target the non-default color schemes with that change? It'd take an extra style like this:
Or is that too hacky? |
Don't jump through hoops to make this happen. I still hope that we'll be able to redesign the WordPress buttons in a holistic way, focus styles and even themes inclusive. There's a core trac ticket for that. While a quick fix for the alternate style focus outlines would be good, it shouldn't be done at any cost. |
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.
From a code perspective, this looks good to me 👍
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.
LGTM. The way this is built, if those alternate color palette colors end up being changed to be accessible in the future, the focus states will update to match. 👍 That sounds like a good plan.
Thanks @haszari!
Description
Use the PostCSS-provided theme colors consistently in button component stylesheet. This ensures that when the button's color is overridden (admin themes, or custom PostCSS) the focus rect is based on the
button
color :)How has this been tested?
I have briefly tested this by making the same change in this package in my project's
node_modules
(!).Screenshots
After :
Before:
Types of changes
Checklist: