gutenberg icon indicating copy to clipboard operation
gutenberg copied to clipboard

ColorPalette: Ensure text label contrast checking works with CSS variables

Open t-hamano opened this issue 2 years ago • 8 comments

Closes: #41459

What?

This PR performs a contrast check for text labels displayed in the custom color palette, taking into account CSS variables, and displays them in the correct color.

Why?

Passing a CSS variable to contrast() function of the colord returns an incorrect contrast value because it cannot interpret the value correctly. As a result, the text labels in the custom color palette will always be white (#fff).

How?

As was done in #47181, I used computed colors from the custom color palette for the contrast check.

Issues about transparency

I have discovered that the contrast() function doesn't take into account the alpha value, as reported in this issue.

For example, the following two return the same value:

console.log( colord( 'rgb(100,100,100)' ).contrast() );
console.log( colord( 'rgba(100,100,100, 0.2)' ).contrast() );

I think that we need to use a library that converts rgba to rgb, or similar logic, and would like to discuss the appropriate approach.

Testing Instructions

In the block editor

Enable Empty Theme and update theme.json as follows:

/test/emptytheme/theme.json
{
	"$schema": "https://schemas.wp.org/trunk/theme.json",
	"version": 2,
	"settings": {
		"appearanceTools": true,
		"layout": {
			"contentSize": "840px",
			"wideSize": "1100px"
		},
		"color": {
			"palette": [
				{
					"name": "Hex Red",
					"slug": "hex-red",
					"color": "var(--wp--custom--hex-red)"
				},
				{
					"name": "RGB Red",
					"slug": "rgb-red",
					"color": "var(--wp--custom--rgb-red)"
				},
				{
					"name": "RGBA Red",
					"slug": "rgba-red",
					"color": "var(--wp--custom--rgba-red)"
				},
				{
					"name": "Hex Green",
					"slug": "hex-green",
					"color": "var(--wp--custom--hex-green)"
				},
				{
					"name": "RGB Green",
					"slug": "rgb-green",
					"color": "var(--wp--custom--rgb-green)"
				},
				{
					"name": "RGBA Green",
					"slug": "rgba-green",
					"color": "var(--wp--custom--rgba-green)"
				},
				{
					"name": "Hex Blue",
					"slug": "hex-blue",
					"color": "var(--wp--custom--hex-blue)"
				},
				{
					"name": "RGB Blue",
					"slug": "rgb-blue",
					"color": "var(--wp--custom--rgb-blue)"
				},
				{
					"name": "RGBA Blue",
					"slug": "rgba-blue",
					"color": "var(--wp--custom--rgba-blue)"
				},
				{
					"name": "Hex Yellow",
					"slug": "hex-yellow",
					"color": "var(--wp--custom--hex-yellow)"
				},
				{
					"name": "RGB Yellow",
					"slug": "rgb-yellow",
					"color": "var(--wp--custom--rgb-yellow)"
				},
				{
					"name": "RGBA Yellow",
					"slug": "rgba-yellow",
					"color": "var(--wp--custom--rgba-yellow)"
				}
			]
		},
		"custom": {
			"hex-red": "#E10800",
			"rgb-red": "rgb(225,8,0)",
			"rgba-red": "rgba(225,8,0,0.3)",
			"hex-green": "#3EC300",
			"rgb-green": "rgb(62,195,0)",
			"rgba-green": "rgba(62,195,0,0.3)",
			"hex-blue": "#337CA0",
			"rgb-blue": "rgb(51,124,160)",
			"rgba-blue": "rgba(51,124,160,0.3)",
			"hex-yellow": "#FFFC31",
			"rgb-yellow": "rgb(255,252,49)",
			"rgba-yellow": "rgba(255,252,49,0.3)"
		}
	}
}
  • Inserts a block that supports the color.
  • Open the Text Color or Background Color menu and click on one of the palettes in the THEME area.
  • Confirm that the text label changes between black and white depending on the color selected.

Note: Palettes with rgba values may result in unintended colors, as mentioned above.

https://user-images.githubusercontent.com/54422211/214237936-379e35af-421f-4c89-b4b7-e3652792810a.mp4

In the storybook

  • Build Storybook and access "ColorPalette" > "CSS Variables".
  • Confirm that the text label has black color when red or yellow is selected.

https://user-images.githubusercontent.com/54422211/214237292-ed8ebc9c-543b-4701-9da9-3d940f401cfd.mp4

t-hamano avatar Jan 24 '23 05:01 t-hamano

cc'ing @brookewp as she's been also looking at how to best take transparent values into account when computing accessible contrast with colord

ciampo avatar Jan 25 '23 14:01 ciampo

TL;DR: we need to find a way to "merge" 2 or more colors into a solid color to be used for colord accessibility calculations, since when the color is transparent, the color underneath affects the contrast too.

I tried the mix() method on 9327bc9 to get a real visible color from a transparent rgba color.

The logic is as follows:

  • In this case, the underlying color (background color of popover content) should be white.
  • If the alpha value of the palette color is less than 1, it is considered transparent.
  • If it is transparent, mix a non-transparent color and #fff.
  • The original alpha value is used for ratio, the second argument of mix().

From what I have tested, it seems to be working well, but what about this approach?

https://user-images.githubusercontent.com/54422211/214759273-fa3cc0df-d116-42ba-878a-063931bccf06.mp4

t-hamano avatar Jan 26 '23 04:01 t-hamano

It seems to be working well!

I think we can refine this approach in a few ways:

  1. We could move it to a separate utility (ie. getCompositeBackgroundColor or something?)
  2. I'd like to find a way to avoid having a hardcoded #fff value. But this is not trivial, since background-color can not be read from getComputedStyle on the parent.
  • We could force the background color inside the component to be var( --wp-components-color-background ), and therefore we could then run getComputedStyle on that DOM element and use it for the computation
  • Alternatively (but this is way more convoluted), we could find a way of "taking a screenshot" of the element, print it to a canvas in memory, then sample the resulting color. This second method would be also better in case of multiple layers of transparent background colors

What do you think? Curious to hear @mirka 's thoughts too

ciampo avatar Jan 26 '23 21:01 ciampo

This is great! I tried the 'alpha blending' formula for https://github.com/WordPress/gutenberg/pull/47476 (I quickly put it up as a draft to share) in relation to this issue https://github.com/WordPress/gutenberg/issues/42715 which yielded nearly identical results. However, your solution is much cleaner. :)

Screenshot 2023-01-26 at 3 58 40 PM

Since you're discussing how to approach the background color, I wanted to share the above PR/issue as it's been requested to show the checkered background behind semi-transparent color values. I believe we should use grey rather than white for the background, as I think there will be more readability issues with the text over the grey. What do you all think?

brookewp avatar Jan 27 '23 01:01 brookewp

Thank you for chiming in, Brooke!

which yielded nearly identical results. However, your solution is much cleaner. :)

