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

WordPress inline CSS doesn't respect editor-font-sizes #38252

Closed
billerickson opened this issue Jan 26, 2022 · 68 comments
Closed

WordPress inline CSS doesn't respect editor-font-sizes #38252

billerickson opened this issue Jan 26, 2022 · 68 comments
Labels
[Feature] UI Components Impacts or related to the UI component system [Type] Bug An existing feature does not function as intended

Comments

@billerickson
Copy link

Description

In WordPress 5.9, inline styles are added with !important, generated by theme.json (for a discussion disabling !important, see #37590). If a theme does not contain a theme.json file, it uses the theme.json file in WordPress core.

This issue occurs with the Gutenberg plugin active or inactive.

Previous versions of WordPress allowed themes to define the editor font sizes using add_theme_support( 'editor-font-sizes' ). These sizes are not being respected in WordPress 5.9.

In the backend, the typography selector shows the sizes specified by add_theme_support(), but the actual rendered font size uses the WP core defaults. In the screenshot below, the theme set "Large" to 21px, but WP core is overriding that with 36px.

If a theme does not use theme.json, the add_theme_support( 'editor-font-sizes' ) should be used when determining the values for those CSS variables.

Step-by-step reproduction instructions

  1. Install a theme that does not use theme.json
  2. In functions.php, define a "large" font size using add_theme_support: https://gist.github.com/billerickson/1111ac70585cf9ca7cf6c5bd306a2cd3
  3. In your theme's stylesheet, style the font size appropriately. Ex: .has-large-font-size { font-size: 21px; }
  4. When WP 5.9 is active, the large font style will appear as 36px

Screenshots, screen recording, code snippet

screenshot

Environment info

No response

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

@webmandesign
Copy link
Contributor

Hi,

I can confirm this. My theme is "classic" one (non-FSE) and I've noticed the new WP 5.9 is adding global-styles-inline-css styles into HTML head such as:
.has-large-font-size { font-size: var(--wp--preset--font-size--large) !important; }

This effectively overrides my theme styles as I also use "large" as one of custom block editor font sizes:

wp5 9-global-styles-overrides

Why this is even being outputted with classic theme?

@johnstonphilip johnstonphilip added the [Type] Bug An existing feature does not function as intended label Jan 26, 2022
@annezazu
Copy link
Contributor

@oandregal tagging you in here. This seems very similar to this previous issue that was resolved around font sizes
#37617 Would love if you can dig in here to see what might be going on/what needs to be cleared up.

@annezazu annezazu added the [Feature] UI Components Impacts or related to the UI component system label Jan 26, 2022
@cbirdsong
Copy link

My current workaround for this on legacy themes is to just get rid of the entire theme.json variable block via wp_dequeue_style( 'global-styles' );. However, that doesn't remove all the .wp-container- classes generated in the footer (#36135).

@luminuu
Copy link
Member

luminuu commented Jan 26, 2022

I can confirm this as well on a staging site.

@webmandesign
Copy link
Contributor

My current workaround for this on legacy themes is to just get rid of the entire theme.json variable block via wp_dequeue_style( 'global-styles' );.

Great workaround. As we are removing core functionality this way I'm just wondering whether it passes theme checks in Theme Check and Theme Sniffer plugins and WordPress theme review.

@webmandesign
Copy link
Contributor

The question for me stays: why is this even produced? Isn't this a theme territory?

@oandregal
Copy link
Member

oandregal commented Jan 27, 2022

👋 We've made some changes to the default presets coming from WordPress, see the section "Changes to the global stylesheet > Default font sizes, colors, and gradients" in the devnote.

The TLDR is that themes with theme.json can modify the default values directly. Themes that don't have a theme.json need to update their CSS to use the CSS Custom Properties to set the values, as in:

:root {
 --wp--preset--font-size--large: <NEW_VALUE>;
}

@webmandesign
Copy link
Contributor

@oandregal Thanks for explanation and for the link.

I already use CSS custom properties in my theme but they are custom ones, not WordPress ones. It seems contraproductive to basically repeat the code for the new WP CSS custom properties - it adds redundant code to my stylesheets. Could you please explain why this is produced by WordPress even for classic themes that does not use theme.json file?

Alternatively, can I provide empty theme.json file in my theme - would that get rid of that WordPress CSS code?
But then, wouldn't it enable FSE features as theme.json file is present in my classic theme (which is what I don't want)?

