ColorPanel: Fixes missing contrast failure notice.

[tomdevisser]

Right now the colors to check the contrast are being retrieved by using getComputedStyle. This works for some blocks, but not for all.

We already have access to definedColors, and even do some checks, but it was not being used for contrast checking.

I noticed for the button, where the contrast message was missing, the correct colors were present in this definedColors constant. In this commit I use this constant if it's present, and if not, I use the earlier solution as a fallback.

The notice now shows up for buttons, and also still for paragraphs and the cover block.

Fixes #48308.

What?

Fixes missing contrast failure notice.

Why?

Right now the colors to check the contrast are being retrieved by using getComputedStyle. This works for some blocks, but not for all, meaning some blocks (like Buttons) are missing the contrast check.

How?

I noticed for the button, where the contrast message was missing, the correct colors were present in this definedColors constant. In this commit I use this constant if it's present, and if not, I use the earlier solution as a fallback.

Testing Instructions

1. Add a button and a paragraph
2. Change the colors to a failing contrast ratio
3. See the contrast notice appear

Screenshots or screencast

[github-actions]

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @tomdevisser! In case you missed it, we'd love to have you join us in our Slack community, where we hold regularly weekly meetings open to anyone to coordinate with each other.

If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information.

[tomdevisser]

I'm sorry, first time contributor, so I'm still learning. I noticed the commit from the other PR I did today is also in this PR, even though it was made on a separate branch. Probably because I "merged" it with trunk, as well as this one. Can anyone guide me in how to fix this and prevent it next time? Thanks!

[Mamaduka]

Merging PR for the previous branch might be the problem. We don't merge branches into the trunk and keep it as a source of the truth.

Let's try to fix this problem. First, I assume you used common names for your remotes. The origin - points to your fork repo, and the upstream points to the Gutenberg main repo.

Here's an example of how you can fix this when using CLI, but GUI tool steps should be similar.

git checkout trunk

// Fetch upstream and reset the local trunk branch
git fetch upstream trunk
git reset --hard upstream/trunk

// Create a new branch from the trunk
git checkout -b new/buttons-contrast-checker

// Let's pick the commit we want on this branch
git cherry-pick 8d192fa

// If the previous step was successful
// Delete old branch
git branch -D update/buttons-contrast-checker

// rename new and push to the remote
git branch -m update/buttons-contrast-checker

// Only use `--force-with-lease` to overwire remote branches
git push --force-with-lease origin update/buttons-contrast-checker

[aaronrobertshaw]

Thanks for the effort towards improving the contrast checkers @tomdevisser 👍

There have been a few issues pop up lately around these and not all of them have had easy answers. @andrewserong recently explored adding contrast checkers to the Global Styles screens. I wonder if there are any plans there that might impact the best course of action with this PR?

I've given this PR an initial test and it appears to cause a regression.

1. Add a paragraph block and select it
2. Set a black background only and note the contrast warning
3. Deselect the block and re-select it, note the contrast warning is missing. On trunk the warning is displayed

Trunk	This PR
Screen.Recording.2023-02-28.at.7.47.27.pm.mp4	Screen.Recording.2023-02-28.at.7.48.55.pm.mp4

I also found the logic changes a little tricky to grok, perhaps it would be good to add some inline comments to explain things. This was especially true when trying to factor in the possibility that themes can load their own external stylesheets and how this impacts the color detection and when or if warnings should be shown.

[tomdevisser]

Thanks for checking this @aaronrobertshaw , let's wait til @andrewserong tells us if this PR is the way forward or if we should wait for the global checker.

[andrewserong]

@andrewserong #48115 (comment) adding contrast checkers to the Global Styles screens. I wonder if there are any plans there that might impact the best course of action with this PR?

Thanks for the ping! #48115 was a bit of an experiment to see if we could throw the ContrastChecker component over into global styles. It wound up being a lot more complicated than I'd anticipated, so I've parked that exploration for now, and I don't think anything there would impact this exploration. The main notes for global styles was to refactor the ContrastChecker component a little so that its internal logic is based on a reusable hook, rather than only outputting via a rendered component. This PR appears to be about how we gather the colors to pass into the component (which then does the checking), so I think you could continue exploring the ideas here and it'd happily co-exist with any future explorations for global styles.

Thanks for the effort towards improving the contrast checkers @tomdevisser 👍

