WordPress / gutenberg

The Block Editor project for WordPress and beyond. Plugin is available from the official repository.
https://wordpress.org/gutenberg/
Other
10.15k stars 4.05k forks source link

Allow developers to opt out of !important CSS rules generated from theme.json #37590

Open MarieComet opened 2 years ago

MarieComet commented 2 years ago

What problem does this address?

Currently, CSS rules generated from theme.json settings get !important automatically added. It's fine for theme authors who need to ensure that their CSS rules are not overridden by plugins or so. But as a theme developer, working at web agency, I don't need those !important. I control the scope of custom plugins.

What is your proposed solution?

My solution is to add a filter, to opt out of adding those !important to CSS, in the compute_preset_classes function. eg : add_filter( 'compute_preset_classes_important', '__return_false' );

MarieComet commented 2 years ago
/**
 * Given a settings array, it returns the generated rulesets
 * for the preset classes.
 *
 * @since 5.8.0
 * @since 5.9.0 Added the `$origins` parameter.
 *
 * @param array  $settings Settings to process.
 * @param string $selector Selector wrapping the classes.
 * @param array  $origins  List of origins to process.
 * @return string The result of processing the presets.
 */
private static function compute_preset_classes( $settings, $selector, $origins ) {
    if ( self::ROOT_BLOCK_SELECTOR === $selector ) {
        // Classes at the global level do not need any CSS prefixed,
        // and we don't want to increase its specificity.
        $selector = '';
    }

    $important_rule = apply_filters( 'compute_preset_classes_important', true );
    $important = $important_rule ? ' !important' : '';

    $stylesheet = '';
    foreach ( self::PRESETS_METADATA as $preset_metadata ) {
        $slugs = self::get_settings_slugs( $settings, $preset_metadata, $origins );
        foreach ( $preset_metadata['classes'] as $class => $property ) {
            foreach ( $slugs as $slug ) {
                $css_var     = self::replace_slug_in_string( $preset_metadata['css_vars'], $slug );
                $class_name  = self::replace_slug_in_string( $class, $slug );
                $stylesheet .= self::to_ruleset(
                    self::append_to_selector( $selector, $class_name ),
                    array(
                        array(
                            'name'  => $property,
                            'value' => 'var(' . $css_var . ')' . $important,
                        ),
                    )
                );
            }
        }
    }

    return $stylesheet;
}
overclokk commented 2 years ago

I think that the right place is to have a field in the theme.json itself instead of adding a filter.

MarieComet commented 2 years ago

I think it's fine in theme.json, but I don't know how to propose this change (code) :)

MarieComet commented 2 years ago

Just thinking : a field in theme.json can't make possible to disable !important from child theme or plugin. PHP filter, yes.

overclokk commented 2 years ago

Yes, not yet but a theme.json is a better place for users, not only for developers.

oandregal commented 2 years ago

Hi Marie, I'd be interested in learning what issues does this causes for you so we can provide help.

I also want to share the rationale for why we use this technique: we need to respect user choices at all times. There're blocks whose specificity is higher than 010, hence the presets classes attached by the user won't be applied if we don't use !important. See this thread for a longer review. If there's an alternative that fixes that issue and doesn't use !important I'd love to investigate it.

glendaviesnz commented 2 years ago

I'd be interested in learning what issues does this causes for you so we can provide help

@jonschr, @webmandesign, @cbirdsong would any of you have a public theme in a repo, or be able to set one up, that demonstrates the issues around this - a classic theme, and a theme.json one if possible/applicable - and some reproduction steps for how to add content to a site running the theme to replicate the issues if needed. While exploring solutions it would be helpful to have some real-world scenarios to test against.

webmandesign commented 2 years ago

@glendaviesnz

