Stop theme defaults from overriding the Featured Item blocks text color

[danielwrobert]

Summary

By omitting the Featured Items blocks from the Storefront Customizer ruleset that sets the default text color, we allow the block to use it's set default of white text over the background overlay, improving color contrast. This also allows custom and global styles to be applied correctly, where applicable.

Fixes #2036

Screenshots

Before	After

How to test the changes in this Pull Request:

1. Activate the Storefront theme.
2. Add the Featured Product block to a page.
3. Select a product.
4. Confirm that the default color of the text inside is white (as seen in after screenshot above).
5. Change the color using the picker to a custom one (not included in the default palette).
6. The color of the text should change.
7. Repeat steps 2–6 with the Featured Category block.

Changelog

Fix – Featured Item Blocks: omit customizer styles to prevent Storefront from overriding block defaults and preventing custom colors from being applied correctly.

[danielwrobert]

Note: The Featured Items blocks are omitted from the customizer styles here via a :not() selector. This is OK - I've confirmed that these blocks are the only two Woo blocks that have the dark background overlay. While, it would be ideal if we could apply a rule that is a bit more broad for future flexibility, this seems like a reasonable "quickfix" for the time being.

The way that the blocks are structured, the has-dim-background class is applied to a child of the main block container. Without re-structuring the block, this approach could be approved in the future when the :has() selector becomes more widely adapted by browsers.

[sunyatasattva]

Thank you for adding loads of docs to the filters. I understand that this is a quick patch, but I'm not entirely convinced by this approach. It kinda heavily breaks the separation of concerns. Also, what's the responsibility hierarchy here? Should themes make sure they are playing well with plugins, or should plugins be aware of themes? It seems that we create a weird precedent by excluding certain classes and potentially make it very hard to scale.

Perhaps having a more generic class to be excluded from this style rule? Maybe less specificity?

I'm not sure what's the best solution here, but I also think I am not 100% clear on the hierarchy of all the pieces coming together.

[sunyatasattva]

This also works I believe :)

