ColorPalette: Improving accessibility and visibility

[t-hamano]

Description

PR to improve the accessibility and visibility of the ColorPalette component.

I think that the new design of the ColorPalette component has the following three problems.

• Accessibility for keyboard users is inappropriate because the appearance of "Custom color picker" button does not change when the focus is on it.
• Not be able to see that there is a custom color picler there because it will be assimilated into the background color when you select the white color.
• The color code of the text is fixed at #fff, which makes the text difficult to read when a color close to white is selected.

colorpalette_before.mp4

I have taken the following actions to solve those problems.

• Add a thin border to the palette area, similar to the Circular picker.
• Change the border style when the palette area is in focus or when the drop-down is open.
• Calculate contrast from color picker values and switch text color values.

colorpalette_after.mp4

This is the first time I've had a pull request with a specific code.
Please point it out if you find anything inappropriate,

Types of changes

Improving accessibility and visibility

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• I've tested my changes with keyboard and screen readers.
• My code has proper inline documentation.
• I've included developer documentation if appropriate.
• I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).

[mirka]

Great and necessary improvements, thank you!

Pinging @jasmussen for a quick design check, especially re: the thin border on the custom color button.

[jasmussen]

Nice! Thank you for the PR. Some nice improvements here. I just opened #36998 detailing some fixes we need to do, including the addition of the basic border.

However instead of creating a pseudo element to overlay the border, you should be able to just do this:

.components-color-palette__custom-color {
	...
	box-shadow: inset 0 0 0 $border-width rgba(0, 0, 0, 0.2);
	outline: 1px solid transparent; // Shown to high contrast modes.
}

I would also echo @mirka around focus, to keep it just for focus alone. But I'd use the same focus style we use elsewhere, something like:

.components-color-palette__custom-color {
	...
	box-shadow: inset 0 0 0 $border-width rgba(0, 0, 0, 0.2);
	outline: 1px solid transparent; // Shown to high contrast modes.

	&:focus {
		box-shadow: inset 0 0 0 var(--wp-admin-border-width-focus) var(--wp-admin-theme-color);
		outline: 2px solid transparent; // Shown to high contrast modes.
	}
}

What do you think?

Nice work!

[t-hamano]

Thanks so much for the code and the design review!

However instead of creating a pseudo element to overlay the border, you should be able to just do this:

The edge will look jagged because it overlaps the checkerboard background if box-shadow is applied to the element itself.

Like this:

This is why I added border on the pseudo-element.

But there is no problem If the text is used to indicate "not set" instead of a checkerborad background as mentioned in #36998.
Alternatively, I think you can solve this problem by not using inset in box-shadow.

And personally I think admin-theme-color is too conspicuous for the color in focus.
I think it would be better to use the same gray color as the circular picker to maintain a sense of unity.

[jasmussen]

I think the jagged edge is okay because yes we do indeed need to change the transparency for the unset color. It's useful for colors that include transparency, but it's not clear enough for "unset" colors. But thank you for considering those details!

[t-hamano]

Thank you for reply.
I think more discussion is needed to improve the UI of the custom color picker, so you can freeze this PR for now.

[jasmussen]

I think more discussion is needed to improve the UI of the custom color picker, so you can freeze this PR for now.

I don't think this PR needs to be blocked on the color picker discussions! I think if we make the changes (inset box shadow, tweaked focus style) this PR would be an excellent step forward. I notably like the contrast checking for the overlay text. Let me know if you need any help, I'd be happy to push some tweaks to the branch.

Thanks again.

[t-hamano]

I understand !
So should I make the changes (inset box shadow, tweaked focus style) and make additiolal commit?

[jasmussen]

If you can commit the changes to an inset box shadow and the focus style, yes, we can land this as a great improvement. The tweak to the transparent custom color card needs to be looked at separately. I hope I'll get time to do it, otherwise someone else is. You are most welcome to contribute of course, but you don't have to. What you have made here is already a big step forward, thank you!

[jasmussen]

Thanks so much for this. This looks like a good step forward: legible text on a white background, and a good focus style.

I will follow up separately on the transparency grid, but this is excellent.

I left one small note on a position relative rule, but it's nitpick territory. Thanks again.

[jasmussen]

This relative position was probably needed for the abs positioned pseudo element but isn't needed anymore.

[mirka]

Thanks for the improvements! If you could add a brief changelog entry before we merge, that would be great ✨

[t-hamano]

Okay, should I add to "Unreleased > Enhancements"?

[jasmussen]

I think it can be considered a bug fix, but unreleased yes! Thank you

[jasmussen]

@mirka it looks like the PHP unit test failures are related to the beta merge window. Should we just force merge it?

[mirka]

it looks like the PHP unit test failures are related to the beta merge window. Should we just force merge it?

I don't think I have rights to do that, but if you do, go ahead! In any case I think we'll be unblocked when #37007 lands.

[jasmussen]

Ah, we can wait for that one. Thank you!

[mirka]

@t-hamano The CI tests seem to be fixed now, could you do a final rebase? 🙏

[t-hamano]

@mirka

I grabbed the latest commit from rebase, combined my work into one commit, and force pushed it.

I'm not familiar with rebase, so please point out any inappropriate points if any...:sweat:

Also, PHP unit tests passed, but one of the E2E tests got 400 error.
Is there anything I need to do to fix this?

[jasmussen]

I've restarted the test, let's hope it passes. Thanks again!