What happens if I dequeue the styles with above workaround? Would it break something in WordPress?

@webmandesign
Copy link
Contributor

Another thing is, WordPress is using body{ --wp--preset--...: ... } and this is being enqueued after my theme's stylesheet. This effectively overrides my theme's :root { --theme-css-variable: ...; --wp--preset--...: ... } declarations. I now have to use more specific CSS selector or/and !important.

Also, the font size classes are now produced twice: by theme in theme stylesheet and by WordPress in inline global styles. Why this happens when providing custom colors and font sizes was presented as a theme territory? WP5.9 doesn't seem to honor these theme declarations.

For years I produced CSS code with as minimal specificity as possible and with no !important. I never presumed I'd have to use opposite in 2021 onwards.

Is there a better place I can rase my concerns regarding global styles?

PS: I've tested creating theme.json file with basically no declarations. Doesn't help and it even messes up the editor layout that is working fine now in the theme.

@oandregal
Copy link
Member

oandregal commented Jan 27, 2022

Hey, I'm happy to expand on this. This is the issue we face: block & theme selectors can have higher specificity than user styles attached to blocks, for example:

/* block classes that themes use to set styles */
.wp-block-post-title h1 { /* this has higher specificity than the preset class */
  color: green;
}

/*
 * Preset class that is only attached to blocks by the user,
 * otherwise they're not present. If they're attached,
 * they should override any other rule the theme has set.
 */
.has-red-color {
  color: red;
}

User styles should prevail at all times over block & theme styles. We considered alternatives but none were satisfactory. It was also discussed in this thread at length. We're all for iterating if there're alternatives to the issue we face.

Note that default presets have been enqueued by WordPress since the block editor was launched. In 5.9, we moved them from an embedded stylesheet (wp-block-library-css) to the global styles one (global-styles-inline-css). See #34510 and #35182 In terms of default presets, WordPress 5.9 ships the same CSS as before, just in a different place. The reason default classes have been enqueued since the beginning is that they can be used by users: in existing content, used in patterns by themes, or now directly through the UI color controls that show theme & default colors.

@cbirdsong
Copy link

In terms of default presets, WordPress 5.9 ships the same CSS as before, just in a different place.

Use of !important means this just isn't the case. Previously, I could override .has-font-size-large with responsive sizes and now Wordpress overrides it. (#34575)

@johnstonphilip
Copy link
Contributor

It would seem to me that if a theme doesn't have a theme.json file, than no theme.json things should be rendered on the front-end at all. Does anyone disagree with that? If so, why?

JiveDig pushed a commit to maithemewp/mai-engine that referenced this issue Jan 27, 2022
@mwhiteley16
Copy link

I would agree.

To go further on this specific issue (as noted by Bill in the initial issue), if there is no theme.json AND add_theme_support( 'editor-font-sizes' ) is being used, that should be respected, not overridden.

Seems pretty logical and straight forward.

@billerickson
Copy link
Author

@oandregal For themes that don't have a theme.json file, why don't the font sizes defined in add_theme_support( 'editor-font-sizes' ); take precedence over the default font sizes in WP core?

@webmandesign
Copy link
Contributor

webmandesign commented Jan 27, 2022

@oandregal But isn't this a theme territory anyway? If the theme overrides WordPress styles, maybe it is doing on purpose. WordPress should not force its own styles in my opinion.

In my opinion, the example you've provided seems to be an insufficient design on the theme's side. I personally deal with such things this way in my themes:

/* block classes that themes use to set styles */
/* this way even user modified color via editor should work without any !important or higher specificity classes */
.wp-block-post-title h1:not(.has-text-color) {
  color: green;
}

/*
 * Preset class that is only attached to blocks by the user,
 * otherwise they're not present. If they're attached,
 * they should override any other rule the theme has set.
 */
.has-red-color {
  color: red;
}

User styles should prevail at all times over block & theme styles.

I 100% agree. But it should not be forced by WordPress by overriding theme declared styles and compatibility with block editor provided using recommended ways.

