Editor defaults for themes with no editor styles and when user disables theme styles #35736
Conversation
|
Size Change: +43 B (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
oandregal
added
[Type] Bug
|
Thanks for the followup. What's a good way to test? And do you personally prefer this PR or #35737? |
packages/edit-post/src/editor.js
Outdated
| // settings.styles can have styles that core adds. | ||
| // We don't want these to count as "theme styles". | ||
| const filteredStyles = settings.styles?.length | ||
| ? settings.styles.filter( ( style ) => ! style.toSkip ) |
There was a problem hiding this comment.
So we're excluding these styles from the styles passed to the block editor. Why are we adding them in the backend in the first place then?
There was a problem hiding this comment.
Oh I think I understand now: "preset styles" are included in settings.styles and we only want to check for the styles that comes from the theme.json styles or custom editor styles here.
I wonder if we should maybe expand on the __unstableType instead and clarify what each stylesheet is about, the "theme", "core" existing types don't seem to be enough anymore.
we need more granular things:
- "core": Any core style that is loaded for all themes
- "theme presets"
- "theme styles" (theme.json or add_editor_styles calls)
It also seems like "theme presets" should also be included in the fallback in addition to settings.defaultEditorStyles to ensure the presets do work even if the user chooses to not user "theme styles" in its editor.
There was a problem hiding this comment.
I saw your comment on the other thread and now understand this better. Will continue to iterate on this PR.
There was a problem hiding this comment.
So, essentially we want the ability to remove theme styles in the post editor, for which they need to be enqueued separately. We need this:
- theme.json styles (core)
- editor styles provided via add_editor_styles (theme)
- theme.json styles (theme)
- theme.json styles (user)
- preset classes (core, theme, user)
- preset css vars for the sidebar, are not processed by .editor-styles-wrapper (core, theme, user)
So when the user disables "theme styles" we recalculate styles without 2 & 3. Perhaps we also skip 1 as well? Does this look correct to you?
There was a problem hiding this comment.
This use case is new for me, and it's making me reconsider how we do some things.: would the styles for editors be simpler if we let them access the raw data so they can calculate the stylesheets themselves?
In addition to what I've shared for the post editor above (the 6 stylesheet plus the __experimentalSettings as well), this is what we currently send to the site editor:
__experimentalGlobalStylesUserEntityIdfor theme.json user data__experimentalGlobalStylesBaseConfigwith theme.json data (core+theme)__experimentalSettingstheme.json settings (core+theme+user)- editor styles provided via add_editor_styles
What if we just sent these four pieces of data instead:
- theme.json core
- theme.json theme
- theme.json user
- editor styles provided via add_editor_styles
There was a problem hiding this comment.
Based on the list above, I don't see why we should split 1, 2, and 4? It's fine if these are combined. and on the list you shared, the list from 1 to 4 are the things we want to exclude from the post editor if the user chooses to. So basically we only need to split presets and styles.
There was a problem hiding this comment.
Don't we want to keep user styles (4) in that case?
There was a problem hiding this comment.
No, I don't think so, for me 4 is just the equivalent of custom CSS editor styles and when toggling the "theme styles", the user expect the same design across all themes.
Initially, I thought #35737 was a better approach but after some clarifications of the use cases to cover, I've closed that one and will focus on this direction instead. It needs a few tweaks, I'll ping you again when it's ready. |
oandregal
force-pushed
the
fix/themes-with-no-editor-styles
branch
from
18cdd2e
to
cdf2e6c
Compare
| @@ -995,19 +995,6 @@ private function get_block_classes( $style_nodes ) { | |||
| $selector_duotone = self::scope_selector( $metadata['selector'], $metadata['duotone'] ); | |||
| $block_rules .= self::to_ruleset( $selector_duotone, $declarations_duotone ); | |||
| } | |||
|
|
|||
| if ( self::ROOT_BLOCK_SELECTOR === $selector ) { | |||
There was a problem hiding this comment.
This is going to make the existing tests for styles fail. I need to fix them.
There was a problem hiding this comment.
Why did you have to remove this? This is not just for editor styles, it's also important for the frontend.
There was a problem hiding this comment.
These styles are part of "block styles", hence, in the client, we can't distinguish the empty state (a theme with no "block classes").
There was a problem hiding this comment.
Note that these weren't removed, but moved to another place lib/compat/wordpress-5.9/spacing-styles.php
There was a problem hiding this comment.
These styles are part of "block styles", hence, in the client, we can't distinguish the empty state (a theme with no "block classes").
Is a theme with theme.json ever a theme without editor styles? IMO no, so I think it's fine to consider them theme editor styles.
There was a problem hiding this comment.
The behavior of using the default editor styles in themes is specially tailored toward old themes that don't have any Gutenberg support (which is not the case for themes using theme.json)
There was a problem hiding this comment.
I was able to make this work properly while retaining those styles working as they are (commit).
After what you said, I realized that those spacing styles should not be enqueued for themes without theme.json, neither in the editor or the frontend. While that is true for the frontend, we do enqueue the spacing styles in the editor in trunk. So this is a bug that we're also fixing with this PR (here).
|
@youknowriad what are your thoughts on the current direction? The main thing is that the "spacing styles" (align, block gap) can't be part of the block styles generated as they are today: we need the block styles to be empty for themes that don't provide any editor styles. |
oandregal
changed the title
oandregal
force-pushed
the
fix/themes-with-no-editor-styles
branch
from
06c57fc
to
d50af2b
Compare
|
@jasmussen @chad1008 @youknowriad I've pushed all the changes and tested this across a number of themes. Left also test instructions in the issue description. It seems to work fine in my testing. |
|
This tests well for me across different themes, with and without theme styles active. Thanks @oandregal! |
| * | ||
| * @return string Stylesheet. | ||
| */ | ||
| function gutenberg_experimental_global_styles_get_stylesheet( $tree, $type = null ) { | ||
| // Check if we can use cached. | ||
| $can_use_cached = ( |
There was a problem hiding this comment.
The cache in this method is dead code, it's never used, so we can remove it.
|
This appears to generally be working for me. One thing I'm noticing, is that the theme colors are not used here, these are the default WordPress blues: Matías encountered the same issue in #35662 ("Disabling theme editor styles in preferences makes some elements in the page load with default colors and not the current color scheme."), so it may be a separate issue. But at the same time, I see this in trunk: that is — the default styles with fonts and such are incorrect, but the colors are correct. |
|
@jasmussen I'm struggling to see what the issue is. Tested with the TT1-blocks them in a few branches (including v11.6, where the issue this PR fixes was not happening) and they all look the same to me. Tried in Firefox and Chrome with no differences. With theme styles:
No theme styles:
|
|
Go to your user profile and choose an admin color scheme other than the default one: Now observe that the color of the primary button remains |
Oh, thanks for the instructions. I've checked v11.5, v11.6, |
| * | ||
| * @return string Stylesheet. | ||
| */ | ||
| public function get_stylesheet( $type = 'all', $origins = self::VALID_ORIGINS ) { | ||
| public function get_stylesheet( $types = array( 'css_variables', 'block_classes', 'preset_classes' ), $origins = self::VALID_ORIGINS ) { |
There was a problem hiding this comment.
I wonder if css_variables should be a type. I mean we don't really know where these variables are used and it seems we should always render them anyway regardless of what types are passed.
There was a problem hiding this comment.
I guess you want to avoid duplication in the applied stylesheets.
There was a problem hiding this comment.
responded #35736 (comment)
lib/global-styles.php
Outdated
| } | ||
|
|
||
| $new_block_classes = array( | ||
| 'css' => 'block_classes', |
There was a problem hiding this comment.
I'm unclear what this "type" is about?
There was a problem hiding this comment.
I guess this is the theme styles generated from theme.json, the name is a bit confusing. It's not just about block classes, isn't it.
There was a problem hiding this comment.
So, these are the "types":
block_classes=> whatever is instylesplus the "spacing styles" we addcss_variables=> the css vars we generate for presets plussettings.custom(these need to be in scope for the sidebar UI controls, which is the main driver. They can't get rewritten by theeditor-styles-wrapperwhile the other types can)preset_classes=> the classes for the presets
There was a problem hiding this comment.
Maybe just "styles", "variables" and "presets".
There was a problem hiding this comment.
I perhaps can expand a bit more: for the preset UI controls to work, we need the CSS Custom Properties in the scope of the sidebar, as they use these values. They are currently scoped to body so we can't let the editor convert that to .editor-styles-wrapper.
There was a problem hiding this comment.
Those names work for me.
(sorry, GitHub didn't show me your comment)
oandregal
force-pushed
the
fix/themes-with-no-editor-styles
branch
from
c83eb9d
to
c73e850
Compare
Fixes #35730
Follow up to #35424 and #34439
In Gutenberg 11.5, we added some defaults for themes that didn't enqueue any editor styles, see #34439
In Gutenberg 11.7, we made every theme to have some styles in the editor: we provided at least the core presets. See #35424 As an unintended side effect of this change, there are no themes that don't provide editor styles anymore, hence we didn't provide any editor defaults for them.
This PR adds back some defaults for themes that don't provide any editor styles.
How to test
Make sure it works with "theme styles" deactivated
Nucleare
Empty theme
TT1 Blocks
TwentyTwentyOne
Make sure it works with "theme styles" activated
Nucleare
Empty theme
TT1 Blocks
TwentyTwentyOne