Global Styles: Output defaults from theme.json for classic themes #43981
Conversation
scruffian
added
the
Global Styles
There was a problem hiding this comment.
Oh, what an interesting problem. This appears to be working pretty well for me in the site's front end. I thought I ran into a style output issue with fallback gap, but it turns out it was an unrelated regression. I've opened up merged a fix for the issue I ran into here: #44001
When it comes to the post editor, though, it appears that the styles are not being output there, as far as I can tell. I think there might be two parts to that problem:
- When we output the block editor settings, there's also a check for whether it's a blocks-theme around this line:
As a result,
stylesis currently only output in the editor if it's a blocks-based theme. However, - If we decide to output
styleshere, too, then in the editor the default editor styles will be skipped around this line (resulting in text, fonts, etc losing the default editor styles and reverting to browser defaults):gutenberg/packages/edit-post/src/editor.js
Line 176 in f26adef
The base-layout-styles output gets around this by being output as a separate __unstableType on this line (so it gets bundled with the presets in the editor's styles):
Other than that, is there any situation where Classic themes would prefer not to have this style output? From memory, one of the reasons we leaned toward using base-layout-styles as a separate type for Classic themes was because the styles object can't be easily overridden by Classic themes (I think there was some concern that it might be hard for themes to get rid of unwanted styles).
If it's okay for Classic themes to have these rules, though, I think a potential fix for the above issues might be to set a different __unstableType for styles when it's a classic theme, on this line:
base-block-styles?
|
@andrewserong Thanks for looking into this, your insights are very valuable. I tried a different approach, which again attempts to simplify things. I switched some of the logic which means that the default editor styles are now output on all themes, not only block themes. This means the following CSS will be added to block themes in the editor: What do you think? |
|
Size Change: +744 B (0%) Total Size: 1.25 MB
ℹ️ View Unchanged
|
Hrm... this is an interesting one 🤔. Personally, I don't think block themes should get this hard-coded list of CSS rules, since the base styles for font, font-size, line-height etc should be provided by either the base From my perspective the default editor styles are most useful for legacy Classic themes that have not been updated to support the block editor, so that the editing experience can be pleasing, even if the theme hasn't been designed for use with the block editor. |
scruffian
force-pushed
the
fix/elements-in-classic-themes
branch
from
635e3e2 to
5a34c89
Compare
|
@andrewserong I see, it's more complicated than I realised. I've pushed up a different and simpler approach, which seems to work. In essence, we always output the core theme.json defaults on the frontend, but in the editor we still only use the default editor styles for classic themes. Thanks for all your help with this. |
There was a problem hiding this comment.
Thanks for following up @scruffian! There's still a couple of issues with this approach as it outputs styles multiple times. It's a pretty tricky one this one, but I think there could be a path forward that still does what you're intending to do here. Apologies for the messy comments!
In principle, I'm wondering if we can get this working without duplicate style output if we:
- In
get_stylesheetin the file changed in this PR, preserve thein_array( 'styles' ...check, but remove the block forbase-layout-styles - In
get-global-styles-and-settings.php, remove the! $supports_theme_jsoncheck from theempty( themes )check and output'variables', 'styles', 'presets'for both classic and blocks based themes - In
block-editor-settings.phpupdate thebase-layout-stylesblock to usestylesas the value for the'css'key (so that we're requesting styles instead of the base layout styles), but set the__unstableTypeto something like'base-styles'(really, anything other thanthemeso that the block editor doesn't treatstylesas theme styles for classic themes)
I haven't tried the above, but 🤞 it gets us closer to it.
Happy to do more testing on Monday!
| if ( in_array( 'variables', $types, true ) ) { | ||
| $stylesheet .= $this->get_css_variables( $setting_nodes, $origins ); | ||
| } | ||
|
|
||
| if ( in_array( 'styles', $types, true ) ) { |
There was a problem hiding this comment.
Is the reason that this works because it means the styles now get output as part of the part of presets and variables? It appears that this results in the rules being output multiple times in the post editor:
I think this is occurring around here each time we call gutenberg_get_global_stylesheet:
Unfortunately this change seems to mean that the public API of wp_get_global_stylesheet when passed array( 'variables' ) will unexpectedly also include styles. Instead of updating this check, should we update things one level up in gutenberg_get_global_stylesheet?
If it turns out we no longer need base-layout-styles, then we can still potentially remove that block below.
There was a problem hiding this comment.
I believe the fix might be that in gutenberg_get_global_stylesheet we'd remove the check on this line
base-layout-styles):
$tree = WP_Theme_JSON_Resolver_Gutenberg::get_merged_data();
$supports_theme_json = WP_Theme_JSON_Resolver_Gutenberg::theme_has_support();
if ( empty( $types ) ) {
$types = array( 'variables', 'styles', 'presets' );
}I haven't tried that out yet, but I believe basically we'd be removing any reference to 'base-layout-styles in the repo.
Then, over in block-editor-settings.php, we'd need to ensure that we still request styles for classic themes, but that they would not be set to the __unstableType of theme (so that they're not considered part of the theme styles).
So, in this else statement, instead of using base-layout-styles, we might use something like the following here:
$block_classes = array(
'css' => 'styles',
'__unstableType' => 'base-styles', // setting this to something other than `theme` means that the editor will not consider these styles as part of "theme" styles.
'isGlobalStyles' => true,
);
5a34c89 to
ebbfcc6
Compare
|
@andrewserong Thank you that seems to solve it. I've tested both classic and block themes and neither have duplicate styles, but the buttons look correct in both. |
|
Thanks @scruffian, this definitely feels closer! The styles appears to be output in the correct places, without the duplication in the earlier commit. Unfortunately, it looks like these base styles wind up having a higher specificity than desired in Classic themes. Here's how it's looking in TwentyTwenty in the editor:
One of the things that works nicely in blocks themes is that the The issue isn't as noticeable in some classic themes like TwentyNineteen, because in that theme the Button styles happen to have a higher specificity, but I imagine we probably can't rely on that to be the case for most Classic themes?
Just pinging a couple of other folks who were involved in the earlier PR (#34180) that moved the styles out of the Button block's styling and who've also looked at global styles (@oandregal, @getdave and @jasmussen) in case they have ideas on how this should work, too 🙂 |
|
I wrapped the button selectors in a Frontend: Editor: |
|
I made an empty classic theme, basically an underscores fork without the CSS and it was working as intended. The frontend and the editor looks mostly the same (there's GB styles that are only applying to the editor but are unrelated to this issue or PR) Frontend: Editor: I think |
|
Should we have tests for this? |
|
👋 I can look at this issue deeply tomorrow morning. I need to familiarize myself with what While reading this, I wondered if we could simplify the current code by hooking into the existing The PR is not working. It's a prototype to help explain my point. Please, feel free to improve it. I thought I'd share early for timezones to work to our advantage and not the contrary. |
There was a problem hiding this comment.
Thanks so much for your perseverance here @scruffian! I was about to approve this one, but then went to double check some of the reports in the linked issue from wp-calypso and for themes like Sequential (mentioned in this comment: Automattic/wp-calypso#64917 (comment)) it looks like it does need the higher specificity in order for the padding to be applied:
Sequential theme with :where() rule |
Sequential theme without :where rule |
|---|---|
 |
 |
This one has me a little stumped as in principle I like the idea of these base / reset-like rules having low specificity, but it looks like for the Button block's styling to work properly in some of these Classic themes, it needs slightly higher specificity in order to win out over a theme's reset rules. Here's the culprit in Sequential theme, where there's a reset on a that sets padding to 0, which wins out over the :where() rule:
😭😭😭😭
So, I'm wondering, where does that leave us for finding a sweet spot for this one? 🤔 It feels like it needs a particular specificity that might be hard to define — one that's higher than base resets, but lower than a theme intentionally making changes to the block styles, like TwentyTwenty.
On the one hand, I like the low specificity in this PR and it does make sense to me that themes would need to ensure that they play nicely with Buttons blocks. But on the other, from a user perspective, if you install a theme from the directory, the Buttons block should "just work".
So far, of each of the iterations, I think maybe the one just before, where we encountered issues with TwentyTwenty in the block editor is the closest to fixing the issue at hand. The trade-off might be that it would be up to themes to ensure that they have a higher specificity than the Buttons block.
Sorry I can't think of a clear-cut fix for this one!
|
Another option is to keep these rules but also add the CSS back to the block... |
Good point. That might wind up being the simplest approach for now 🤔 |
| @@ -84,9 +84,7 @@ function gutenberg_get_global_stylesheet( $types = array() ) { | |||
| } | |||
| $tree = WP_Theme_JSON_Resolver_Gutenberg::get_merged_data(); | |||
| $supports_theme_json = WP_Theme_JSON_Resolver_Gutenberg::theme_has_support(); | |||
| if ( empty( $types ) && ! $supports_theme_json ) { | |||
There was a problem hiding this comment.
I've noticed we've already done this for classic themes here (6.0 code): #42012 It wasn't backported to core. I suggest we revisit that code once this conversation settles.
We've tried to avoid classic themes to have any unwanted styles so far. If we go ahead with this, every time we add something to the default theme.json it'll be shipped to classic themes. I have strong reservations against doing this because it's a source of bugs for classic themes (nobody will check how a change in the default theme.json will affect them in the future).
|
Hey, I've been checking the evolution of styles for buttons in this cycle. It's been a lot of back and forth so I may have missed something. Please, correct me if this is not what happened:
There was a lot more of back and forth, but this should be the essential steps. As a consequence, the styles for the button block that were shipped as part of the block library stylesheet are not longer shipped because we ignore Some notes:
|
|
I think this approach is always going to need some intervention from some themes. I tried a different approach in #44334. |
One issue we are dealing with is trying to ensure that themes can have consistent styling for buttons. This is the role of the button element vs the button block. In order to maintain the current visual appearance of buttons we need to define some styles, but if we define them in the block then they will have a higher specificity than the styles that users define for the button element - so to override them users would need to define styles for both button elements and button blocks, which is very inconvenient and would likely result in inconsistencies on people's sites as the might only remember to update the styles for one, not both. |
Do you mean the |
Oh, I see, we do this is a different place: gutenberg_add_global_styles_for_blocks. The way we do it now is problematic. It's unguarded, it does not consider what origin we want to render depending on context. Try this:
The concern I shared here for elements is demonstrated with how block styles work at the moment: the classic themes get unwanted styles. |
I want to rephrase this in light of my comment above because it may be not clear what I meant. I think we should always load the styles for the |
|
The Gutenberg plugin no longer supports WordPress 6.1, but I just wanted to check if this PR is still valid. |
@t-hamano We can reopen this if necessary. :) |
What?
In classic themes we don't output the styles for elements, which we rely on for button styles. This adds those rules to the global styles output for all themes so that buttons retain their styles.
Fixes Automattic/wp-calypso#64917
Why?
We need buttons to work for classic themes as well as block themes.
How?
Previously we weren't outputting anything defined under
stylesfor themes that don't support theme.json. However since in this case we already set the origins of the output to$origins = array( 'default' );, we don't need to worry about outputting the default rules from the core theme.json file.For classic themes this means the following changes. This CSS is added:
Testing Instructions