Note that default presets have been enqueued by WordPress since the block editor was launched.

That is perfectly fine. But I've never had issues with WordPress overriding my add_theme_support() theme styles previously and I build Gutenberg compatible themes for 2+ years.

It's funny that this seems to affect only themes that are actually compatible with Gutenberg editor (but are not block themes/FSE themes). Themes that are not compatible with Gutenberg or are block themes, they seem to work perfectly fine also with this WordPress change. I'm just sad that the themes that advocate for using Gutenberg editor are affected the most. (Which only still convinces users to use alternative editors, unfortunately...)

The reason default classes have been enqueued since the beginning is that they can be used by users: in existing content, used in patterns by themes, or now directly through the UI color controls that show theme & default colors.

I understand that. But such case can also occur when user switches between 2 themes that are compatible with Gutenberg. And in that case if a content was built with color and font presets of the first theme it may no longer work when the second theme is activated. There is no naming convention in place for such custom theme presets, which causes issues. I don't think there is a solution for this now. But I understand that WordPress keeps default values for these in the code.

It's just that I don't think WordPress should force its own default styles on themes. Especially those ones that uses the same custom font size labels but different actual font size values (for example). No default WordPress style should be outputted with such high specificity and !important declaration.

Tip: This is not related, but I've dealt with colors in block patterns too. If I may advice someone who could benefit, rather use custom color values and not preset colors in block patterns. That way the patterns have higher chance to look the same even if the color patterns are changed some "wildish" way.

My suggestions:

I understand that WordPress has to keep the styles for backwards compatibility. But could this be done with non-obtrusive way like it was until now? It would be beneficial to lower priority and specificity of the global styles, such as:

  • enqueue the global styles as early as possible, maybe even with negative priority: add_action( 'wp_enqueue_scripts', 'wp_enqueue_global_styles', -100 ); That way any theme and plugin styles hooked with default priority of 10 may override the global CSS with the same CSS selectors at least.
  • lower CSS selector specificity from body { ... } to :root { ... } in global styles code.
  • do not use !important in the global styles declarations. That way if there is a style declaration for existing WordPress class in the theme, the theme styles get precedence.
  • and finally, if none of the above is possible (or even if it is), provide hook to filter the output of wp_get_global_stylesheet() function. That way themes can control output of these styles themselves too.

@mkkeck
Copy link

mkkeck commented Jan 28, 2022

Dear Wordpress and Gutenberg developers,

does anybody of you know, that code duplication specially in CSS styles could lead in bad page speed and bad web vitals?

Styles, and of course other things like SVG, which are loaded or inlined but not used, increase the time and filesize to load, parse and display the above fold content. Running tests with Lighthouse and Webhints shows me longer tasks for recalculating and rendering the layout. Of course, this may be not essential, but such every little thing added, decrease pagespeed perfomance.

PLEASE: if a user, I'm not talking about non professionells - which are installing WordPress in 5 minutes and than only use it for simple blogging, not need your "global / core" stuff, give them an opt-out (I prefer opt-ins).
If anybody uses a custom made theme, I personally think, he/she/it knows what he/she/it does. So why do you force and dictate them how and why using your "global / core" stuff and that they have to accept, many junk is added to rendered pages?

The other point is: Are you really testing before release? Perhaps that could be done better.
Thats why there are many junk inline styles and SVG added before closing the body and some essentials things like responsive font sizes not working without terrible hacks.

PLEASE: Add all this in your own Twenty-Themes, but not global to all other themes out there, without an explizit opt-in.

I'm really frustrated, that since WordPress version 5 nearly every minor and major update breaks my (front) pages and/or leads in bad pagespeed performance and web vitals.

@tommusrhodus
Copy link

The use of !important is definitely a huge issue here, another is that without declaring add_theme_support( 'experimental-link-color' ), font sizes and colour styles created through add_theme_support do not override the WP Presets if they share the same name, e.g large or black

I have detailed this issue in https://core.trac.wordpress.org/ticket/54954 and have a PR to remove the dependancy of experimental-link-color at WordPress/wordpress-develop/pull/2236

