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

Use .has-background for Cover block consistently with all other blocks. Consider adding background type class to all blocks. #21439

Open
mrwweb opened this issue Apr 6, 2020 · 18 comments
Labels
CSS Styling Related to editor and front end styles, CSS-specific issues. [Feature] Themes Questions or issues with incorporating or styling blocks in a theme. [Type] Enhancement A suggestion for improvement.

Comments

@mrwweb
Copy link

mrwweb commented Apr 6, 2020

I'm theming a site right now that uses both background colors and background gradients.

I'm having to write double the selectors to target any cover block (or presumably group block in the future) that has either a background color or background gradient because they don't share a selector. I have to style both .has-background and .has-background-gradient.

Solution: Always apply .has-background to a block that has a background color or gradient (or image or video #16660)

@talldan talldan added the [Type] Enhancement A suggestion for improvement. label Apr 7, 2020
@talldan
Copy link
Contributor

talldan commented Apr 7, 2020

The reason for this would be that originally has-background was implemented before gradients existed to indicate a background color.

It should possibly be renamed has-background-color to be clearer about what it's for, but that may cause some breakage now.

@mrwweb
Copy link
Author

mrwweb commented Apr 7, 2020

The reason for this would be that originally has-background was implemented before gradients existed to indicate a background color.

I'd question whether that latter assumption is really how folks are understanding this. It certainly is not how I imagined that class worked, and I doubt treating it as a universal "has any kind of background" class would break things.

I'll reiterate my concerns about CSS madness without a universal .has-background class. As an example, without that universal class, we will have to write the following for a standard need like adding padding to anything with a background:

.wp-block-group.has-background,
.wp-block-group.has-background-gradient,
.wp-block-group.has-background-video,
.wp-block-group.has-background-image,
.wp-block-group.has-background-pattern {
    padding: 1.5rem;
}

Yes, I could write .wp-block-group[class*="has-background"], but that introduces oddities around specificity, could theoretically match things that aren't intended (.mochas-background), and is theoretically less performant.

Instead, what if we added a .has-background-color so that you could instead write a universal rule with exceptions when necessary:

.wp-block-group.has-background {
    padding: 1.5rem;
}
.wp-block-group.has-background-color {
    padding: 2.5rem;
}

So to put forth a concrete proposal:

  • In the next release, introduce .has-background to ALL types of backgrounds, starting with gradients on buttons and cover blocks.
  • Add a .has-background-color class to any element with a solid background color.
  • On all future types of backgrounds, use .has-background along with a second class like .has-background-image to match the existing .has-background-gradient.

@mrwweb mrwweb changed the title Apply .has-background to any block with a gradient background Use .has-background to indicate any type of background. Introduce second class for type of background. Apr 7, 2020
@talldan
Copy link
Contributor

talldan commented Apr 8, 2020

I doubt treating it as a universal "has any kind of background" class would break things.

There's some information on backwards compatibility here:
https://github.com/WordPress/gutenberg/tree/master/docs/designers-developers/developers/backward-compatibility#how-to-preserve-backward-compatibility-for-a-block

In particular it says:

The styling of the existing blocks should be guaranteed.

If I'm reading it right, changing the way has-background is used would be considered a breaking change. But I might be interpreting things wrong, so happy to hear other perspectives.

I do think your proposal makes logical sense, but there has to be some care taken with those changes.

@mrwweb
Copy link
Author

mrwweb commented Apr 8, 2020

The styling of the existing blocks should be guaranteed.

It's possible that I'm missing something, but I can't find an example in the core block styles where adding .has-background to cover or button elements would cause a default styling change.

Further, I still think this is more a bug breaking expected behavior rather than an enhancement.

It feels like this needs more eyes. But if others agree, it does seem important to get this change in ASAP (5.5 or 5.4.X).

@kjellr
Copy link
Contributor

kjellr commented Apr 15, 2020

A few bullets to clarify the proposal here (courtesy of @carolinan):

  • In the next release, introduce .has-background to ALL types of backgrounds, starting with gradients on buttons and cover blocks.
  • Add a .has-background-color class to any element with a solid background color.
  • On all future types of backgrounds, use .has-background along with a second class like .has-background-image to match the existing .has-background-gradient.

From my perspective, this seems like a decent path forward. In most cases, themes use the has-background class just to cancel out any styles that might interfere if there were any sort of custom background. Personally, that's what I'd envisioned it being used for — I hadn't even considered that that class wouldn't be present if there was a gradient or video background for instance.

My only hesitation to following this approach would be if it would cause widespread breakage, but I think on the contrary, it's more likely to suddenly fix themes that had been thinking about the has-background class this way.

@mrwweb
Copy link
Author

mrwweb commented Apr 15, 2020

100% in favor of @kjellr's proposal. Another addition: add .has-background-image to Cover blocks when there is an image

@mrwweb
Copy link
Author

mrwweb commented May 8, 2020

@kjellr @talldan How can I help build some momentum to get this into 5.5? It feels like the clock is ticking to make this change before it's too late, and there seems to be a pretty clear consensus from themers that this is the way to go.

@kjellr kjellr added the [Feature] Themes Questions or issues with incorporating or styling blocks in a theme. label May 8, 2020
@talldan
Copy link
Contributor

talldan commented May 14, 2020

Is there a solution that wouldn't cause breakage across themes? Or alternatively one that allows a more gradual transition away from the problematic class name?

@talldan talldan added the CSS Styling Related to editor and front end styles, CSS-specific issues. label May 14, 2020
@mrwweb
Copy link
Author

mrwweb commented May 14, 2020

I really think this needs to almost be reframed as a bug, given that I just noticed that the Cover block doesn't even use the .has-background class for when it has a background color. There really needs to be a universal class for "this block has a non-transparent background" and I can't think of any other classname that would be more appropriate.

While it's plausible that adding the .has-background class to the Cover block would break some things, I suspect it would be very limited. Having styled that class a number of times, any time I'm styling that, it's in a multi selector ruleset that is unwieldy because .has-background isn't used consistently (see my OP). And as both @kjellr and I have mentioned, given how easily misunderstood this classname is, it is quite possible it will fix some themes too. (Hence, treating it like a bug.)

Adding this class will be a big long-term help to theme developers and I think cause much less breakage than a change like the buttons block markup.

@talldan
Copy link
Contributor

talldan commented May 15, 2020

I'm open to other opinions, but my opinion is that what's being proposed here prioritizes the life of developers over end users.

It'd be good to see some other options being proposed that don't result in unknown consequences, like using new class names, and then gradually phasing out the old ones.

@carolinan
Copy link
Contributor

carolinan commented May 21, 2020

Here is a list of 140 themes that uses .has-background
https://www.wpdirectory.net/search/01E8W17CDZK0CJG23ZY8WCC8FG

Note that I added a space after background in the search to avoid catching things like
has-background-dim

If you want to see all of the results, there are 272 themes:
https://www.wpdirectory.net/search/01E8W15J82H0FYYKVWHW1WSNH7

I did not do an analysis. A very quick browse shows that the most common use is for adding padding to elements that has the class.

    p.has-background {
        padding: 20px;
    }

@mrwweb
Copy link
Author

mrwweb commented May 22, 2020

@talldan I'll just reiterate again that this feels more like a bug and making up for an oversight rather than an additional feature. I don't see how .has-background could be transitioned away from either as that would be much more damaging to existing themes than even adding it to a few more elements.

prioritizes the life of developers over end users

Forcing developers to deal with an unclear and inconsistent naming scheme (see columns below) that requires producing very complicated selectors and long CSS rulesets is a recipe for bugs and bloated files which are both extremely negative outcomes for users.

@carolinan report is super useful! I haven't exhaustively gone through it, but here are a few things I've noticed:

  • Of the top hits, a majority are for selectors like p.has-background or .wp-block-group.has-background where adding that class to a new block would have no impact because the selector is qualified by an element or another class. A number of the more complicated selectors are probably similar in impact given how complicated their selectors are.
  • A lot of the less popular themes seem to have a single use of it that is something like .post-content .has-background { background-color: #272F38; } (from Wilson). Given that is the only use of the class in the theme, it's either working and would probably apply to things it should have been already, or was breaking things and would just continue being broken.

A another useful search is for .has-background (space before and after) which will find most unqualified selectors. That only produces 54 matches: https://www.wpdirectory.net/search/01E8YTQG723ZFPKRHSR52FCHC8

The result of all this is that I'm even further convinced that theme authors have been using .has-background under the assumption that it applies to any element with a background.

Finally, there are only for hits for ^.has-background {: https://www.wpdirectory.net/search/01E8YVM52ZVAF4SVRAH8WYMZEX. All four themes are applying either a background color or padding and that would almost certainly result in a nicer theme design for end users.

Columns is already doing this

Further calling out the need for standardization here, I can see that when using the Gutenberg plugin, the columns block adds .has-background when it's using a gradient background. This is the exact behavior I've proposed (though ideally it would also add a .has-gradient-background as well). Clearly, the idea that .has-background was intended only for background colors is unclear and undocumented, so it doesn't really feel like a real rule that is worth following.

Testing

Finally, I created a cover block and added the .has-background class to it for both Twenty Twenty and the Go theme which would appear to have the most complicated .has-background selectors of any theme I looked at. In neither case did it have a negative impact. Precisely it had no impact.

Conclusion

There is already existing inconsistency and we need to resolve it somehow. Adding more classes feels like a solution that would lead to clutter and confusion so long as .has-background still exists. Given that .has-background can't be removed now that it's in use and both my testing and review of selectors indicates very little impact (either negative or positive), it feels like following the path I've suggested as quickly as possible is the best path forward.

Feedback always welcome.

@carolinan
Copy link
Contributor

Unqualified guess, the themes that uses CSS like
.post-content .has-background { background-color: #272F38; }
aren't styled for the block editor, they are old styles.

@mrwweb
Copy link
Author

mrwweb commented May 24, 2020

@carolinan That was also my guess. Lucky for us, practically all of the most popular themes that have a rule like that are by @andersnoren of Chaplin and Twenty Twenty fame.

Would you mind weighing in, Anders? How would this change impact your themes positively or negatively?

@andersnoren
Copy link

andersnoren commented May 24, 2020

Most of my themes that set a background color on .entry-content .has-background or :root .has-background uses it as a fallback background color, for blocks that have a background color class that isn't included in the theme. For example, if a block has .has-background.has-magenta-background-color because it was created in a different theme, Wilson will default it to a suitable background color since the class .has-magenta-background-color isn't styled in Wilson. Chaplin doesn't use the class like that, and I'll probably end up removing it from my old themes as well at some point.

Pretty much all of my themes style p.has-background and some style .wp-block-group.has-background, in both cases to apply padding, which wouldn't cause any issues with using the class in the way proposed here. I'm all for it.

@carolinan
Copy link
Contributor

I am thinking that with global styles, we will also have a different way to add padding.

@mrwweb
Copy link
Author

mrwweb commented Aug 6, 2020

This is officially a bug, as it is now clear that the Cover block is the only block not following this behavior. I've audited WP 5.5-rc2 for background-related classes and found the following:

  1. The Cover block is the only block that does NOT use has-background when there is a gradient applied.
  2. The Cover block is the only block that uses the has-background-gradient class.

Background Classes Audit

{name} = placeholder for named colors and gradients

Cover Block

  • Color: has-{name}-background-color, has-background-dim
  • Gradient: has-background-dim has-background-gradient has-{name}-gradient-background
  • Image: has-background-dim

Button

  • Color: has-{name}-background-color, has-background
  • Gradient: has-{name}-gradient-background, has-background

Media & Text

  • Color: has-{name}-background-color, has-background
  • Gradient: has-{name}-gradient-background, has-background

Columns

  • Color: has-{name}-background-color, has-background
  • Gradient: has-{name}-gradient-background, has-background

Group Block

  • Color: has-{name}-background-color, has-background
  • Gradient: has-{name}-gradient-background, has-background

Proposed Solution

To reiterate:

  1. Add has-background to all blocks that have any form of background: color, gradient, image, video, and who knows what else
  2. Consider adding classes for background type like has-background-gradient and has-background-image to all blocks that support any type of background in addition to has-background

Next Steps

  • I will update the title of this issue to clarify.
  • Could someone please relabel this as a bug? @talldan?
  • How can we get this addressed in the soonest possible release?

@mrwweb mrwweb changed the title Use .has-background to indicate any type of background. Introduce second class for type of background. Use .has-background for Cover block consistently with all other blocks. Consider adding background type class to all blocks. Aug 6, 2020
@metalandcoffee
Copy link

Stopping by because I've been having issues understanding what the purpose of the has-background-dim class on the cover block is. With the order of classes that is rendered on the front-end, it's beating my selected background color and forcing me to use !important to get the user-selected color to actually take precedence.

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CSS Styling Related to editor and front end styles, CSS-specific issues. [Feature] Themes Questions or issues with incorporating or styling blocks in a theme. [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

No branches or pull requests

6 participants