You can test with my Michelle theme (https://wordpress.org/themes/michelle/):


Steps to reproduce the issue:

  1. With WordPress 5.9.2 install Michelle 1.2.0.
  2. Create a page and add a content from my gist.
  3. Publish the page and check how it looks like on front end. The font size in both columns should be equal. This is OK in WordPress 5.9.1 (unlike WP5.9). But for my theme the issue persist on smaller screens. (So you guys have technically fixed the issue probably for most cases in WP5.9.1. But the main issue we have is with !important styles.)
  4. Now update the theme to version 1.3.5 and check the page again. You'll see both columns have the same font size on all screen sizes.

Results:

I can only speak for myself. The issue was relatively fixed in WordPress 5.9.1 - meaning the font size is OK on large screens. But the real issue for me persist: WordPress still applies !important styles here.

The theme is set with responsive typography and you can still see the issue when resizing the screen (with Michelle 1.2.0 and WordPress 5.9.1+).

The only way around this is to remove !important rules applied by WordPress via the theme, or update the theme CSS code to use WordPress CSS variables naming.

In my opinion, though, there should be no !important styles applied by WordPress. That's the main issue here and without it everything would be fine.

If !important resolves some issue with theme.json themes, maybe go ahead there if that's really the only solution (although I still feel there must be a better way). But why messing up existing themes? Especially themes that promote using block editor?


Finally, I think we should also make more distinction in "classic themes" terminology. Among "classic themes" there are themes that don't support Gutenberg editor and the ones that do. So, I suggest using this terminology:

This whole issue seems to be affecting block editor themes only.

cbirdsong commented 2 years ago

I can set up a sample theme later this week, but for now I can describe some issues.

For font sizes, responsive typography is a big one. Even in theme.json themes you can only accomplish it through a clamp() value, which is much less straightforward than media queries and has other issues. (see below)

For block editor themes, another issue is that you can only supply pixel units for fonts in add_theme_support( 'editor-font-sizes'). Since I always use em or rem units and adjust them responsively , I would usually just pick a pixel value that looked correct-ish in the editor at common laptop resolutions, and then do the real thing with rem/em and clamp()/media queries in my CSS. The value I supplied was never intended to be used on the front end, and 5.9 overrode all my styles with it.

However…

This whole issue seems to be affecting block editor themes only.

This is untrue! There are plenty of reasons you might want to override a value, even within a theme.json theme. For instance, this is a common pattern I use when declaring any value with min(), max(), or clamp():

.has-large-font-size {
    font-size: 1.25rem;
    font-size: clamp(1.25rem, 1.25rem + 0.5vw, 1.8rem);
}

min(), max(), or clamp() are supported pretty well (~90% worldwide), but 10% is a lot pf people, and declaring the static font-size value before the clamp() one ensures those visitors still get a reasonable version of your design. The fact that you can only declare a single !important value in theme.json makes it impossible to offer this straightforward bit of progressive enhancement.

This only gets into issues created for font sizing. This approach creates many similar problems in other areas (most notably colors), which like this leads me to agree here:

In my opinion, though, there should be no !important styles applied by WordPress. That's the main issue here and without it everything would be fine.

!important should be treated as a “break glass in case of emergency” tool. Using it correctly requires a CSS author to consider how it would affect the project as a whole, but this just slaps it on everything without judgement, resulting in the inability to use CSS as designed to handle responsive typography, dark mode and even changing link colors on hover/focus, which is as fundamental and basic as it gets.

It should absolutely not be applied retroactively to pre-5.9/theme.json v2 themes, and I'd strongly suggest it not be applied at all, no matter how much easier it makes the job of the editor authors.

webmandesign commented 2 years ago

@cbirdsong

For block editor themes, another issue is that you can only supply pixel units for fonts in add_theme_support( 'editor-font-sizes'). Since I always use em or rem units and adjust them responsively , I would usually just pick a pixel value that looked correct-ish in the editor at common laptop resolutions, and then do the real thing with rem/em and clamp()/media queries in my CSS. The value I supplied was never intended to be used on the front end, and 5.9 overrode all my styles with it.

Indeed, I experience the same issue here, but this was true also pre-WP5.9 unfortunately. I approach it similarly as you do. I kind of made peace that add_theme_support( 'editor-font-sizes' ) will not be updated to work with relative units or even with CSS variables.

This is untrue! There are plenty of reasons you might want to override a value, even within a theme.json theme.

Sorry, I don't have enough experience with theme.json themes. But I can see your point. I hope this will be fixed in the future somehow. The issue I can see here is that for some reason it looks like CSS is being replaced with JSON in WordPress themes. I'm skeptic this approach would ever provide the same flexibility as CSS does... But I have to dig deeper into theme.json theme development to see what it's capable of - currently it's very early stage to match classic themes flexibility.

However, with theme.json theme I think I would approach your issue via CSS variables named according to WordPress naming convention. Basically, I'd updated your CSS code:

/* From this: */
.has-large-font-size {
    font-size: 1.25rem;
    font-size: clamp(1.25rem, 1.25rem + 0.5vw, 1.8rem);
}
/* To this: */
body {
    --wp--preset--font-size--large: clamp(1.25rem, 1.25rem + 0.5vw, 1.8rem);
    /**
     * You should be able to use @support now for backwards compatibility,
     * or simply revert to @media query approach or do whatever you like with
     * the CSS variable.
     * This should work with core's `!important` without any issue I think.
     */
}

By setting up the CSS variable I think you can do the whole responsive typography spiel to your liking and you don't even need to declare styles for .has-large-font-size class. But this is just from top of my head and I haven't tried this ;)