Whilst that PR does not resolve issues caused by the use of !important, it does at least correctly merge your defined style choices with the WP defaults before they get set as important.

@webmandesign
Copy link
Contributor

webmandesign commented Jan 28, 2022

Could you provide hook to filter global styles please?

The more I think about this, the more I incline to my last suggestion about filtering wp_get_global_stylesheet(). That way WP5.9 code could stay the same while theme authors could manipulate global styles code to their needs in a specific theme that requires such manipulation.

(Though still, no !important should be used :) )

@paulgibbs
Copy link

In terms of default presets, WordPress 5.9 ships the same CSS as before, just in a different place.

Use of !important means this just isn't the case. Previously, I could override .has-font-size-large with responsive sizes and now Wordpress overrides it. (#34575)

As @cbirdsong wrote, the change with !important makes the original statement here inaccurate, and we've noticed it on the first few of our clients' sites that we've updated. I think we're going to rollback to WP 5.8.x as we have hundreds of existing client sites which will be affected. This ought to be considered a breaking change / concern because it impacts our sites that do not have a theme.json and are classic theme.

I don't follow Gutenberg development closely enough to know whether the best fix is to merge the styles correctly, or drop the !important as a quick fix, but something needs to happen for WP 5.9.1 please.

@webmandesign
Copy link
Contributor

webmandesign commented Jan 28, 2022

Hey guys,

I've got a temporary fix for now (would be great if we can filter the global styles output though).

Here is PHP7+ code that should go into your (child) theme's functions.php file:

/**
 * Lowering specificity of WordPress global styles.
 */
add_action( 'init', function() {

	// WP5.9+ only.
	if ( ! function_exists( 'wp_get_global_stylesheet' ) ) {
		return;
	}

	// Dequeue original WP global styles.
	remove_action( 'wp_enqueue_scripts', 'wp_enqueue_global_styles' );

	// Enqueue WP global styles early.
	add_action( 'wp_enqueue_scripts', function() {

		// Lower CSS code specificity.
		$stylesheet = str_replace( [ 'body', '!important' ], [ ':root', '' ], wp_get_global_stylesheet() );

		if ( empty( $stylesheet ) ) {
			return;
		}

		wp_register_style( 'wp-global-styles', false );
		wp_add_inline_style( 'wp-global-styles', $stylesheet );
		wp_enqueue_style( 'wp-global-styles' );
	}, 0 );

	// Treat also editor styles.
	add_filter( 'block_editor_settings_all', function( $editor_settings ) {

		// Lower CSS code specificity.
		$editor_settings['styles'] = array_map( function( $style ) {
			if ( ! empty( $style['css'] ) ) {
				$style['css'] = str_replace( [ 'body', '!important' ], [ ':root', '' ], $style['css'] );
			}
			return $style;
		}, $editor_settings['styles'] );

		return $editor_settings;
	} );
} );

The code notes describe what's going on in there.


👉 This code was ported into a plugin Global Styles Mods.

@mkkeck
Copy link

mkkeck commented Jan 28, 2022

Hey guys,

I've got a temporary fix for now (would be great if we can filter the global styles output though).

Here is PHP7+ code that should go into your (child) theme's functions.php file:

Nice :)

I've also a fix for the functions.php to remove the wp-container- styles produced by blockGap and the duotone-styles:

remove_filter('render_block', 'wp_render_duotone_support');
remove_filter('render_block', 'wp_restore_group_inner_container');
remove_filter('render_block', 'wp_render_layout_support_flag');

But the junk SVG can not be removed at the moment, missing an action/ filter to remove them. The problem is:

// file: wp-includes//block-supports/duotone.php; 
// line: #459 ... 467
//       produces the junk SVG, even if duot0ne is not used
add_action(
  // Safari doesn't render SVG filters defined in data URIs,
  // and SVG filters won't render in the head of a document,
  // so the next best place to put the SVG is in the footer.
  is_admin() ? 'admin_footer' : 'wp_footer',
    function () use ( $svg ) {
      echo $svg;
    }
);

@pattyok
Copy link

pattyok commented Jan 31, 2022

Hey guys,

I've got a temporary fix for now (would be great if we can filter the global styles output though).

