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

Cover Block regression: Text which was previously white by default is now black and not visible (WP 5.9 beta1) #37031

Open
briceduclos opened this issue Dec 1, 2021 · 17 comments
Labels
[Block] Cover Affects the Cover Block - used to display content laid over a background image [Type] Regression Related to a regression in the latest release

Comments

@briceduclos
Copy link

Description

For certain images, the useCoverIsDark hook returns a false value while the Cover is dark.

For the example below, there is no reason to change the text to black. It lowers the contrast ratio, and the content isn’t visible anymore.

Step-by-step reproduction instructions

  1. Inside the block editor, paste the following code:
<!-- wp:cover {"url":"https://s.w.org/images/core/5.5/don-quixote-06.jpg"} -->
<div class="wp-block-cover has-background-dim"><img class="wp-block-cover__image-background" alt="" src="https://s.w.org/images/core/5.5/don-quixote-06.jpg" data-object-fit="cover"/><div class="wp-block-cover__inner-container"><!-- wp:heading {"textAlign":"center"} -->
<h2 class="has-text-align-center">Heading</h2>
<!-- /wp:heading --></div></div>
<!-- /wp:cover -->
  1. Observe that by default the text is black.
  2. As a result, the text color of a Cover block which was white in WP5.8 is now black in WP5.9.

Screenshots, screen recording, code snippet

WordPress 5.8.2 WordPress 5.9 beta 1
58 59

Environment info

  • WordPress 5.9 Beta 1, Twenty-Twenty theme

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

@glendaviesnz glendaviesnz added [Block] Cover Affects the Cover Block - used to display content laid over a background image [Type] Bug An existing feature does not function as intended [Type] Regression Related to a regression in the latest release and removed [Type] Bug An existing feature does not function as intended labels Dec 2, 2021
@glendaviesnz
Copy link
Contributor

glendaviesnz commented Dec 2, 2021

Looks like the method that is calculating whether the background is dark or not needs some adjustment - currently it doesn't see that particular image as dark until the overlay is at opacity 60

cover-text

Update - turned that this is because when dimRatio > 50 the background color is used to calculate darkness instead of the image

@glendaviesnz
Copy link
Contributor

glendaviesnz commented Dec 2, 2021

It looks like there are a couple of issues here:

  1. The underlying fast-average-color library is calculating the average color of the Don Quixote image as being light so applying a dark text color, but the portion of the image that the text is sitting over is quite dark in relation to the rest of the image. If an actual dark image is used it works as expected. I am not sure how we would get around this as it would be complex to try and work out exactly which part of the image the text was going to sit over in order to get a more accurate default setting.

Screen Shot 2021-12-02 at 4 45 30 PM

  1. Using a remote url for the image causes fast-average-color to fail with a DOMException: Failed to execute 'getImageData' on 'CanvasRenderingContext2D': The canvas has been tainted by cross-origin data. error which causes it to fall back to #ffffff as the average color, which again is light, so text set to dark.

@glendaviesnz
Copy link
Contributor

Also, as background, the reason for this change is outlined here

@hellofromtonya
Copy link
Contributor

Was reported on Trac too https://core.trac.wordpress.org/ticket/54791.

@cbirdsong
Copy link

I've just encountered this issue on a production site when upgrading it to Wordpress 5.9.

@pattyok
Copy link

pattyok commented Feb 15, 2022

Most of the text on the home page of a recently launched site when white (on white) because of this change! It doesn't appear that it is doing any color detection it is purely based on the opacity level selected.

I have a light colored image, using a white overlay. And if the opacity of the overlay is under 50% all of the text goes white.

image

image

I will add that in my theme I was taking advantage of the has-white-background-color class that was attached to the block. In my themes, I have css rules that set the default text color based on the class added to the block. That rule goes out the window with this change as well.

There's some good discussion on how changes like this impact theme developers. In this issue: #38694 (comment)

@mrwweb
Copy link

mrwweb commented Feb 15, 2022

As I mentioned in my comment on the Core block refactor dev not: For any sites that don't support the full color controls, the recommended workaround is not feasible. On a site I maintain, we don't allow editors to apply colors through the standard color tools, so this isn't a hypothetical. For now, my solution was to override the new CSS rules with higher specificity selectors that revert the behavior.