jonschr commented 2 years ago

@glendaviesnz The remaining issues for me are, like others, around responsive styles. I use a sass mixin for smooth scaling up and down font sizes between a desktop size and a mobile size, and the !important declaration overrides it, resulting in a normal experience on desktop and incorrectly-sized text on mobile for any text that's set to small, large, xlarge, etc.

For me, however, this is the same whether I'm in an add_theme_supports theme or a theme.json theme. The !important declaration slightly exacerbates this.

I'm not a huge fan of using !important declarations in general, but in this particular case I don't actually have a huge problem with its use, as the user is setting a size in the Gutenberg UI and !Important helps to enforce that choice, overriding defaults that in some cases probably do need to be overridden.

My biggest thing would probably be that theme developers don't really have a recommended way to add support for different sizing on mobile that would inherit the !important declaration (I see some suggestions up above that I haven't tried, but haven't seen similar stuff in the documentation).

cbirdsong commented 2 years ago

By setting up the CSS variable I think you can do the whole responsive typography spiel to your liking and you don't even need to declare styles for .has-large-font-size class. But this is just from top of my head and I haven't tried this ;)

This is true, but that doesn't set a progressively enhanced fallback for browsers that don't support custom properties.

More importantly, it doesn't fix all the existing themes that don't have anyone actively developing them and assumed no one would be coming through and adding "!important" to these classes.


I'm not a huge fan of using !important declarations in general, but in this particular case I don't actually have a huge problem with its use, as the user is setting a size in the Gutenberg UI and !Important helps to enforce that choice, overriding defaults that in some cases probably do need to be overridden.

Actually, these are !important-ed with theme-supplied font size presets and custom font sizes disabled.

For reference, when starting with this: ```php add_theme_support( 'disable-custom-font-sizes' ); add_theme_support( 'editor-font-sizes', array( array( 'name' => __( 'Small', 'block-editor-theme' ), 'size' => 12, 'slug' => 'small' ), array( 'name' => __( 'Medium', 'block-editor-theme' ), 'size' => 16, 'slug' => 'medium' ), array( 'name' => __( 'Large', 'block-editor-theme' ), 'size' => 24, 'slug' => 'large' ), ) ); ```
The CSS generated looks like this: ```css body { --wp--preset--font-size--small: 12px; --wp--preset--font-size--medium: 16px; --wp--preset--font-size--large: 24px; } .has-small-font-size { font-size: var(--wp--preset--font-size--small) !important; } .has-medium-font-size { font-size: var(--wp--preset--font-size--medium) !important; } .has-large-font-size { font-size: var(--wp--preset--font-size--large) !important; } ```
glendaviesnz commented 2 years ago

Thanks for the extra details and sample themes, everyone. I am just trying to get a couple of other unrelated PRs finished off and will hopefully have a closer look at this tomorrow.

