Skip to content
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

ContrastChecker: Cover block overlay and text color incorrect #50531

Closed
richtabor opened this issue May 10, 2023 · 11 comments · Fixed by #53080
Closed

ContrastChecker: Cover block overlay and text color incorrect #50531

richtabor opened this issue May 10, 2023 · 11 comments · Fixed by #53080
Assignees
Labels
[Block] Cover Affects the Cover Block - used to display content laid over a background image [Feature] Colors Color management Needs Accessibility Feedback Need input from accessibility [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended

Comments

@richtabor
Copy link
Member

I've noticed that the ContrastChecker does not work well with the Cover block's Overlay and Text colors. Overall its very inconsistent, but I have noticed a few things in particular:

A. The notice shouldn't be fired in the default state (no text and overlay colors):

CleanShot 2023-05-10 at 16 35 29

B. The ContrastChecker tends to five a false positive for accessible colors. i.e showing the notice when the color does have appropriate contrast. The checker should use to the Overlay fallback color (perhaps that should be set in the attribute, instead of CSS directly):

CleanShot.2023-05-10.at.16.37.03.mp4

C. Changing the Overlay color never fires the ContrastChecker.

@richtabor richtabor added [Type] Bug An existing feature does not function as intended [Feature] Colors Color management labels May 10, 2023
@ramonjd
Copy link
Member

ramonjd commented May 12, 2023

I'm having flashbacks!

This reminds me of a conversation we had around the unreliability of the contrast checker in the absence of solid colors, e.g., alpha

It probably doesn't make sense in this scenario, but for reference we can turn off the contrast checker for blocks

@richtabor
Copy link
Member Author

we can #43357 for blocks

Yea, I'd like to avoid that here.

This reminds me of a #37731 (comment) we had around the unreliability of the contrast checker in the absence of solid colors, e.g., alpha

I don't think this is related to alpha, as the overlay is a div with the color and opacity applied. But it may be related to the Overlay color not being a standard color applied (like background and text is for all other blocks).

@TaisaDesigner
Copy link

I have also noticed that on the new beta 6.3 because there were changes on backgrounds and came to check if it was already reported... It stays the same. Even when I use black/White, no images (I understand with images can be more complicated).

image

I don't know othe people, but I do use backgrounds a lot...

@richtabor richtabor added the [Block] Cover Affects the Cover Block - used to display content laid over a background image label Jul 20, 2023
@kathrynwp
Copy link

@annezazu @ndiego As this is a very popular block – and a pretty in-your-face incorrect warning, do you think there's any chance a fix might make it into 6.3? TY!

@ndiego
Copy link
Member

ndiego commented Jul 25, 2023

Thanks for the ping @kathrynwp.

@richtabor @annezazu in the interest of 6.3, I think we should disable the contrast checker with "enableContrastChecker": false,. I believe the built-in contrast checker is checking for text color against a background color, which the Cover block does not have. Instead, it has a custom color control for Overlay, which does not seem to work properly with the contrast checker.

Setting "enableContrastChecker": false, will remove the erroneous alert and fix the bug in the short-term. We can then look into making the contrast checker more extensible in 6.4. What do you think?

@annezazu annezazu added the Needs Accessibility Feedback Need input from accessibility label Jul 25, 2023
@annezazu
Copy link
Contributor

annezazu commented Jul 25, 2023

I fear making calls here without involving accessibility. I've added the label for now to get their thoughts. Otherwise, what you shared seems sound to me.

@annezazu
Copy link
Contributor

@alexstine @joedolson @afercia in case you have time.

@richtabor
Copy link
Member Author

I fear making calls here without involving accessibility.

As-is, it’s giving a false positive for accessible color contrast - which is worse than having the checker at all. 😬

I think disabling it is the best temporary solution.

@joedolson
Copy link
Contributor

This pretty clearly doesn't work right now. I'd agree that a warning that isn't trustable is worse than no warning at all.

However, I think that getting this improved globally needs to be a high priority in 6.4.

Will disabling the contrast checker only impact the cover block, but leave the innerblock supported? That contrast check still works when a background color is set on the innerblock, so I wouldn't want to lose that.

@github-actions github-actions bot added the [Status] In Progress Tracking issues with work in progress label Jul 27, 2023
@ramonjd
Copy link
Member

ramonjd commented Jul 27, 2023

I think we should disable the contrast checker with "enableContrastChecker": false,
This pretty clearly doesn't work right now. I'd agree that a warning that isn't trustable is worse than no warning at all.

See this discussion from year ago about how the contrast checker doesn't play well with the Cover block: #37731 (comment)

It contributed to the motivation to create the enableContrastChecker flag

Here's a PR to disable it for the Cover block:

@hanneslsm
Copy link

Only for future reference if the contrast checker is discussed again:
With a sticky group with a background with opacity you could run into situations like this:
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Cover Affects the Cover Block - used to display content laid over a background image [Feature] Colors Color management Needs Accessibility Feedback Need input from accessibility [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended
Projects
Development

Successfully merging a pull request may close this issue.

8 participants