Given how many people are finding that the color calculations for light/dark are not working well with their specific colors and images, a filter or theme.json setting to disable this behavior would be really useful.

@glendaviesnz
Copy link
Contributor

@scruffian, do you have any thoughts on ways to mitigate the problems noted above due the change in Cover block background settings?

@glendaviesnz
Copy link
Contributor

Most of the text on the home page of a recently launched site when white (on white) because of this change!

@pattyok thanks for reporting this. I wasn't able to replicate this on 5.9 with TwentyTwentyTwo theme:
cover-text-color

It would be good to try and narrow down what is different in your theme that is causing the issue for you. Is it a publicly available theme that we could do some testing on? If not if you have time to see what the difference might be between your theme and TwentyTwentyTwo that would be really helpful to try and track down what is causing the issue for you, and to help with coming up with a fix.

@scruffian
Copy link
Contributor

This is unfortunate, but I still think it is preferable to the previous situation where the text would always be white, even if the block/image was white making the text invisible.

One thing to bear in mind is that user still have control over the color of the text in the cover block - the idea of setting the color to black/white for them is to try to provide some clever defaults, but ultimately we want the user to have control over that color, which they can do using the block controls. Would the best course of action here might be to encourage those users who are impacted to manually set the text color when contrast doesn't work for their image?

@glendaviesnz
Copy link
Contributor

glendaviesnz commented Feb 22, 2022

Would the best course of action here might be to encourage those users who are impacted to manually set the text color when contrast doesn't work for their image?

@scruffian, I think the main issue with this is the case that @mrwweb notes where a theme prevents users from changing the colors.

@scruffian
Copy link
Contributor

Ah yes I see. I'm not sure if a filter to disable this behaviour is a solution either though, as problem would still exist when the image was light...

@mrwweb
Copy link

mrwweb commented Feb 25, 2022

I'm not sure if a filter to disable this behaviour is a solution either though, as problem would still exist when the image was light...

It's mostly the case that block settings can be opted out of and it feels like this should be the case here as well. In our case, black is the only overlay color allowed and so there is no circumstance where white text is desirable. I have no doubt I'm not the only person in this situation.

@pinksharpii
Copy link

We are also experiencing this on at least 2 sites we have developed. We define the color scheme for the site, only allowing brand colors to be used so it's not a question of color contrast. Selecting black or dark blue overlay should always output white text regardless of opacity.
Previously we handled this in our theme by stating .has-black-background-color { color: #fff; } but 5.9.1 removing the background color from the parent and adding in a forced is-white or is-dark-theme really makes theme development difficult to do when it comes to cover blocks.
Having a filter or an option to not add that class, and have it be defined by the theme would be great.
Putting the background color class back in the parent wrapper (so we as theme devs can easily target the text in CSS) would be ideal but I understand that's probably not the direction this will go.

@patrickwc
Copy link

patrickwc commented Oct 6, 2022

We are experiencing issues with this at our agency. We not add a filter which makes it possible for theme authors to change the dimRatio > 50 value mentioned above by @glendaviesnz ? @scruffian can I ask why was 50 decided upon? I feel like this update only took into account solid background colours not background images with overlay settings applied to them. A dark overlay with 50 opacity is still pretty dark, so much so that a "light" image with 50% dark opacity now has illegible text after upgrading. Having to tell users to manually change the text colour on all posts where cover blocks are used after updating WP is not an option for us - we have clients who have thousands of pages.

I have managed to get around this issue on our themes by overriding the CSS:

.wp-block-cover.is-light:has(> .has-black-background-color),
.wp-block-cover.is-light .has-black-background-color ~ .wp-block-cover__inner-container {
    color: #fff;
}

I cannot think of a scenario where if .has-black-background-color is applied, you would want WordPress to force your text to be dark.

@jonathan-dejong
Copy link

This is a wider issue with visually breaking changes in many core blocks in combination with being static blocks.. but please, for this specific issue, at least play ball with us in finding a code solution that does not require us telling clients to go update all their content.

@scruffian
Copy link
Contributor

@scruffian can I ask why was 50 decided upon

This felt like the right compromise between two difficult positions. If we tweak it either way, then the other case will be even more broken.

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 [Type] Regression Related to a regression in the latest release
Projects
None yet
Development

No branches or pull requests

10 participants