Here is PHP7+ code that should go into your (child) theme's functions.php file:

Thanks for this!

This fixes the output on the front end. But still having issues trying to fix the view in the editor. The WordPress variable are set at .editor-styles-wrapper level...

.editor-styles-wrapper {
     ...
     --wp--preset--font-size--small: 13px;
    --wp--preset--font-size--medium: 20px;
    --wp--preset--font-size--large: 36px;
    --wp--preset--font-size--x-large: 42px;
}

Making it another challenge to override.

@webmandesign
Copy link
Contributor

Hi @pattyok, I've updated the code in my reply above so it includes fix for editor too.

@maddisondesigns
Copy link

This issue still wasn't fixed in 5.9.1

@oandregal
Copy link
Member

oandregal commented Feb 28, 2022

The fix for this was ported to WordPress at WordPress/wordpress-develop#2233 and it's part of 5.9.1 (core ticket).

@WestCoastJitterbugs
Copy link

The fix for this was ported to WordPress at WordPress/wordpress-develop#2233 and it's part of 5.9.1 (core ticket).

Looks like that PR was closed without being merged? Or am I misunderstanding?

@oandregal
Copy link
Member

Looks like that PR was closed without being merged? Or am I misunderstanding?

It's the usual way core merges work: the GitHub PR is closed and it's committed via SVN. This is the commit in SVN trunk and 5.9.1.

@dgmstuart
Copy link

Looks like that PR was closed without being merged? Or am I misunderstanding?

It's the usual way core merges work: the GitHub PR is closed and it's committed via SVN. This is the commit in SVN trunk and 5.9.1.

Ah OK - thank you for explaining.

@maddisondesigns
Copy link

@oandregal Are you saying that this was fixed in 5.9.1? If so, then you need to look at it again, as it definitely wasn't fixed. The issue still remains because you're overriding theme styles by your use of !important. Until you get rid of !important the issue is going to remain.

@mkkeck
Copy link

mkkeck commented Mar 1, 2022

@oandregal
Hi André,

why not putting the whole stuff, wich can break third party themes, away from the WordPress core to the WordPress own themes?

Also, as long there is no trivial and simplified css style class naming convention for default styling, it does not make sense to force using and mergin default styles. As an example look at the class names for colors: has-light-blue-color.
I would suggest to use instead colors names some generic names like has-color-primary, has-color-info etc. Many css frameworks using such generic names, which makes easier to adjust user settings and provide fallbacks for missing.

Yes, if an editor or author is setting values to colors, font sizes or styles this should the highest priority.
But the styles from core should have the lowest priority and not overwrite theme styles.

@jonschr
Copy link

jonschr commented Mar 8, 2022

This doesn't appear to have been fixed in 5.9.1, @oandregal. Am I and the other commenters since that release missing something here? All of the core issues appear to remain: users who appropriately used add_theme_support( 'editor-font-sizes' ) are seeing those font sizes overridden by new !important styles from core.

Using the workaround above for now, and I'm definitely going to move my workflow over to a theme.json one in the near future, but my problem here is that lots of existing sites get broken in small ways by this change.

If the theme uses add_theme_support( 'editor-font-sizes' ) and no theme.json style, then at minimum the new inline styles should respect the sizes defined there.

@webmandesign
Copy link
Contributor

@jonschr Unfortunately, I personally don't see this being fixed in near future. For this case I've created a plugin fixing the issue. It may help you too. https://wordpress.org/plugins/global-styles-mods/

@jonschr
Copy link

jonschr commented Mar 8, 2022

@webmandesign I'm using it (thanks so much!) on some older sites already. I HATE the idea of installing something like this on sites that are in development and which use add_editor_styles but don't yet use theme.json. Unfortunately, I have a few helper plugins that need updated as well, as they also access data in the editor_styles but don't have a mechanism (yet) to access the corresponding data in theme.json.

At core, the problem is that we were given a "right" way to do things, and the theme.json stuff (which is great, BTW), allows WordPress core defaults to override theme-defined styles that were correctly done only a month ago. This might speed up adoption, but at the expense client sites breaking until/unless they're updated.

@webmandesign
Copy link
Contributor