glendaviesnz commented 2 years ago

Update: Thanks for all the extra detail around how to replicate the specific issues caused by this change everyone. I am coming fresh to this from both the theme perspective, and the editor change side, so I have just been taking a bit of time to make sure I understand the issues correctly from both ends. I have been able to replicate the issue caused by the ! important at the theme end (thanks @webmandesign), and also from the editor end for user applied defaults if ! important is not applied.

I am now just pondering potential solutions, and will hopefully come back with some ideas, and probably more questions also, tomorrow.

Actually. Given how the style system works, !important is a good choice for preset classes the user attaches to the post content. If those values need to be overridden, in addition to regular CSS, we can offer hooks for the theme.json processing, so plugins have an alternate mechanism to modify this behavior.

@oandregal you mention the above here, was any work ever started on the hooks for overriding the behaviour? There is this PR, but it looks like it was abandoned.

glendaviesnz commented 2 years ago

Finally, I think we should also make more distinction in "classic themes" terminology

@webmandesign it was actually difficult to find a clear definition of what constitutes a Classic versus a Block, versus an FSE theme, so I have added some detail about this to the handbook (But I noticed I left out details about FSE, so PR here for that).

In terms of the block editor, there are really only two types of themes, Classic and Block, and the distinction is based on there being .html format templates in the /block-templates or /templates, if there are it is a Block theme, otherwise, it is considered Classic even if it uses some of the block editor theme supports.

Also, a Classic theme can use theme.json to add editor supports instead of add_theme_support, and doing so doesn't make it a block theme in the eyes of the editor, and doesn't make it eligible for FSE. Only Block themes with the above-mentioned templates and folders will activate the FSE functionality.

This whole issue seems to be affecting block editor themes only

I did some testing across classic themes without any editor supports, classic theme with editor supports, classic theme with a theme.json and a block theme and they were all affected by this issue given the right combination of CSS selectors in the theme.

What sort of a solution we come up with to the ! important issue might dictate whether it is worth being able to identify in code if a theme is classic but with block editor supports or not, but given that this issue effects both types of themes there may not be any value in making that distinction. In terms of backwards compatibility this issues seems to indicate that we need to look at the impact on Classic themes in general rather than just those with editor supports added.

glendaviesnz commented 2 years ago

So in order to move this issue forward, I have put together a very draft PR with one solution, which takes into account the preference stated a few times that this change shouldn't be automatically applied to pre5.9/non-theme.json themes.

This is just one possible approach, and there may be some gotchas that I am overlooking, so no guarantees that we could move forward with an approach like this, but it at least keeps the conversations moving.

This draft PR:

Pros

Cons

/cc: @oandregal

cbirdsong commented 2 years ago

This would work for my needs, but I'd still argue using !important at all is a mistake.

!important being present by default is going to affect (and often complicate) CSS written elsewhere in the ecosystem, which then might not work as expected if !important is absent, either on classic themes or because it was disabled by the theme author.

For instance, the example given in https://github.com/WordPress/gutenberg/pull/29533#discussion_r586724054 as a reason for using !important is applying a color to the post title block. That block is available in classic themes, and after !important is removed changing its color will no longer work. That specific case can also be easily worked around by just using the cascade:

.wp-block-post-title {
  color: green;
}
.wp-block-post-title h1 {
  color: inherit;
}

.has-red-color {
  color: red;
}