+1 thanks for diving in to improve the logic here!

Right now the colors to check the contrast are being retrieved by using getComputedStyle. This works for some blocks, but not for all, meaning some blocks (like Buttons) are missing the contrast check.

Just to make sure I'm following along — why does it not work for the Button block, I wonder? Is it looking at the wrong node when using getComputedStyle?

Deselect the block and re-select it, note the contrast warning is missing. On trunk the warning is displayed

Is that because of the included dependency array on the useEffect? Previously it looks like there wasn't a dependency array, so this effect would have fired on every render, if I'm following along correctly.

[tomdevisser]

Thank you, @andrewserong !

Just to make sure I'm following along — why does it not work for the Button block, I wonder? Is it looking at the wrong node when using getComputedStyle?

I'm not sure to be honest!

Is that because of the included dependency array on the useEffect? Previously it looks like there wasn't a dependency array, so this effect would have fired on every render, if I'm following along correctly.

That could definitely be it, but my linter was giving a warning that the empty array could cause infinite loops in some edge cases which is why I added these recommended ones. Do you think it's safe to keep it empty?

[tomdevisser]

I will try to find some time to test those things later this day, but if you have any time yourself feel free to co-author on this one.

[andrewserong]

my linter was giving a warning that the empty array could cause infinite loops in some edge cases which is why I added these recommended ones. Do you think it's safe to keep it empty?

Good question. In principle the linter is probably right in the sense that we only want the effect to fire when we expect it to and no more than that. The tricky thing with this component is that it appears that a simpler approach was to get it to just fire all the time. If we're limiting it, then we probably need to figure out how frequently we want it to fire, and make sure the dependency array is explicit about that. For example, if block selection changes, etc... though when changing blocks, I'd have thought that the ref in the dependency array would do it. One thing to potentially try might be to place clientId in the array, too?

but if you have any time yourself feel free to co-author on this one.

Unfortunately, I'm a little stretched for time to jump into other PRs right now, but happy to test this PR / add feedback when you have time to work on it 🙂

[tomdevisser]

One thing to potentially try might be to place clientId in the array, too?

@andrewserong Will give it a shot, thanks for thinking along!

[tomdevisser]

Deselect the block and re-select it, note the contrast warning is missing. On trunk the warning is displayed

@aaronrobertshaw I've been trying to recreate this issue, but this seems to work on my end.

Edit: Seems like this is broken since the newer Gutenberg, after updating to 15.3.0 it breaks. Trying the fix suggested by @andrewserong now.

contrast.mov

My environment:
wp-env (config in Gutenberg trunk)
Gutenberg 15.2.0
WordPress 6.2-beta3-55437

[tomdevisser]

Since I updated to Gutenberg 15.3.0-rc.1, everything broke down. It didn't even work on first render. But I managed to fix everything and add some inline comments as requested!

Let me know if this looks good to you @andrewserong ! As things are breaking in the new Gutenberg version, I think it would be good to give this PR a bit more priority and to merge it with the next RC if possible.

fixed-contrast.mov

[andrewserong]

Thanks for the continued efforts here @tomdevisser!

I'm not sure if the logic is quite right, yet. I noticed with a theme that sets a default background colour for button elements that the contrast checker is firing when I select only a single text colour, but no background colour (as the background colour is set in theme.json):

In cases where there might not be enough information to make a reliable call as to whether or not there's a contrast issue, I think I'd lean toward not showing the contrast checker. So, in principle, only showing the contrast checker when there's enough information to reliably know that there is an issue.

In the test case above, from logging out values, it seems that it's detecting a white background for some reason, but I wasn't too sure why it was getting that value 🤔:

[aaronrobertshaw]

As @andrewserong said, thank you for continuing the efforts towards improving the contrast checking here 👍

Andrew also reflected on similar concerns and issues I encountered while reviewing this PR's current approach. Given he beat me to posting (🚀), I've rewritten this review so as to not double up there.

The remaining issue for me was that the latest approach changes the contrast-checking behaviour when no colors have been selected.

Previously, if there were no selected colors, no contrast checking was performed as the detected colors were all reset. On this PR branch, if theme.json or global styles define a poor color combination, the user gets the contrast warning about "this color combination" despite the two controls showing no selections which could be confusing.

I appreciate that having the warning there could be considered a good thing depending on your perspective. It doesn't appear to be a deliberate change in behaviour though so is worth making a conscious decision on.

