Use a var for user presets to avoid having to use ! important to enforce them #40335
Conversation
glendaviesnz
added
[Status] In Progress
|
Size Change: +196 B (0%) Total Size: 1.24 MB
ℹ️ View Unchanged
|
|
I can't figure out how to get this running locally to check it out firsthand, but based on the code change it looks like you're outputting the color both ways, like this? .has-magenta-background-color {
background-color: magenta;
--wp--user--preset--background-color: magenta;
}In my themes I only output the custom property and assign the background color on .has-magenta-background-color {
--wp--user--preset--background-color: magenta;
}
.has-background {
background-color: var(--wp--user--preset--background-color);
}Aside: This also makes it easier to assign a complementary text color using |
This was a quick hack yesterday to see if this concept worked as it was really easy to slot this into the same spot where the var was being added. Next week I will take a closer look at whether it can be added under the |
glendaviesnz
force-pushed
the
try/use-css-var-for-user-presets
branch
from
6660015
to
decbc38
Compare
|
One issue with this approach is that the theme css is loaded after the global css, so there is potential for theme styles to override the user applied styles. One possibility would be to load the theme styles first, but that was discussed here, and is not possible as it could cause breakages in themes that are expecting to be able to override other global styles via load order only. Taking a look at whether we can decouple the user presets from the global styles and load them in the footer to try and make it more likely that they will take precedence of theme styles. |
Have separated out the loading of the user preset css and enqueued with a lower priority, so in most cases should load after theme css - we could look at loading in footer instead. |
|
Loading in the footer would cause weird render jank since the styles wouldn’t be parsed until well after content was rendered, like this bug that was just filed on trac. |
yeh, I don't think it is an option at all in this case - as some of these styles are critical to the look of the page - thanks for the link to the related bug, always good to have supporting reasons for a decision. |
glendaviesnz
requested review from
TimothyBJacobs and
spacedmonkey
as code owners
glendaviesnz
removed
the
[Status] In Progress
|
@cbirdsong you can download a copy of this PR as a plugin build from here for testing. This approach seems to work, except in cases where a theme has a setting with a higher specificity for something, eg. if a theme had something like it would need to be updated to or to prevent this theme css overriding the user preset. |
There was a problem hiding this comment.
Nice work navigating all the nuances around this issue @glendaviesnz 👍
Given the issue that this PR aims to address, I believe the tradeoff of requiring some themes to update a few styles is acceptable. So long as we continue to communicate with the wider community about the issue we're tackling and how/why we've settled upon this approach.
In general, the code changes look pretty good although I did hit one issue while testing.
Add a post with two tables, and select user defined colors on one, and make sure both display as expected in frontend
I added a gradient background to table blocks via global styles. This wasn't being overridden by user-selected flat colors in both the block editor and frontend.
- Selecting new preset or custom gradients did work
- Selecting custom or preset flat colors failed to take precedence
Video of issue
Screen.Recording.2022-04-20.at.7.07.18.pm.mp4
Edit: Looks like the issue I encountered is also present on trunk as well.
I don't see that style in either version of Twenty Twenty-One in the theme directory, but it would be better to discourage targeting specific elements for color instead of inheriting from the parent container? One other problem I see with this as it is: the cover block does not use <div class="wp-block-cover is-light">
<span aria-hidden="true" class="wp-block-cover__background has-pale-cyan-blue-background-color has-background-dim-20 has-background-dim"></span>
<div class="wp-block-cover__inner-container">
</div>
</div>The cover block CSS is a real mess, but if it can't be switched use
The table block seems to have a lot of issues around color inheritance. The per-block setting doesn't cascade into the figcaption in any scenario, and once I set dark/light green custom block text/background colors in the site editor it looked like this in the editor, but was unchanged in the front end:
|
🤦 My local dev env was loading an old version of the theme that I forgot to revert! Sorry to have wasted your time looking for that. |
Nice catch! I would like to refactor that Cover block completely! ... but for the sake of an easy fix for this PR I have added a background setting to the cover block css. |
Thanks @aaronrobertshaw, this was caused by the fact that |
This seemed to work for me, this is the frontend view: What theme are you using when testing this? |
glendaviesnz
force-pushed
the
try/use-css-var-for-user-presets
branch
from
c25f18d
to
d06cf34
Compare
I'd like to clarify this point, which is not true with my current understanding. I'll be happy to update my understanding. I'd like us to grow a common ground of the problem to solve, otherwise is difficult to compare trade-offs and make a decision. Let me share what I know and, please, correct me if I'm missing something. Using Can these themes set the value they want depending on media queries? Yes, they can: @media (min-width: 600px) {
body { --wp--preset--font-size--normal: <new_value>; }
}That's all the CSS they need. Allowing this use case is precisely the reason we introduced CSS Custom Properties in preset classes. Based on this, what I see is that the approach proposed in this PR forces all themes and all blocks to potentially update their CSS to make sure they don't overwrite the user presets. So, instead of asking the subset of themes that have the issue to update, this PR proposes we ask a much larger community. I'm invested in finding a fix for the subset of themes that have the issue, but I'm not convinced that asking everyone else to update is a good trade-off. (I just wanted to share my high-level view before looking at the implementation). |
|
I've looked at implementation and this is what I see.
.has-black-color {
--wp--user--preset--color: var(--wp--preset--color--black);
color: var(--wp--user--preset--color);
}
.wp-block-cover [class*="-background-color"] {
background-color: var(--wp--user--preset--background-color);
}If the problem to solve is responsive typography for themes that enqueue their own classes, I understand how removing the The part I don't understand of this proposal is why we need to create new generic CSS Custom Properties and promote them as public API for themes & blocks to use in their stylesheets. The direction so far has been the opposite: instead of leaking implementation details (the CSS Custom Properties) to the blocks or themes, try to absorb the block styles into the |
| $presets_stylesheet = gutenberg_get_global_stylesheet( array( 'presets' ) ); | ||
|
|
||
| wp_register_style( 'use-preset-styles', false, array(), true, true ); | ||
| wp_add_inline_style( 'use-preset-styles', '.has-background { background-color: var(--wp--user--preset--background-color);} .has-text-color {color: var(--wp--user--preset--color);}' . $presets_stylesheet ); |
There was a problem hiding this comment.
I understand why we do:
.has-text-color {
color: var(--wp--user--preset--color);
}
.has-black-color {
--wp--user--preset--color: var(--wp--preset--color--black);
}Instead of:
.has-black-color {
--wp--user--preset--color: var(--wp--preset--color--black);
color: var(--wp--user--preset--color);
}Though I think is a premature micro-optimization that makes everything more complex (more effort to maintain and future bugs).
To improve size performance of the global styles stylesheet, I think we'd be better off exploring some of these paths:
- make sure the stylesheet is cacheable
- only enqueue the styles that are actually in use by a given post Theme.json: Block specific CSS is printed even if a block is not used on the current page #31642
Performance wise, it's also worth noting this approach means 28k of CSS (global styles + use presets stylesheet) vs the 18k in trunk (global styles stylesheet).
There was a problem hiding this comment.
With this, .has-text-color and .has-background could easily move back into a cacheable file and don't need to be injected as an inline style, leaving only the actual color classes in a style tag.
Also, looking at the code generated, we should not need to add the background: declaration to the .has-<name>-background-color and .has-<name>-background-gradient classes. It should just look like:
.has-background {
background-color: var(--wp--user--preset--background-color);
background-image: var(--wp--user--preset--background-gradient);
}Then the individual .has- classes then just set those properties.
There was a problem hiding this comment.
Though I think is a premature micro-optimization that makes everything more complex
Yep, definitely worth reviewing the optimization versus complexity here if we move forward with this concept.
There was a problem hiding this comment.
I have moved the .has-background definition from inline style to static css file, and also integrated the gradient backgrounds into it which removed approx 6kb from the inline css
| @@ -50,6 +50,10 @@ | |||
| background-color: $black; | |||
| } | |||
|
|
|||
| [class*="-background-color"] { | |||
| background-color: var(--wp--user--preset--background-color); | |||
There was a problem hiding this comment.
I don't understand why we do this if the cover block doesn't have any block supports related to this (color, background)?
There was a problem hiding this comment.
This is just a side effect of the potentially premature optimisation of .has-background class, ie. Cover uses the likes of the has-white-background-color preset classes, but doesn't assign the corresponding .has-background class. Adding this style just avoids a deprecation to add that class, but better options are probably to rethink that optimisation as you note, or to refactor the cover block to use the color supports and add the associated classes.
I don't really see how this change affects a much larger community. Removing However, adding
Having said all that, I think |
I don't think you can anticipate every possible situation where a color might need to be applied by a theme/block style, and using custom properties like this allows for a theme/block dev to take user preferences into account while also leaving flexibility for how CSS can be written compared to forcing it on with These custom properties would only need to be used in situations where a block needs to use the custom color in an unexpected way. In the case of the cover block, this is only required because it doesn't use the standard |
I find this concerning for a few reasons: We're trying to circumvent the existing issues of the block supports mechanism with a limited fix instead of addressing them. One example: how the global styles sidebar can expose an UI for the user to change the cover block color with this approach? We actually used something like this in the first iterations. See the PR that introduced them in the button block and the PR were it was reverted. I've documented the struggles we ran into in this issue: essentially, 1) nested content was problematic (inner blocks, patterns) and 2) undefined css vars made css properties that don't inherit (background-color) to inherit. |
@webmandesign can you please confirm if the above approach, noted here, would resolve your issue with the responsive typography caused by the addition the
@cbirdsong are you able to provide a concrete example of where the use of |
Reading through those, I can see (and have personally experienced) how custom properties applied that way can cascade unexpectedly, but limiting the application of the There will certainly be cases where a CSS author will have to apply a With
Footnotes
|
This may be a valid way to accomplish responsive typography within the constraints of
The most common scenario is buttons, which feel broken without hover/active states. It is difficult to meet color contrast standards when adding them using opacity/filter, so I use a Sass function to automatically choose complementary active shades for each color. Often it's just darkening/lightening the color by a percentage, but on others I've had them hand-curated by the designer or automatically calculated them from a list of potential shades in the site's palette. Simplified example: @each $name, $color in $colors {
$color-active: color-shift($color, 33%); // darkens or lightens the color by 33%, depending on if it's a light or dark color
.has-#{$name}-background-color {
--button-color: #{$color};
--button-color-active: #{$color-active};
--button-color-inverse: #{color-contrast($color)}; // chooses white or black text, depending on if it's a light or dark color
--button-color-inverse-active: #{color-contrast($color-active)};
}
.has-#{$name}-color {
--button-color-inverse: #{$color};
--button-color-inverse-active: #{$color-active};
// these come after the ones in the background color class so setting a custom text color is still possible
}
}On occasion I've also created lightened/transparent versions of colors for the background of the outline button style. @each $name, $color in $colors {
.has-#{$name}-background-color.is-style-outline {
--button-color: #{transparentize($color, 0.3)}; // the color at 30% opacity
--button-color-active: #{transparentize($color, 0.15)}; // the color at 15% opacity
--button-color-inverse: #{$black}; // the text color is always black, in this case
--button-color-inverse-active: #{$color}; // then switching to the full opacity color on hover
}
}These can both be applied using the same CSS1: .wp-block-button > .wp-block-button__link {
color: var(--button-color-inverse);
background-color: var(--button-color);
&:hover, &:focus, &:active {
color: var(--button-color-inverse-active, var(--button-color-inverse));
background-color: var(--button-color-active);
}
}Outside of buttons there are simply scenarios where it might make more sense to apply a text/background color elsewhere within the block. One recent theme I worked on had a block style for the Media & Text block where the content element overlapped on the image, like so:
Because my .wp-block-media-text.is-style-overlapping {
.wp-block-media-text__content {
background-color: white;
transform: translateX(-8%);
border-radius: 1.5rem / 0.25rem;
box-shadow: 0 0 0 0.2em black;
}
}
.wp-block-media-text.is-style-overlapping.has-background {
background-color: transparent;
.wp-block-media-text__content {
background-color: var(--wp-background-color);
}
}Again, I could do all of this on a stock 5.9 install by just using Footnotes
|
|
@cbirdsong thanks for all your input - your comments and examples are really helpful in building up the full picture of the issues at play here. |
Thanks for the extra context @oandregal, will take a look. |
Thanks for sharing. Let's say the theme wanted yellow to be the hover color: .wp-block-button:hover {
--wp--preset--color--black: yellow;
--wp--preset--color--cyan-bluish-gray: yellow;
--wp--preset--color--white: yellow;
--wp--preset--color--pale-pink: yellow;
--wp--preset--color--vivid-red: yellow;
--wp--preset--color--luminous-vivid-orange: yellow;
--wp--preset--color--luminous-vivid-amber: yellow;
--wp--preset--color--light-green-cyan: yellow;
--wp--preset--color--vivid-green-cyan: yellow;
--wp--preset--color--pale-cyan-blue: yellow;
--wp--preset--color--vivid-cyan-blue: yellow;
--wp--preset--color--vivid-purple: yellow;
/* add another custom property for every color defined by the theme as well */
}It can be done and it is ugly. I understand why indirection through a generic custom property in the preset classes would make this particular use case easier to achieve. Though, it is not necessary if we remove the I feel that we're trying too many things at once in this PR: either we propose a solution that addresses the hover/focus/active statuses and/or responsive/intrinsic web design across all layers (user block styles, user global styles, theme global styles) or we focus on a proposal that helps themes for which How can me make progress in the second problem?
Unless there's any other thing we can try, in my view, we're stuck at either asking these subset of themes to update their hover/viewport code (current approach) or asking every theme with no (Sorry for the long comment, I wanted to braindump all I could before I start a long off-work period where I won't be able to participate in these conversations. I hope it has been useful and, please, know that I appreciate a lot the drive to keep looking for solutions to our current struggles.) |
To be clear, I'm not trying to solve hover/active styles or any other larger thing with this, just offering examples where
I don't understand why the universal removal of |
|
Sorry about the delay in further comment on this, I got tied up with some other things, will hopefully get back to it tomorrow. |
…clare them at block level
…and background-color as only they have generic .has-color and .has-background class to declare these in
… use the .has-background class
…ration, and in the application to the given property to avoid confusion
… gradients to reduce css by approx 6kb
glendaviesnz
force-pushed
the
try/use-css-var-for-user-presets
branch
from
0186966
to
38a5047
Compare
|
I wonder if this PR will help to solve this in a different way. By specifying the block CSS in block.json, we can ensure that the correct order of precedence for settings is always adhered to (user > theme > core) without needing to use CSS tricks like strong selectors or !important. |
What?
Removes the
! importantthat is being applied to user applied style presets. This was added here in order to fix an issue with theme style settings overriding user selected settings.Why?
The application of the
! importantacross all the predefined classes has been a breaking change across a range of themes (#38252, #34575, #37590) as it overrides the valid use of some of these presets, and in some cases in a way which can't be worked around at the theme level, particularly with responsive typography.While it is important to ensure that the styles applied by the user in the editor take priority over global styles, and theme styles, I think it is worth reconsidering the use of
! importantto achieve this for the following reasons:! importantapplied it will still override the user selection! importantin order to work due to the removal of a wrapper around the Post Title block. While it will still be an issue with any blocks applying these styles with a selector of specificity higher than (0,1,0) this can potentially be solve by appending the preset classes to these custom selectorsWhile this change may make it easier for theme styles to override, either accidentally or intentionally, user applied styles, clear documentation for theme authors on how to prevent this with the use of lower specificity selectors may be a better long term solution than trying to enforce it with
! important.How?
!importantfrom the preset classes.wp-block-columns h2 { background-color: var(--wp--user--preset--background-color,black);N.B. This approach works as expected across the various levels of style application in the likes of TT2 theme, but it does mean that themes or plugins could override user presets if a higher specificity is used - but they can already override them by adding a higher specificity and their own
! importantTesting Instructions
Go to the editor, add three post title blocks:
Publish the post and go to the front end.
Add some theme.json element settings:
The expected behavior would be that the user choices are respected, both in the editor and front-end.