-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Reduce the specificity of block styles to simplify editor styles slightly #14407
Conversation
@@ -6,7 +6,7 @@ | |||
background-position: center center; | |||
min-height: 430px; | |||
width: 100%; | |||
margin: 0 0 1.5em 0; | |||
margin: 0 0 2em 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When this margin is 2em, it perfectly matches the existing margin. This means no overlap between blocks when selected or hovered:
We can't totally prevent that from happening. For example a 3rd party block might add zero margin, and it'll look like this:
I.e. overlap.
Is this something we should be okay with? Technically it works fine since the multi block selection style and block boundaries are painted outside the block. But surfacing here for discussion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This style rule will only match up with the current margins for the bottom.
Is matching up with the current margins a goal with all blocks, some blocks , or just by coincidence sometimes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This style rule will only match up with the current margins for the bottom.
Is matching up with the current margins a goal with all blocks, some blocks , or just by coincidence sometimes?
Well it's a little more complex than that. In https://codepen.io/joen/pen/XOLamR you can see sort of a minimalistic microcosm of the current block setup. The red backgrounds represent the block footprint. The margin between the red is what those blocks themselves provide. You could have zero margin, and the text would bump against each other. Since blocks paint the selected borders outside the block footprint, this would mean those borders would overlap adjacent blocks, like this:
Inversely if you have a ton of margin, you'll just get a lot of space between blocks:
There's $block-padding variable, set to 14px if I recall. It would be nice to, separately, explore making this variable styleable by an editor style. For example if you have a layout that relies on really tight block margins but you don't want the selected block borders to overlap. But this is a challenge, and one best looked at separately I'd say.
So to answer your question, yes it's a lucky coincidence that 2em = 32px (2x16px base font), and that happens to be the same as ($block-padding * 2) + $block-spacing;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the border really a border? Perhaps it should use outline instead, so it doesn't affect the margin collapse.
@@ -0,0 +1,66 @@ | |||
// These follow a Major Third type scale | |||
h1 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should these styles be in theme.scss
, or editor.scss
? Currently, theme.scss
is currently loaded in the editor regardless of whether the theme has opted in, but it has the added benefit of not being output in the theme itself unless opted into.
It seems like we should either load theme.scss only into the editor if opted into, or reconsider what it means for the editor to have a "vanilla stylesheet". See later comments on that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So in the case of the heading Block theme.scss
exists to
- Apply default heading styling rules within the Editor
- Optionally (when opted into) apply heading styling in the Theme when you load front of the website.
Is that right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still unsure about outputting un-scoped element styles in a theme's front-end. Even with opt-in theme.css.
The pros:
- specificity couldn't be any less
- If the theme has this styled at all it's styles will be used
- 98% of themes will have
h*
font-sizes defined.
The questions:
- What if the theme has made a choice to go with the browser defaults for heading font-size or margins. Would they be happy with our modified defaults?
Essentially what we're doing here and with any naked element styles is creating our own "browser defaults".
Do we need to add our own styles here at all? The browser defaults aren't that bad.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great thoughts, thank you.
What if the theme has made a choice to go with the browser defaults for heading font-size or margins. Would they be happy with our modified defaults?
Essentially what we're doing here and with any naked element styles is creating our own "browser defaults".
Do we need to add our own styles here at all? The browser defaults aren't that bad.
I agree, the browser defaults aren't bad!
However this is another one of those situations where if we don't provide any styles for the heading sizes, then the WordPress heading sizes will be used because we inherit them. Here's an example:
That's an unscoped h1
style that the wp-admin stylesheet provides. For h1 it happens to match the browser default of 2em, but this is not hte case with h2 and h3s, and as you can also see it customizes the default font weight of all heading sizes too.
It is frustrating that this bleeds from the wp-admin into the editing canvas, and as mentioned I would like to refactor this away long term by better scoping the admin styles, or by protecting the canvas using Shadow DOM, or any other method we can to prevent or mitigate CSS bleed.
So this PR is focusing on the most beneficial next step we can take — which is to still normalize those sizes, but at least reduce the specificity so they are easier to override.
My reason for putting these in theme.scss was to provide a theme some means to adopt them without outright copy them. Another option is to put them into editor.scss
, where they will ONLY be loaded for the editor. But then a theme has to copy them, if they want to build on them. But in this specific case — for headings — maybe this is what we want?
The other perspective is that it can be convenient for a theme to simply opt into styles using
add_theme_support( 'wp-block-styles' );
�and then the frontend immediately looks like the backend and can then build from there. But perhaps this is an argument for a separate opt in, something like add_theme_support( 'wp-block-normalize' );
, which opts into the same normalization stylesheet that we need for the editing canvas?
@@ -0,0 +1,4 @@ | |||
p { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar question here.
Because the purpose of this PR is to remove the intrinsic block margin, we have to actually re-apply those margins somewhere else.
Where should these rules live? Should they be in a vanilla editor style only? Should they be opinionated editor styles as they are here? Or something else? Really difficult question, but it gets at the heart of some of the CSS bleed issues this PR also aims to fix. CC: @m-e-h for expert commentary here thank you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as with headings. If the theme has their own p
margins, those will get used. Otherwise it'll fall back to these styles or to the browser styles.
Both Chrome and Firefox have this:
p {
margin-block-start: 1em;
margin-block-end: 1em;
}
which is the same as margin-top
and margin-bottom
Here are some samples of default browser styles for reference: https://github.com/mozdevs/cssremedy/tree/master/process/UA_stylesheets
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where should these rules live?
Good question. Not really sure.
I do think that general selectors like this should probably never go in a block specific stylesheet.
Sure, this is a "paragraph" block and it makes sense to style a p
here but it sets a poor example. I wouldn't want to encourage blocks to freely use element selectors in their block styles.
So, the more I think about it, maybe a separate set of styles like this, imported or enqueued before any other block styles, is a good way to go.
It would be invisible to any theme (most) that had their own styles for these elements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make a difference if you viewed it from a Drupal developer's perspective?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels appropriate for some kind of "canvas reset" file, which is only enqueued in the backend.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as with headings. If the theme has their own p margins, those will get used. Otherwise it'll fall back to these styles or to the browser styles.
Just to clarify, Otherwise it'll fall back to these styles or to the browser styles.
is not correct for the editing canvas. In the editing canvas it will fall back to the WP-admin styles, which look like this:
I'm starting to think that the idea by @getdave and later @chrisvanpatten of a "normalization" stylesheet might be the correct place to put styles like these, which are unfortunately necessary for the time being.
@@ -0,0 +1,4 @@ | |||
p { | |||
margin-top: $default-block-margin; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could also be 2em if we thought that was nicer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer if we could stick to em
-based sizes for all text-related measurements in general. It looks like we're already doing that with top/bottom margins for our headings, so we should continue that here.
|
||
|
||
/** | ||
* Vanilla Block Editor Styles |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This here is inspired by feedback on a separate but related PR, here: #14366 (comment)
The idea is that we know we want to apply a modicum of styles to blocks, either to support them structurally (function in the first place), or to make sure we don't provide a broken or unstyled-looking experience. In the case of the figcaption
element as is the core of that other PR, this element is born without margins, which means it sits right up next to the image unless it receives a margin.
The question is, should Gutenberg have such a base stylesheet that cascades? I've collected the two existing rules we already have, for img
and iframe
, and once that other PR goes through, we'd also have figcaption
on this list. Should p
also be there? And should there be a way to opt-in to this vanilla stylesheet, or opt out of it? These are questions that would be good to figure out, and I would appreciate your thoughts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should Gutenberg have such a base stylesheet that cascades?
I'm not clear on the implications of this either way. Or is that the question?
Are you proposing effectively a normalize.css
style base stylesheet for the Editor that just restores the default styles in a standard manner or would this go further still?
And should there be a way to opt-in to this vanilla stylesheet, or opt out of it?
You should certainly be able to disable it if you know what you're doing. Probably on by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the biggest group of themes is the ones that created a style sheet for the old editor. The best way to move forward with the least impact is to have the add_editor_style
function call the new block editor style function that loads the styles with the prefixed selectors. Then the old styles will be in effect for the editor and the front end, and no blocks are styled. The HTML elements themselves are styled. This is the majority of existing themes. The editor should only style the UI elements and not the blocks. If any blocks need styling, put it in the style sheet that is opt-in.
With the old editor, there were rudimentary styles for standard classes alignleft
. alignright
, aligncenter
. These were not output on the front end. The theme is responsible for that, and it makes sense to continue in the same way.
// Apply default block margin below the post title. | ||
// This ensures the first block on the page is almost in a good position. | ||
// This rule can be retired once the title becomes an actual block. | ||
margin-bottom: $default-block-margin; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has the side-effect of always providing this much space down to the first block:
Compare with master:
But this is actually the simplest and cleanest way to provide a consistent experience without resorting to crazy first-child and conditional rules. Consider a situation where the first block inserted is not born with a top-margin. Or consider one where the first block inserted has a HUGE intentional top margin. If we zero that margin out to override and provide a different one, we cancel that out.
By providing this slightly larger margin, we avoid all that happening. If we are passionate about keeping the current behavior while improving things, the real way to go here is to convert the title to an actual block, and not just a "faux" block as it is right now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This rule seems ok, especially when we consider that if the first Block does have a margin and it is larger than the bottom margin on the page title then margin collapsing should ensure that it doesn't double up.
Block shouldn't have huge top margins, but if they do then perhaps the Block author should be responsible for ensuring they play nicely in these types of situations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems 100% fine to me, for all the reasons @getdave mentioned. And frankly, I like the appearance of the little bit of extra margin up there. 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I say remove all styles that have padding and margins for blocks. Only style the UI elements of the editor.
@@ -12,8 +12,8 @@ p { | |||
|
|||
ul, | |||
ol { | |||
margin: 0; | |||
padding: 0; | |||
margin: 1em 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's discuss this one too.
In browsers, if you don't load a stylesheet, ol
and ul
are both born with 1em margins above and below.
In this case, and the reason this rule sits in editor-styles.scss
, is that it actually has to be rewritten again, otherwise the editing canvas will inherit upstream rules from the WordPress admin CSS.
While the long term solution here is to refactor the WordPress admin to be better scoped and not bleed into the editing canvas, that's likely a ways out. In the mean time, there's this.
Which brings us to the question: is this the right place for it? How about theme.scss
like the p
rules?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels like this is needed to ensure the core markup in the editor renders in a suitable fashion out of the box. Therefore in my mind it should be part of the core editor styles rather than an optional Theme specific style. If Theme authors want to tweak the spacing they can do so or they can opt out of the theme.css
file entirely and provide their own rules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't have any styles on plain elements. If you can't put a class on it for the editing canvas, you shouldn't be styling it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it should be part of the core editor styles rather than an optional Theme specific style.
I agree. This shouldn't be optional if we're re-normalizing something from common.css
or an inline style from WP admin. If that's the case margin: initial
would work as well.
However, currently theme.css
isn't optional in the editor so there would be fine if that doesn't change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't have any styles on plain elements. If you can't put a class on it for the editing canvas, you shouldn't be styling it.
This is a noble goal, one we should strive for. But until we can prevent CSS bleed from wp-admin through, for example, Shadow DOM, we have to unstyle even plain elements, otherwise they will look like just another wp-admin settings page. Just to be completely clear, here's what that would look like: https://user-images.githubusercontent.com/1204802/54341682-b75ec680-463a-11e9-964f-607f5223364f.gif
@@ -5,7 +5,6 @@ | |||
} | |||
|
|||
.editor-rich-text__editable { | |||
margin: 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ellatrix can you recall why we added this rule?
I removed it in this PR, because it bleeds into the p
tag inside the Paragraph block, and as part of this PR we'd like to provide as clean and easily CSS overridable rules for that tag as possible. See https://github.com/WordPress/gutenberg/pull/14407/files#diff-7d328366ff8c0797fbdd334dcdf8b1a2R1
As far as I can tell, removing this one doesn't cause any visual regressions, but if you can recall why it's there it might help verify this.
assets/stylesheets/_variables.scss
Outdated
@@ -57,6 +57,7 @@ $block-spacing: 4px; // Vertical space between blocks. | |||
$block-side-ui-width: 28px; // Width of the movers/drag handle UI. | |||
$block-side-ui-clearance: 2px; // Space between movers/drag handle UI, and block. | |||
$block-container-side-padding: $block-side-ui-width + $block-padding + 2 * $block-side-ui-clearance; // Total space left and right of the block footprint. | |||
$default-block-margin: ($block-padding * 2) + $block-spacing; // This is the default margin between blocks. It matches 2em in the vanilla style. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're mostly using 2em for blocks that need it, we might want to retire this. But it's fortunate that 2em matches 32px, which happens to be the same as this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's fantastic to see this experiment raised. It really needs addressing, especially in context of Columns.
I've left what feedback I can. Available to discuss.
Thank you for the feedback, it's solid! I'll respond more in depth later, but just wanted to say thanks! |
Wonderful, thanks for testing Kjell. Yes, I definitely want to go over each block with a fine tooth comb to ensure the out of box experience is solid. I have, for the time being, focused on the Paragraph and Heading because once we get those right (and their CSS in the right place), all the other blocks can follow that pattern. |
For all of you reviewing, and please do because I'm excited to get this PR on the road, I'd like to clarify a bit about the CSS we have for blocks, why we have it, and why it's necessary. In general, the CSS we load into the editor falls into more than just one category:
For 3, the long term solutions are wide-ranging from Shadow DOM to simply refactoring wp-admin. But in order to provide near-term improvements, we have to override those. The three are important to keep in mind, when evaluating where a CSS rule should be placed, and subsequently loaded. |
I haven't dug into these enough to leave detailed feedback, but overall I really appreciate the ideas at play here. I think the "ideal" state (which, admittedly, may not actually be possible) is that Gutenberg's UI is completely agnostic about all styles except its own editor chrome. This definitely moves us a bit closer in that direction, which is really nice to see. |
@jasmussen Just to clarify:
Is this essentially the editor.css file provided by each block? This doesn't live in a centralized place, right? (Although we may compile it down to one place in the case of core blocks, I mean that these are editor styles linked to specific blocks, not applying in a global/editor-level context)
I know there's more discussion happening on #14366, but I think this is a particular area of confusion. It seems like these styles always load in the editor, but are opt-in on the front-end. There may be nothing we can do about it now, but it would be helpful to clarify — is there in fact a way to opt-out of these styles loading in the editor? I would also add that I think a good long term goal should be to remove these styles. At a certain point, they would be replaced by loading front-end styles within the editor (or loading the editor over the front-end), so older themes don't need any special accommodations.
I saw a note about |
I would agree with this, with the added note that in this ideal state, the editor is already and always styled, because it just loads the theme stylesheet in directly! It's good to restate this as the utopian goal. As you say we might not be able to fully get there, but I think we can get close, even if it may take many in-between steps, like this PR. |
No, by structural block styles, specifically with the Cover block as an example, there are styles that are necessary to load into both the editor and the theme in order for the block to work at all. For example the fixed background won't work unless the block itself supplies the ruleset, even to the theme. Though if you want as CSS pure an experience, one could imagine a block manager could disable blocks on a per-user or per-site basis, and thus not enqueue those styles.
I think it's okay to have that conversation here! So to simplify: you're suggesting that theme.scss should only be loaded into the editor if the theme has declared support using
It's both a wp-admin styles reset, but it's also a vanilla style for themes that don't supply editor styles, which is still the overwhelming majority of themes. The idea being that if you're using an old theme, you should still have a good editing experience, and not have something that looks unstyled (or in this case it would actually be a gray wp-admin Settings-like page). Balancing that with allowing those themes that actually do style the editor to easily override it, is sort of the crux of it all. |
92ae590
to
8d3cea6
Compare
Okay, comment fixed, rebased to be in sync with master, and squashed slightly so as to make it easier to rebase again since it will be in queue for a bit. |
Suggested dev note:
|
@jasmussen If we could expand the dev note about how themes could override these, that would be great. We still have time for it as it's stated for WP 5.3 but I appreciate that it's being thought of in the PR :) |
Great points, Riad. How's the following? It's quite long for a dev note, but perhaps that's fine? In #14407 changes were made to how margins are applied to blocks in the editing canvas. Where before, margins were applied uniformly to any registered block with a highly specific Here are a few examples of what editor styles should now work as expected:
Remember that in order for editor styles to properly load in the editor, you need to declare support for editor styles using |
^ That note seems fairly solid. 👍 The code example mentions a "Minimum block margin", but that's not specifically noted anywhere above. At the very least, we may want to write "Optionally add a minimum block margin" or something, to show this is a new, totally optional thing. |
Btw, we can merge this branch if you all think it's ready. |
Great! I'm merging, that way we'll have some time to let the changes sit in master before the next plugin release. Thanks all!
This is actually not new :| it was new in the beginning of this PR, in that the PR actually removed all block margins and re-applied them on a per-block basis. But after conversation on this thread, that "minimum block margin" was reapplied again, and brought back to life, just 4px smaller, the idea being that every block does need a minimum margin between itself and other blocks. While this can definitely still be argued and probably will be, this PR at least does a ton of refactoring to make future explorations easier. In addition to that, it will now be trivially easier for editor styles to control that margin, as they just need to target |
This change, specifically the margins applied using the Broken layout: Previous (and expected) layout: Can this change be revisited please? This selector is too broad, not scoped with any specific Gutenberg class and, at the very least, should be restricted to only the editor content area. |
Changing the selector to |
This PR is round 2 of #8350. The ultimate goal is to make it easier for themes to style the editor, and to do less CSS overriding in order to do so.
Problem: Currently, every single block is born with an intrinsic top and bottom margin. This margin matches the padding that sits between the block and the "selected block" border, plus 2px of space. This margin is the same regardless of whether the block needs a margin or not, and it is applied to nesting containers as well. In the case of the Columns block for example, that means the column block (singular) has top and bottom margin, even though it shouldn't have that.
Since then a number of changes have been made to the editor to make it a good time to revisit this:
Proposed solution: By removing the intrinsic margin, we can re-apply it only to the blocks that actually should be born with an intrinsic margin, such as paragraphs, lists, quotes ,etc.
Some discussion points that are likely to surface: where should those vanilla styles be stored? How should they be structured so that themes can easily override them? Should these not be loaded if a theme provides its own editor styles? Should we leverage the cascade and store these generically in one location or should these be applied in the style.scss file for every block in the library? Given these intrinsic margins have been present since day one, can we expect plugin authors to remember to add these margins themselves for every block they make? Is there a back-compat way we provide these default margins to blocks that rely on them?
This is a try branch, in order to figure out answers to those questions. This first commit only does a few things:
Next commits will explore how to remove that blanket reapplication, and try to provide those vanilla styles in a per-block basis.
This PR, depending on feedback, will likely be ready to go once the editor looks mostly the same in this branch as it does in master, regardless of the WordPress theme you're running. But with the added benefits that themers now have increased quality of life.
See also:
#13989 (comment)
https://github.com/WordPress/gutenberg/pull/8350/files
CC: @m-e-h @chrisvanpatten