Trunk	This PR

If we were to proceed with this PR's approach (complete with logic tweaks required), there is a little polishing of the code we can do. I've added inline comments for those.

Lastly, it still feels like we might be sidestepping a root issue somewhere which explains why the detected colors on a button aren't correct and why the existing color contrast checking wasn't working. Solving that could still make the changes proposed here moot.

Unfortunately, I'm a little short on time to do that digging myself. I'm happy to keep reviewing and discussing PRs though if it helps.

[tomdevisser]

In cases where there might not be enough information to make a reliable call as to whether or not there's a contrast issue, I think I'd lean toward not showing the contrast checker. So, in principle, only showing the contrast checker when there's enough information to reliably know that there is an issue.

@andrewserong would this mean returning early when the background and either the link or text color are undefined?

[tomdevisser]

Previously, if there were no selected colors, no contrast checking was performed as the detected colors were all reset. On this PR branch, if theme.json or global styles define a poor color combination, the user gets the contrast warning about "this color combination" despite the two controls showing no selections which could be confusing.

@aaronrobertshaw This is a deliberate change and actually a sign it works in my opinion. It's not checking where the color is defined/detected, just what that color is. If that has a bad contrast value like white on white, you should wanna change it. Otherwise the contrast notice would not be reliable and thus useless.

If we were to proceed with this PR's approach (complete with logic tweaks required), there is a little polishing of the code we can do. I've added inline comments for those.

Thank you for these, corrected those! :)

Lastly, it still feels like we might be sidestepping a root issue somewhere which explains why the detected colors on a button aren't correct and why the existing color contrast checking wasn't working. Solving that could still make the changes proposed here moot.

It wasn't working only in this component, because the logic in this component wasn't sound. I've been up and down the component tree a lot of times during this and am pretty sure this was an issue with this component and couldn't find any root issues on this.

[tomdevisser]

I'm not sure if the logic is quite right, yet. I noticed with a theme that sets a default background colour for button elements that the contrast checker is firing when I select only a single text colour, but no background colour (as the background colour is set in theme.json)

@andrewserong Hm, this is weird. Seems like when a color is specified in the theme.json, it's not being set as a computed style on the current ref, causing the background color loop to kick in. Which ends on the background of the site being white. This is in my humble opinion a bug for another issue on how theme.json values are being saved, don't you agree? Otherwise we're losing separation of concerns in these tickets, while the contrast checker works fine there. It's just missing a value that another piece of the software is failing to provide.

[aaronrobertshaw]

This is a deliberate change and actually a sign it works in my opinion. It's not checking where the color is defined/detected, just what that color is. If that has a bad contrast value like white on white, you should wanna change it. Otherwise the contrast notice would not be reliable and thus useless.

My understanding is that there was a prior decision that we should only provide that contrast feedback once the user has made a color selection. Prior to that, the color contrast and combination would be defined by the theme and there is a different level of trust involved in there.

If the text is white on a white background, or really bad in terms of contrast, you are likely to be changing the colors regardless of whether there is a warning or not.

To summarise this point, I believe the contrast warning was to provide feedback on user-selected combinations which do in part rely on the computed styles.

I'm not ruling out the proposed behaviour, just that it should be a considered change. It's probably one where we should get some wider opinions before committing to an approach.

@jasmussen or @jameskoster do you have any thoughts on how best contrast checking should work here?

It wasn't working only in this component, because the logic in this component wasn't sound.

Your last comment suggests there is still an issue in correctly detecting the background color when it has been applied via a theme.json style. I think having that resolved would ease evaluating any changes to this component as we aren't then making any assumptions about where problems or bugs might lie.

This is in my humble opinion a bug for another issue on how theme.json values are being saved, don't you agree?

I'm not 100% sure it is. Being able to detect the right colors to check for contrast seems directly related to the issue at hand.

Theme.json and Global Styles are converted into managed stylesheets. I'm not sure there is much scope to change how these add loaded in the editor if that is what you were suggesting.

It's just missing a value that another piece of the software is failing to provide.

An alternative perspective could be that this component isn't retrieving the value provided.

If it is only an issue with theme.json or global styles, I believe we've recently gained access to the merged global styles within the block editor store. So we could access those values directly. I'm still not sure how a theme-provided stylesheet (non-theme.json) applying button block colours might impact background color detection if it isn't currently working.