@jonschr I feel your pain, buddy. It's sad that Gutenberg editor ready (not FSE) themes were affected with WP5.9. That's why nobody seems to be talking about it outside of this GitHub repo. It almost looks like "nobody" was using Gutenberg as editor except us here...

I haven't played with theme.json much, but from what I understand it seems like it is supposed to replace CSS, which in my opinion will produce many issues for real world websites. Certainly it is no where near the (design) functionality of current themes, but I'm optimistic it will mature as block editor had.

@glendaviesnz
Copy link
Contributor

@billerickson there are a few places where the problems with the use of ! important have been noted. In order to try and focus discussion around potential solutions in one place, what do you think about closing this issue and making reference to it over on this issue?

@jonschr
Copy link

jonschr commented Mar 11, 2022

@glendaviesnz I don't want to speak out of turn – however, it's probably good to note that the !important declarations here aren't the only/primary issue @billerickson noted.

The primary issue here is that core defaults aren't respecting theme-declared defaults.

While it's true that !important exacerbates this issue, at the end of the day much of the problem we're experiencing would be addressed if the values being output themselves were inheriting the theme-declared values from add_theme_support( 'editor-font-sizes' ) rather than inhering core defaults (still would probably be breaking things on mobile, though, because of the usage of !important).

@glendaviesnz
Copy link
Contributor

Thanks for the extra detail @jonschr. I thought that the issue of inheriting the core defaults had been fixed by this PR, which was released in 5.9.1, but I may be misunderstanding something there.

@jonschr
Copy link

jonschr commented Mar 14, 2022

My apologies.

I just rechecked this against 5.9.2, and @glendaviesnz is completely correct on this count. I was looking at mobile, which is a whole different can of worms (and the reason I'm still getting support requests around this topic, as my clamp-based methodology no longer works, but that's because of !important and not because of theme inheritance).

The core defaults do appear to be inherited properly now from add_theme_support( 'editor-font-sizes'). Thanks for following up. The remaining issue here, as you said, is now tied purely to the !important declaration.

@glendaviesnz
Copy link
Contributor

Going to go ahead and close this, and the remaining issues with the ! important declaration can be followed up here.

Feel free to re-open @billerickson if you think there are still other things on this issue other than ! important that haven't been covered.

@maddisondesigns
Copy link

@glendaviesnz The main solution suggested within #37590 is to add a filter in to a theme.json file. One of the main issues with this use of !important is that it's affecting themes that don't have a theme.json file, which has been mentioned multiple times here. We shouldn't have to add a theme.json file to our themes, to fix an issue that was introduced by core. In all honesty, we shouldn't even have to use a filter to fix this issue. Why is it, once again, up to theme developers to modify their themes to fix issues introduced by Gutenberg development. This happens over and over and over again!

@jnicol
Copy link

jnicol commented Mar 16, 2022

We shouldn't have to add a theme.json file to our themes

This. The issue affects non FSE themes, with no theme.json file. It's been mentioned several times, in several different ways, but the message doesn't seem to be getting across. For agencies/developers who maintain dozens of legacy WP sites, it's a disheartening prospect to have to go back and modify each site to workaround !important declarations. ("Legacy" in this case means anything pre 5.9).

@webmandesign
Copy link
Contributor

Hey guys, I personally experience issues only with themes that does not include theme.json but support block editor (such as font sizes, color palettes, wide alignment).

I don't experience issues with more "legacy" themes that does not support block editor, for example. Do you?

So, from my point of view only legacy themes that provide support for block editor are afected. The themes with no support declaration for block editor should be fine. Can you confirm this?

@glendaviesnz
Copy link
Contributor

glendaviesnz commented Mar 22, 2022

@glendaviesnz The main solution suggested within #37590 is to add a filter in to a theme.json file. One of the main issues with this use of !important is that it's affecting themes that don't have a theme.json file, which has been mentioned multiple times here.

Sorry for the delay in replying. Although the main suggestion in #37590 is to add a toggle in the theme.json, or a filter, that is only a 'suggestion' at this point, hopefully a number of solutions can be compared against the feedback on this issue, and over on that issue before a decision is made on an actual solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] UI Components Impacts or related to the UI component system [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

No branches or pull requests