use theme color for all shadows for focus state in primary button

[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

• Bug fix

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• My code has proper inline documentation.
• I've included developer documentation if appropriate.

[jasmussen]

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.

[aduth]

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?

[haszari]

Hmm, good point. Perhaps this should be color(theme(button)), or a brighter shade. Based on the blue focus rect – looks closer to regular button blue; color(theme(button) shade(50%)) is likely too dark. I think this PR needs (more) design feedback!

[kjellr]

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.

[haszari]

Awesome, thanks – will make that change asap :)

[haszari]

Have updated based on @kjellr feedback - ready for re-review/next step.

Is there something I should do about the CI test failure?

[kjellr]

Looks good, thanks @haszari — I think you may just need to sync up with master to get that tested passing. Once that's all set, I think we're almost good to go. Edit: Looks like this is passing now anyway. 🙂

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:

Default

Light

Blue

Coffee

Ectoplasm

Midnight

Ocean

Sunrise

[haszari]

Looks good, thanks @haszari — I think you may just need to sync up with master to get that tested passing. Once that's all set, I think we're almost good to go. Edit: Looks like this is passing now anyway.

I rebased anyway, and it's still looking good :)

[jasmussen]

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 shade value. Maybe shade(50%) isn't necessary, but would be good to test.

Nice work.

[kjellr]

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 shade value. Maybe shade(50%) isn't necessary, but would be good to test.

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:

body[class^="admin-color-"] .components-button .is-primary:focus:enabled {
	box-shadow:
		inset 0 -1px 0 color(theme(button) shade(50%)),
		0 0 0 1px $white,
		0 0 0 3px color(theme(button) shade(50%));
}

Or is that too hacky?

[jasmussen]

I wonder if we could just target the non-default color schemes with that change? It'd take an extra style like this:

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.

[haszari]

@gziolo @aduth It looks like we've worked through the design / accessibility questions. Is there anything more I should do to keep this PR moving? Or is it time for a final review? Let me know :)

[aduth]

From a code perspective, this looks good to me 👍

[kjellr]

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!