Thanks for your patience on this one and the quick turnaround on addressing the other tweaks to this PR 👍

[tomdevisser]

Prior to that, the color contrast and combination would be defined by the theme and there is a different level of trust involved in there.

@aaronrobertshaw I think this trust would be misplaced, as there are still lots of designers out there that just want to create the most beautiful themes, regardless of if they're accessible or not. And they are in their right to do so of course. But as long as we don't have an "accessibility tested" badge or something like that in the theme repo, we can't trust every theme follows the contrast rules.

I'm not ruling out the proposed behaviour, just that it should be a considered change. It's probably one where we should get some wider opinions before committing to an approach.

I understand, and agree! :)

I'm not 100% sure it is. Being able to detect the right colors to check for contrast seems directly related to the issue at hand.

True, but we would also be working in a very different part of the project, where others might wanna review, etc. In my short experience, issues that become too big will hang around forever. We could put this one on hold and make a separate issue and finish that one first, but I don't think it's a good idea to make this one too big. This one is about a missing notice, and already broadened to some degree. Adding "how theme.json generates a stylesheet" is a different subject.

Besides that, I've never worked in that area of the project before, so I'm not sure if I'm the best one to make a PR on that. Although I'm willing to give it a shot, I think we're gonna need help there as generating files from a JSON is a completely different thing than tweaking React components.

Thanks for your patience on this one and the quick turnaround on addressing the other tweaks to this PR.

No problem, thank you for the thorough testing and patience. I think accessibility is a really important aspect where we can and need to make improvements as soon as we can.

[jasmussen]

My understanding is that there was a prior decision that we should only provide that contrast feedback once the user has made a color selection. Prior to that, the color contrast and combination would be defined by the theme and there is a different level of trust involved in there.

First off, providing excellent contrast checking is a pretty amazing tool we have available in the block editor. It would be nice to apply this as deeply and as smartly across as many color aspects as possible. So a solid behavior for this is definitely something we want.

There is a bit of nuance, though, and it has less to do with where the colors come from, and is more a matter of design. Consider the following:

I can imagine a new user going: What, why am I getting a warning? I haven't done anything!

Because they haven't: no colors were chosen, it's honestly not their fault, it's that of the theme. So should we not provide a contrast warning? Yes, we probably should, but we need to ensure that it's actionable for the user. That requires some design thought, first and foremost. That is:

• It is an opportunity to point out to the user the theme itself is responsible for the error.
• It should provide some instructions and/or a link to fix it at the source.
• It should not look like a warning, at least in the local context. Perhaps it could be a notice, dismissible or not.
• It should look like a warning in the responsible context, i.e. global styles.

Mainly what we want to avoid is that users try to fix the contrast issue in the local state, having to update every page or post across their entire site. Because it's the fault of the theme, we need to push them to fix the issue at the source. If we just show an orange notice in the color panel, one which disappears when they fix it locally, we implicitly approve of the "local" fix, when the more important fix is a global one.

[jasmussen]

CC: @WordPress/gutenberg-design ☝️

[aaronrobertshaw]

Wise words as always @jasmussen, thanks for taking the time to weigh in 🙇

I think this trust would be misplaced,

@tomdevisser My use of the term here was also misplaced 🙂

I was hoping to put actions taken by the theme author into a different basket for consideration than those by the user without getting the discussion any further into the weeds.

Joen articulates things much more eloquently in his comment so we can focus on that.

This one is about a missing notice, and already broadened to some degree. Adding "how theme.json generates a stylesheet" is a different subject.

It still is about the missing notice if its generation depends on the correct detection of color on an element.

I might be missing something but I don't see how detecting the computed background color is going to require rewriting theme.json generated stylesheets.

In my short experience, issues that become too big will hang around forever.

Unfortunately, every now and again some issues do evolve in this fashion.

In terms of this PR's scope though, I think it's under control for now. What comes out of the design review will be detailed in a separate issue and can be broken down there as needed into new PRs.

To move this PR forward though, my suggestion would be to restore the original behaviour where the contrast checking only occurs when the user has selected a color. It will avoid the potential confusion, and follow-on issues, until we have a more holistic solution.

I think accessibility is a really important aspect where we can and need to make improvements as soon as we can.