Awesome! The timing of the two of you working on these related issues couldn't have been better :)

it's been requested to show the checkered background behind semi-transparent color values

Thank you for sharing this and giving more perspective on the requirements for this component!

I believe we should use grey rather than white for the background, as I think there will be more readability issues with the text over the grey. What do you all think?

I'm not 100% sure I understand your suggestion here — I can see 2 ways of interpret it:

  1. Visually show a grey background as the background color for the ColorPalette picker
  2. Continue to show a white background, while using a mid grey (instead of white) when computing the final blended color, to make up for the checkered background

The challenge here is that we can't assume that the background would always be white, especially since soon we may expose the ability for consumers of the package to change the background color to any color), and therefore we can't assume in advance what will the resulting "average" color be.

I also though about whether we could instead just assume a "darkened" version of the background, but that also wouldn't work well in case we ever switched to a "dark" theme.

I think it's probably ok to "ignore" the impact of the checkered color for now — we can take a look at the results and iterate if necessary?

ciampo avatar Jan 27 '23 09:01 ciampo

Flaky tests detected in 294598229e59a5ddaac3bda46f1a064a215a2ae9. Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4109786938 📝 Reported issues:

  • #47632 in specs/editor/blocks/query.test.js
  • #39787 in specs/editor/various/multi-block-selection.test.js
  • #46984 in specs/editor/blocks/heading.test.js

github-actions[bot] avatar Jan 28 '23 15:01 github-actions[bot]

Thanks for the review, @ciampo!

We could move it to a separate utility (ie. getCompositeBackgroundColor or something?)

I have responded at 98bf1dc. If you find anything inappropriate, please feel free to commit 🙏

We could force the background color inside the component to be var( --wp-components-color-background ), and therefore we could then run getComputedStyle on that DOM element and use it for the computation

Does this variable var( --wp-components-color-background ) have anything to do with the themeable color implemented in #45466?

t-hamano avatar Jan 29 '23 08:01 t-hamano

My suggestion is to keep the scope of this PR to the CSs variable issue only, and deal with alphas as a separate issue.

This is because I think the alpha issue may require some design changes that should get proper input from the design team. We probably do in fact need to keep the checkered background showing through to signal that the color is semi-transparent. In this case we will always (I think?) run into readability issues at some point in the gradient scale if we put text on top of that checkered pattern. I think it's inevitable that we modify the design so it accommodates this case — for example, placing the text outside of the swatch, or placing the text on a pill-style solid background. The current design really does not take into account things like CSS variables (i.e. longer strings) and transparency.

Overcomplicating the algorithm may not be necessary once the design issue is addressed!

mirka avatar Jan 30 '23 13:01 mirka

Update:

  • ✅ Don't consider transparent color
  • ✅ Don't use ref, simplify normalizeColorValue() function
  • ✅ Added JSDoc to normalizeColorValue() function
  • ❌ Added two unit tests

In my unit tests, I expected the following, but both tests failed.

  • Custom color picker should have label color based on actual background color
  • When the custom color picker button is clicked, The color picker should have a HEX value based on the computed color

It appears that the normalizeColorValue() function is not getting the computed style of the element correctly. Or maybe I'm not passing the CSS variables to the test component correctly.

If there is a way to resolve this, I would love to hear about it 🙏

t-hamano avatar Feb 04 '23 14:02 t-hamano

Mhh, it looks like CSS custom properties are not fully supported in jsdom. In case we can't write tests at the component level, we could at least write unit test for the normalizeColorValue function 🤷

ciampo avatar Feb 06 '23 15:02 ciampo

I have changed to a unit test for the normalizeColorValue() function in e225bf5. I generated the argument element with createElement. The test passes as expected, but I just can't seem to fix the two type errors...

t-hamano avatar Feb 06 '23 16:02 t-hamano

A rebase/merge with trunk should hopefully solve the CI failures (and the conflicts) — after that, we should be ready for final review & approval!

ciampo avatar Feb 07 '23 12:02 ciampo

Thank you, @ciampo!

t-hamano avatar Feb 08 '23 00:02 t-hamano

Cherry-picked this PR to the wp/6.2 branch.

ntsekouras avatar Feb 09 '23 13:02 ntsekouras