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.

Sign up for GitHub

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

font-size presets: fix specificity in editor #21969

Merged
merged 2 commits into from
May 6, 2020
Merged

font-size presets: fix specificity in editor #21969

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2181f04
Remove root pseudoclass
oandregal Apr 29, 2020
163143a
Wrap the font-sizes for the editor
oandregal Apr 30, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 25 additions & 19 deletions packages/block-library/src/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,30 +216,36 @@


// Font sizes.
:root {
.has-small-font-size {
font-size: 13px;
}

.has-regular-font-size, // Not used now, kept because of backward compatibility.
.has-normal-font-size {
font-size: 16px;
}
// The reason we add the editor class wrapper here is
// to avoid enqueing the classes twice: here and in ./editor.scss
.editor-styles-wrapper .has-small-font-size,
.has-small-font-size {
Copy link
Contributor

@kjellr kjellr Apr 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Screen Shot 2020-04-29 at 9 15 47 AM

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()).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. 👍

font-size: 13px;
}

.has-medium-font-size {
font-size: 20px;
}
.editor-styles-wrapper .has-regular-font-size,
.editor-styles-wrapper .has-normal-font-size,
.has-regular-font-size, // Not used now, kept because of backward compatibility.
.has-normal-font-size {
font-size: 16px;
}

.has-large-font-size {
font-size: 36px;
}
.editor-styles-wrapper .has-medium-font-size,
.has-medium-font-size {
font-size: 20px;
}

.has-larger-font-size, // Not used now, kept because of backward compatibility.
.has-huge-font-size {
font-size: 42px;
}
.editor-styles-wrapper .has-large-font-size,
.has-large-font-size {
font-size: 36px;
}

.editor-styles-wrapper .has-larger-font-size,
.editor-styles-wrapper .has-huge-font-size,
.has-larger-font-size, // Not used now, kept because of backward compatibility.
.has-huge-font-size {
font-size: 42px;
}

// Text alignments.
.has-text-align-center {
Expand Down