You'll find no arguments here. We also need to make sure we don't create further issues rushing a solution. It can be a frustrating balancing act at times. ⚖️

[tomdevisser]

To move this PR forward though, my suggestion would be to restore the original behaviour where the contrast checking only occurs when the user has selected a color. It will avoid the #48313 (comment), until we have a more holistic solution.

@aaronrobertshaw I see what you mean, pushed a new commit. I think this does what you're suggesting:

contrast.mov

[tomdevisser]

@aaronrobertshaw Can I do anything to help moving this PR forward? :)

[aaronrobertshaw]

Thank you for the iterations on this @tomdevisser 👍

I'm still a little reluctant to lose taking the ultimate computed styles into account when checking contrast. The current approach in this PR ignores what the end results are if the user selects colors that get overridden by the theme.

I did a quick bit of digging to answer my and Andrew's earlier question about why the computed styles for the Button weren't matching the selector colors or being determined "correctly".

The answer is that for the Button block, the color support and global styles are applied to the inner element via the __experimentalSelector supports property. In the editor, this inner element is the rich text field. The color panel, uses the block ref, which would be the block's main outer wrapper, to determine the computed styles.

Exploring this further, the root issue should impact any blocks that skip serialization of color supports and apply them to inner elements. These include the Calendar, Search, and Table blocks. The Separator block also skips serialization but is a design element only that disabled contrast checking.

I wonder if there is a means through which we could actually declare the element from which the contrast checking should occur. @andrewserong do you have any ideas on that front?

Perhaps an additional property on the color support object? For the Button block, something that specifies the inner element in question e.g. .wp-block-button__link. In the ColorPanel, we'd then get the block supports, and if that property is present attempt to determine the computed colors via the element that selector would represent. If it isn't defined we'd use the main block element as per the current use of ref.current now.

This might mean we can still cover weird edge cases where theme styles override use selections while still fixing the root issue this PR and #48308 relate to.

[andrewserong]

I wonder if there is a means through which we could actually declare the element from which the contrast checking should occur. @andrewserong do you have any ideas on that front?

Nice sleuthing @aaronrobertshaw. This is just an idea off the top of my head, but since there's already an __experimentalSelector, I wonder if we could re-use this for a querySelector call on the ref, but strip the value of the block wrapper and/or the current element, so that it works? I'm not sure how reliable that might be, but just thinking about whether or not it'd be possible to infer a reliable enough selector from __experimentalSelector. For example, we'd strip the .wp-block-button prefix from the Button block's __experimentalSelector, and for Calendar since the whole selector matches the current ref, we'd use the ref as-is... I'm not sure how easy it'd be to write some stable logic for that, though. For example, for Table, which is .wp-block-table > table we'd need to know to only strip things left of the child combinator selector, or > table would be stripped 🤔

In the ColorPanel, we'd then get the block supports, and if that property is present attempt to determine the computed colors via the element that selector would represent. If it isn't defined we'd use the main block element as per the current use of ref.current now.

Once we've got the correct selector to use, though, this fallback strategy sounds like a solid plan to explore to me 👍

[aaronrobertshaw]

This is just an idea off the top of my head, but since there's already an __experimentalSelector, I wonder if we could re-use this for a querySelector call on the ref, but strip the value of the block wrapper and/or the current element, so that it works?

The __experimentalSelector could potentially be a compound selector (see Image block). So that could cause some wrinkles.

I'd also be inclined not to rely on an experimental property. We do have a new selectors API on the way which would allow us to retrieve the selector that the color Global Styles would be generated under in a stable manner.

Once we've got the correct selector to use, though, this fallback strategy sounds like a solid plan to explore to me 👍

Yeah, getting the contrast checker to actually check the right element and the end result colors, sounds more and more like what we should be aiming for.

With 6.2 due for release soon, the stabilized and extended selectors API won't be far behind. So I'd probably vote to pause this PR until we explore the alternative approach.

[andrewserong]

With 6.2 due for release soon, the stabilized and extended selectors API won't be far behind. So I'd probably vote to pause this PR until we explore the alternative approach.

I agree, I think pausing for the moment, and then using the stable selectors API sounds wise so that we can avoid introducing something that feels brittle. That has my vote, too 👍

[aaronrobertshaw]

Just quickly flagging that the Selectors API has landed now and this could be revisited.

See the Selectors API PR and dev note for more info.