font-size presets: fix specificity in editor #21969
Conversation
oandregal
requested review from
ajitbohra,
jorgefilipecosta,
Soean and
talldan
as code owners
oandregal
changed the title
oandregal
added
[Feature] Themes
|
Size Change: +565 B (0%) Total Size: 817 kB
ℹ️ View Unchanged
|
oandregal
added
CSS Styling
|
To clarify, it looks like this is merely intending to revert styles to as they existed prior to the changes here: |
|
One thing I'm observing in testing is that when assigning "Huge" font size in the TwentyTwenty theme, it doesn't actually appear large. It's because the style introduced here is being wrapped as Given the timing of #21428, I'm not sure if this would be "expected". |
Yes, exactly.
You mean, TwentyNnineteen, right? That's a known issue for that theme. The reason is that we stopped adding inline styles in the editor when the user selects a preset font (like huge), so it behaves like the front-end. |
|
I think the root removal is a good thing especially since it’s affecting the frontend but we do the same for the colors on the same file and I believe we should be consistent; |
|
I still think this is low risk/high reward, in case we still wanted to go ahead with this. I'm also fine with slating this for the next release given that it's just 2 weeks away (I just feel bad for not having spotted this earlier due to AFK issues).
Based on my reading of #15167 I understand that, for colors, it was done because some hover/visited statuses that would have higher specificity than the classes. My archeology skills are failing to understand why we decided for increasing the specificity with |
| .has-normal-font-size { | ||
| font-size: 16px; | ||
| } | ||
| .has-small-font-size { |
There was a problem hiding this comment.
This is working to fix the issue in #21759, however I'm not sure the bare .has-xx-font-size classes are actually specific enough to work out of the box now. When testing with a theme that supplies no editor styles (Gutenberg Starter Theme), it appears that these rules are just overridden by the default .editor-styles-wrapper p font size rule.
Do we need to at least change these to .editor-styles-wrapper .has-xx-font-size? That should allow them to kick in by default, but still be overridden by theme styles (assuming the theme styles have the .editor-styles-wrapper selector added via add_editor_style()).
There was a problem hiding this comment.
The root problem seems to me that we bump up the specificity for theme's stylesheets but not for its dependencies (the defaults provided by Gutenberg). The ideal solution would be that we also wrap the block-library stylesheets with editor-styles-wrapper at runtime. However, I fear that ship may have already sailed. I'll be more than extremely happy to be proved wrong and go with this approach, though.
Alternative solution: increase the specificity of these classes, only for the editor. My only concern is how would we do it. All the options I see make me sigh. I pushed a fix by adding the editor-styles-wrapper class in this file, although I know that we tend to avoid adding the editor wrapper class to anything that goes to the front-end. The alternatives I've considered include:
- Replicate the classes in both files => we enqueue the classes twice + duplicate source of truth (if we change them in the future we'll have to update both places and can be easily forgotten).
- Extract them to a file that will be imported here and in editor.scss => we enqueue the classes twice + it adds indirection.
Note that this happens in every theme that don't provide a font size palette themselves (for default themes: all but TwentyNineteen and TwentyTwenty) since we no longer inline the style declaration for presets.
[1] And also TwentyNineteen, but for different reasons: although it does provide a palette, it is identical to Gutenberg's except for the removal of Medium text size, so it doesn't care about enqueuing the classes itself. For that reason, it behaves like the themes that don't opt-in into the font-size palette. It could be argued that this case should be fixed in the theme itself. If we provide a fix for the other themes, we're also fixing this, though.
There was a problem hiding this comment.
What if we extracted the palettes (color, font, gradients) to its own stylesheets and only enqueue them if the theme hasn't declared support?
It somehow makes extracting the files clearer for me, it'd be reasonable that they contain styles for both editor&front-end, and also is good practice not to ship CSS we don't use. At the same time, it is also a bit more work than what we have here and, given the sheer amount of things to do, I find it difficult to justify (although it's the approach I'd do otherwise).
There was a problem hiding this comment.
Alternative solution: increase the specificity of these classes, only for the editor. My only concern is how would we do it. All the options I see make me sigh. I pushed a fix by adding the editor-styles-wrapper class in this file, although I know that we tend to avoid adding the editor wrapper class to anything that goes to the front-end.
Can't we just have two sets of styles — one in editor.scss, and one in theme.scss? It would be in two separate places, but that's what we usually do when we need separate styles for the front and back end.
There was a problem hiding this comment.
Oh, but isn't theme.scss only enqueued if theme declares support for wp-block-styles (which is something themes that don't declare font-size palettes are less likely to do)? I thought that wouldn't fix the issue for most themes.
There was a problem hiding this comment.
Thank you for explaining so thoroughly, Kjell.
Master:
In other words, the custom theme rule will be ignored completely in the front end. 😞
So far, so good.
This PR:
.has-huge-font-size { font-size: 72px } // Custom Theme Rule
.has-huge-font-size { font-size: 72px } // Default Gutenberg Rule
Cool, works because it comes after, also good. Side question, though: when a theme referenced class name such as this one clashes with Gutenberg, is either the theme or the editor doing something wrong?
p { font-size: inherit; } rule that is prefixed with .editor-styles-wrapper. This has higher specificity than the new .has-xx-font-size, so it cancels out the custom font size rule in the editor.
Yes, I'm running into this with my own theme I'm building, where this:
p {
margin-top: 1em;
margin-bottom: 1em;
}
in the editor is cancelled out by:
.editor-styles-wrapper .block-editor-block-list__block {
margin-top: 28px;
margin-bottom: 28px;
}
It seems a similar issue, the tension between the editor wanting to provide some baseline styles, and the theme needing to trivially override those.
This works, but it provides an unnecessary .editor-styles-wrapper .has-xx-font-size rule to the frontend. Not a huge problem, but not ideal.
There we go, that seems to be the crux of the issue.
As I read this, my instinct is to think that if we provide a thorough comment, this is an okay tradeoff for now. Take the entire introduction in the normalization file, it's one big "ugh wp-admin bleed" disclaimer. So it all trails back to that.
But we are more aware than ever! An iframe is potentially on the horizon, which would allow us to throw out most, if not all, of the normalization stylesheet, prompting us to be able to refactor this also.
I wonder if we should make a specific keyword for such a refactor — in my own private projects, I often write @todo so I can search for that identifier. We could make a @todo-editor-styles, which we can then search for if either an iframe, or Shadow DOM protection, or better scoping of wp-admin styles land.
There was a problem hiding this comment.
Side question, though: when a theme referenced class name such as this one clashes with Gutenberg, is either the theme or the editor doing something wrong?
That's a good philosophical question. 😄 I don't think either is doing it wrong — we expect some Gutenberg styles to be overridden by the themes in general.
As I read this, my instinct is to think that if we provide a thorough comment, this is an okay tradeoff for now. Take the entire introduction in the normalization file, it's one big "ugh wp-admin bleed" disclaimer. So it all trails back to that.
Good point. I think you're probably right. This works for now, and things will be (hopefully) changing soon. So that may be good enough for the moment. @nosolosw what do you think?
There was a problem hiding this comment.
Reading @kjellr outline above it seems to me that the issue in twofold: how to make palettes work with 1) themes that don't provide one and 2) themes that provide one with the same class names that Gutenberg defaults.
In its current state, this PR fixes 1. In addition to this, should we also enqueue the palettes only if the theme hasn't provided one? That should fix 2 as well.
Sorry if I missed anything and this doesn't make any sense 😅 I'm from mobile and AFK so my brain is in low-power mode. I join the wishes that we can remove this altogether in the not so distant future!
There was a problem hiding this comment.
Re-reading this thread I gather we're ok with this solution for now. Is that right @kjellr @jasmussen ? If so I'd need an 💚
I was thinking this could land shorty so it can be in for next week release. I'd like to avoid having the :root for font-sizes for much longer as it was only added for 7.9 because we forgot to remove it after some experiments we did (can be considered a regression).
If the other issue I mentioned (conflicts with themes that provide the same class names than Gutenberg) is pressing I can prepare a different PR to solve that (I believe the answer to that issue would be to not enqueue the color & font-sizes classes if the theme has declared its own).
There was a problem hiding this comment.
If the other issue I mentioned (conflicts with themes that provide the same class names than Gutenberg) is pressing I can prepare a different PR to solve that (I believe the answer to that issue would be to not enqueue the color & font-sizes classes if the theme has declared its own).
That sounds like a promising solution. 👍
There was a problem hiding this comment.
As discussed, this fixes the problem for now. 👍 Thanks, @nosolosw!
oandregal
changed the title
Fixes #21759
This is a leftover of an experiment where we introduced font-size declarations in the blocks by default that we reverted later.