(Live Codepen demo: https://codepen.io/cbirdsong/pen/gOombYv)

Keeping !important in the mix either conditionally or universally will also complicate writing editor CSS for future features, like https://github.com/WordPress/gutenberg/issues/4543, https://github.com/WordPress/gutenberg/issues/34717 or https://github.com/WordPress/gutenberg/issues/27075.

!important might have solved some short-term problems, but it will create just as many in the long term, and casual use of it sets a poor standard for CSS in the WP ecosystem. We should just get rid of it.

glendaviesnz commented 2 years ago

This would work for my needs, but I'd still argue using !important at all is a mistake.

@cbirdsong this was discussed in some additional recent comments over on the PR - linking here for reference - but it was discussed there that it would be good to see this suggested fix as a temporary quick fix to at least unblock the likes of the responsive font issues while the full removal of ! important was explored.

Jeremy-Bolella commented 2 years ago

It's really frustrating that this issue doesn't have a maintainable hot-fix yet beyond just dequeuing the entire preset style sheet. I was a huge fan of Gutenberg for almost 2+ years even through all the weird oddities/bugs. The 5.8+ update and the FSE release has made Gutenberg development unbearably difficult.

I tried FSE and realized it's a complete paradigm shift, cancelled that adventure. I get the reasoning for a lot of the blackboxing going on with Gutenberg and the new block themes but this should only be this rigid on Block themes. Classic theme developers are freaking out left and right with all of these changes and lack of consideration for us.

I solved the majority of issues that I had with the current release (inline layout/blockGap styles, wide/full align that just works, odd image block issues, integrating theme.json pretty intimately with my theme without really ever having to touch the json file itself etc...). All of this while feeling pretty confident that long-term my code shouldn't explode with future gutenberg updates.

This issue though. It's unsolvable without being a maintenance nightmare. To preface, I just want hover styles on my buttons.

Things I've Tried:

At the end of the day I really only need to hook a couple lines of code in the WP_Theme_JSON class to enable the functionality I need without completely reinventing the wheel. Instead the class has a pretty strong warning to not touch/extend it. There are no filters/hooks, and I have not successfully found a workaround for the 5-10 lines of code that would have solved this issue days ago.

I'm sorry in advance for the ranty nature of this comment but I'm at my witts end with Gutenberg.

I just want basic hover/active effects on core buttons without completely ripping apart the ecosystem or overloading the styles statically in my own theme files. I did my due diligence before going on this soapbox. I am not the first person to be talking about this. I've seen similar issues dating back almost 2+ years at this point and I haven't found one solution that seems maintainable long-term.

glendaviesnz commented 2 years ago

@Jeremy-Bolella there is work being done for better support of hover/focus/active states for blocks. A PR was merged to allow adding of these to theme.json, and also some work on the supporting UI .

Jeremy-Bolella commented 2 years ago

@Jeremy-Bolella there is work being done for better support of hover/focus/active states for blocks. A PR was merged to allow adding of these to theme.json, and also some work on the supporting UI .

That's great and all but neither of those solutions fully address this situation. This is a bare minimum basic page builder functionality and the fact that Gutenberg has been around for 10+ major releases without something so basic and critical to any web development project is shocking.

My solution to my big rant above without causing myself a huge heartache/maintenance explosion in the future resulted in having to parse the css presets manually and tag my needed styles. It's just beyond frustrating that even page builders have more flexibility than what Gutenberg currently offers. I understood a lot of the decisions being made with all of my other issues I ran into and solved them.

This issue though is taking a ludicrous amount of code just so I can hook in hover styling in a dynamic way. It's disappointing but I'm really hoping this is the last major issue I run into trying to adapt my theme to run fully on Gutenberg.

At this point if I run into another quark that completely dead stops me development wise I'm dropping Gutenberg. I loved this system it made the whole contentfill process so much more seamless but it's not worth the frustration and loss of sanity just so I can give clients configurable blocks...

glendaviesnz commented 2 years ago

@Jeremy-Bolella, it would be great, if you have the time, to outline exactly what you see is needed to fix the problems you are experiencing with this, including details about the workarounds you have tried that haven't worked, over on the related issue.

Jeremy-Bolella commented 2 years ago

Absolutely once I get this wrapped up I'll jump over there with what my problem is and my fix. I'm hoping when the style engine is released this fix can just be rolled into that pretty seamlessly. Seems like the css parser route I'm taking to solve the hover/active issue is close to the idea around the style engine being developed.

ramonjd commented 2 years ago

I'm hoping when the style engine is released this fix can just be rolled into that pretty seamlessly. Seems like the css parser route I'm taking to solve the hover/active issue is close to the idea around the style engine being developed.

Hello! Just a note to say that I'm not sure the style engine alone will be the whole solution - I think the work going on with pseudo selectors mentioned above will determine how we support active/hover styles on elements such as the button.

So that's the area to watch for now.

Once that logic and design is in place, the role of the style engine (at least my understanding of it) will be to parse the style rules coming from Gutenberg and output the CSS.

🍺

djmtype commented 1 year ago

Applying !important as a blanket statement is an awful practice. By doing so you've increased specificity to the most extreme. When everything is important, nothing is important. After all, we have css @layer in all modern browsers today. Why is that not being utilized?

WraithKenny commented 1 year ago

We should remove all !important declarations from the codebase entirely, rather than making it opt-out. This is possible because we can now reduce specificity from problematic rules with :where() (rather than resorting to increasing specificity with !important overrides) and :where() has now officially been used in the codebase in other places. There's no reason left to not entirely remove !important

cbirdsong commented 1 year ago

@djmtype @layer is difficult for two reasons:

  1. Progressive enhancement is impossible because unlayered CSS always beats layered CSS, so using it would require a wholesale rethinking of all the CSS shipped in the editor to fit into layers.
  2. Because using layers requires you go all in, browser support needs to be exceptional, and as of this post @layer is only supported by around 90% of global browser traffic.

[^1]: and really the entire WP ecosystem

djmtype commented 1 year ago

@cbirdsong What about @WraithKenny's idea, using the :where selector?

cbirdsong commented 1 year ago

They do that a lot and I'm not a fan, though my reasons are getting less relevant as time passes. It is definitely a way to avoid !important, though.

michaelbourne commented 1 year ago

The biggest issue was that there was never a reasonable excuse to use !important in the first place, anywhere in the codebase. The problems compound over time; even today we still have !important used on basics like columns for breakpoints which provides no added functionality, support, or compatibility; it's just a hurdle for theme developers to overcome.

gyurmey2 commented 1 week ago

I also want to share the rationale for why we use this technique: we need to respect user choices at all times. There're blocks whose specificity is higher than 010, hence the presets classes attached by the user won't be applied if we don't use !important. See this thread for a longer review. If there's an alternative that fixes that issue and doesn't use !important I'd love to investigate it.

Hi @oandregal, considering the recent changes with the gradual reduction of CSS specificity, is it still necessary to use !important for helper classes?

This still creates a lot of problems. Here is one example:

Currently it is like this:

.has-luminous-dusk-gradient-background {
 background: var(--wp--preset--gradient--luminous-dusk) !important;
}

But when I try to add additional properties, they don't work:

.has-luminous-dusk-gradient-background.custom-class {
 background-attachment: fixed;
 background-attachment: fixed !important;
}

For example, couldn't the current state be replaced with this?

:root .has-luminous-dusk-gradient-background {
 background: var(--wp--preset--gradient--luminous-dusk);
}

Or this?

:root :is(.has-luminous-dusk-gradient-background) {
 background: var(--wp--preset--gradient--luminous-dusk);
}

In this case, additional properties will work.

:root .has-luminous-dusk-gradient-background.custom-class {
 background-attachment: fixed;
}

Any ideas?

cbirdsong commented 2 days ago

In that case, it could also be more specific and use background-color instead of stomping all over every background- property with the background: shorthand. Good blog post: https://csswizardry.com/2016/12/css-shorthand-syntax-considered-an-anti-pattern/

eric-michel commented 21 hours ago

I am becoming increasingly concerned that there is an attitude of "everything should be done in the editor" as Gutenberg continues to grow. I have seen comments from contributors that make it very clear that consideration of theme CSS is not a priority during development. And as Gutenberg continues to offer more controls for various styling features, so does its CSS footprint. Each update introduces new conflicts that developers have to fight, sometimes very unnecessarily (see https://github.com/WordPress/gutenberg/issues/60922).

Don't get me wrong, I absolutely love that Gutenberg creates an easy interface for creating complex content, but it can't be everything to everybody. There has to be room for theme customization. I know a lot of us have belabored this point, but it's a huge ongoing issue.