Suggested change

	.editor-styles-wrapper .block-editor-block-list__block:not(.wp-block-woocommerce-featured-product):not(.wp-block-woocommerce-featured-category) {
	.editor-styles-wrapper .block-editor-block-list__block:not(.wp-block-woocommerce-featured-product, .wp-block-woocommerce-featured-category) {

[danielwrobert]

@sunyatasattva thanks for your thoughts on this, you raise some very good points!

While the .block-editor-block-list__block selector is specific to Gutenberg, targeting the Featured Item block classes directly is even more specific and I agree that it does break the separation of concerns.

Perhaps having a more generic class to be excluded from this style rule? Maybe less specificity?

Yes, I really like this thought. Ideally, we’d target the has-background-dim class, which is more general and applies to both blocks - and potentially any other block that would apply a dark background overlay in the future.

The issue when I looked at this route was that the has-dim-background class is output in a child element of the block’s main wrapper from the editor (which was to my point when referencing the experimental :has() selector. However, the has-dim-background class is rendered on the main container on the frontend.

Perhaps a better way forward is to still target this has-dim-background class from Storefront but adjust where that class is rendered in the editor on the Featured Item blocks, if possible (which would also be a little more consistent with the output on the frontend). What do you think?

Also, what’s the responsibility hierarchy here? Should themes make sure they are playing well with plugins, or should plugins be aware of themes?

I know I owe a post on this to kick off a broader discussion - I’ll do this today. IMO, it makes sense here for the theme to ensure it’s working with plugins. For example, the theme should not output dark text in scenarios where it will lack contrast (i.e., over a dark background. When we tried to prevent this from the plugin side, it caused an issue with Global Styles, which seems more confusing for the user. In this particular instance, even if we did nothing and allowed for the theme to output poorly contrasted text by default for these blocks, it is adjustable via the editor controls in an intuitive way.

[sunyatasattva]

Excellent thoughts, @danielwrobert. Thanks for taking the time to write them down and indulge me on this. I know it's just as easy to approve this small change and swipe it under the rug, but I feel it is important that we share thoughts on such things.

Perhaps a better way forward is to still target this has-dim-background class from Storefront but adjust where that class is rendered in the editor on the Featured Item blocks, if possible (which would also be a little more consistent with the output on the frontend). What do you think?

I also thought that class would make more sense. In this way other plugins and blocks can still use the has-background-dim and know they are safe from those contrast issues. I remember there being some issues on where to output the class in the DOM as the editor inserts a bunch of wrappers, but maybe it is possible to change it without a huge difficulty.

Would it just not work leaving the class where it is currently? Would CSS rules not apply then to what we want to target?

I know I owe a post on this to kick off a broader discussion - I’ll do this today. IMO, it makes sense here for the theme to ensure it’s working with plugins.

I would tend to agree with this. It's really hard, though, right? Because the point of a theme is that it would give a consistent style and—well…—“theme” to the website. So I understand how conflicts like this can arise.

[danielwrobert]

Would it just not work leaving the class where it is currently? Would CSS rules not apply then to what we want to target?

@sunyatasattva not without the use of some “cutting-edge” (Level 4) CSS selectors, from what I can tell. The theme applies the color rule to the main block container in the editor. For the theme, we could target the child div with the has-background-dim class, which would override the theme color. However, we then have the problem where this also overrides the color application for Global Styles - which puts us right back to square one. 😭

That's why I noted the :has() selector above. With that, we could write something like:

.editor-styles-wrapper .block-editor-block-list__block:not(:has(div.has-background-dim)) {
	color: ' . $storefront_theme_mods['text_color'] . ';
}

Unfortunately, this isn't currently as supported as we'd need - although it is getting there!

To remedy any support concerns, we could include a sensible fallback for the time being by either letting unsupported browsers override the text (essentially ignoring the problem in unsupported browsers until they update) or temporarily add the specific selector for the Featured Items that I set in this PR originally:

.editor-styles-wrapper .block-editor-block-list__block:not(:has(div.has-background-dim)) {
	color: ' . $storefront_theme_mods['text_color'] . ';
}
@supports not (selector(:has(*))) {
	.editor-styles-wrapper .block-editor-block-list__block {
		color: ' . $storefront_theme_mods['text_color'] . ';
	}
}

OR

.editor-styles-wrapper .block-editor-block-list__block:not(:has(div.has-background-dim)) {
	color: ' . $storefront_theme_mods['text_color'] . ';
}
@supports not (selector(:has(*))) {
	.editor-styles-wrapper .block-editor-block-list__block:not(.wp-block-woocommerce-featured-product, .wp-block-woocommerce-featured-category) {
		color: ' . $storefront_theme_mods['text_color'] . ';
	}
}

I remember there being some issues on where to output the class in the DOM as the editor inserts a bunch of wrappers, but maybe it is possible to change it without a huge difficulty.

Yeah, this seems like it could be a good route if the above options with :has() and @supports are unfavorable.

[sunyatasattva]

Alright, in that case I think your workaround with the fallback sounds best to me. Eventually, I think we probably will fade out Storefront in favor of a default blocktheme I assume, right?

I'd say for now it doesn't make much sense to block you any further. The fallback option works for me.

[danielwrobert]

Eventually, I think we probably will fade out Storefront in favor of a default blocktheme I assume, right?

I assume so. And as themes continue to head in that direction, I imagine that will make the aforementioned responsibility hierarchy questions a bit more clear. It seems like we're still in a bit of an in-between state with everything.

FWIW, I also think we'll see improved support for :has() fairly quickly. Maybe it was timing but it already improved in the couple of days since I looked at the docs.

I'd say for now it doesn't make much sense to block you any further. The fallback option works for me.

Thank you! I haven't super concerned about being blocked - I enjoy thinking all of this through and am more interested in making a good decision. I appreciate the conversation and your thoughts! 🙂

[danielwrobert]

@sunyatasattva I've updated this PR in 4fab27f to use the approach discussed above.

Also note that the docblock updates were handled in #2038. I originally branched from that update so I could commit my changes here w/o the linter screaming at me and preventing my commits.

After merging that PR and rebasing this one with trunk we only have the actual CSS changes here, as intended. 🙂

[sunyatasattva]

Thanks @danielwrobert for putting up with me and for the clean PR!

I think we can go ahead with this.