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.

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

Try: Remove all baseline margins. #22208

Closed
wants to merge 1 commit into from
Closed

Conversation

jasmussen
Copy link
Contributor

This PR is a proof of concept, intended to start a conversation. It should probably not be merged as-is.

The block editor has something we refer to as a "baseline margin". This:

.block-editor-block-list__block {
    margin-top: 28px;
    margin-bottom: 28px;
}

What it does, is add margin above and below every single block. The purpose is two-fold:

  1. To override CSS bleed that is inherited from wp-admin.
  2. To provide the vanilla editor styles with some vertical rhythm that provides a better editing experience for themes that do not provide editor styles.

This baseline margin is editor only, it is not output on the frontend.

There are many downsides to this baseline margin:

  • It's provided to every block, including pure-layout blocks such as the column block that exists inside the columns block.
  • It is, for example in the case of the column block above, compensated for. This adds complexity to the block editor style CSS.
  • Due to how margins can collapse, it's often hard to debug where that extra spacing comes from.

This PR tries to remove that baseline margin entirely. Before:

before

After:

after

Observe how in the "after" image, the margin between paragraphs is smaller. That's because now it inherits this from the Paragraph rules set by wp-admin:

Screenshot 2020-05-08 at 09 50 19

In principle, this sucks, because it's a rule designed for an entirely different context.

So why remove the baseline margin?

Because it will benefit themes that do provide an editor style, and it will simplify the CSS that blocks have to write to work in the editor. Right now even blocks containing of a div elements that are born without margins, have margins applied to them by this baseline margin.

Notably, it's means that the editor looks different from the frontend in many cases; a block that does not have a margin registered won't have margin on the frontend, but it will on the backend.

A better example is the Cover block, which is a div that has no margin. Before:

Screenshot 2020-05-08 at 09 43 06

After:

Screenshot 2020-05-08 at 09 42 52

This might not look good in the editor, but it would be accurate to what is rendered on the frontend.

Is there something else we can do, that allows the editor to look more like the frontend, and still provide a good baseline editing experience for themes that don't?

A couple of ideas:

  1. Iframe the editing canvas. Difficult, potentially impossible, but a proof of concept has been created: Iframe the editor content #21102 — this would at least prevent blocks from inheriting random rules set by wp-admin.
  2. Provide a stylesheet that is loaded only for the editor when no editor styles are registered.

1 would solve many problems with regards to front and backend consistency, but is not something we should count on landing soon, possibly ever.

2 would allow us to create a good baseline editor style when the theme doesn't provide one. It might even be an opportunity to revisit what the editor looks like without editor styles — Noto Serif hasn't aged well.

Penny for your thoughts?

@jasmussen jasmussen added Needs Design Feedback Needs general design feedback. [Feature] Custom Editor Styles Functionality for adding custom editor styles labels May 8, 2020
@jasmussen jasmussen self-assigned this May 8, 2020
@github-actions
Copy link

github-actions bot commented May 8, 2020

Size Change: -23 B (0%)

Total Size: 824 kB

Filename Size Change
build/block-library/editor-rtl.css 7.07 kB -6 B (0%)
build/block-library/editor.css 7.08 kB -7 B (0%)
build/editor/editor-styles-rtl.css 421 B -4 B (0%)
build/editor/editor-styles.css 422 B -6 B (1%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.02 kB 0 B
build/annotations/index.js 3.62 kB 0 B
build/api-fetch/index.js 4.08 kB 0 B
build/autop/index.js 2.82 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/index.js 6.61 kB 0 B
build/block-directory/style-rtl.css 760 B 0 B
build/block-directory/style.css 761 B 0 B
build/block-editor/index.js 102 kB 0 B
build/block-editor/style-rtl.css 10.3 kB 0 B
build/block-editor/style.css 10.3 kB 0 B
build/block-library/index.js 115 kB 0 B
build/block-library/style-rtl.css 7.28 kB 0 B
build/block-library/style.css 7.29 kB 0 B
build/block-library/theme-rtl.css 683 B 0 B
build/block-library/theme.css 685 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/blocks/index.js 48.1 kB 0 B
build/components/index.js 180 kB 0 B
build/components/style-rtl.css 17 kB 0 B
build/components/style.css 16.9 kB 0 B
build/compose/index.js 6.66 kB 0 B
build/core-data/index.js 11.4 kB 0 B
build/data-controls/index.js 1.29 kB 0 B
build/data/index.js 8.44 kB 0 B
build/date/index.js 5.47 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 568 B 0 B
build/dom/index.js 3.1 kB 0 B
build/edit-navigation/index.js 4.4 kB 0 B
build/edit-navigation/style-rtl.css 618 B 0 B
build/edit-navigation/style.css 617 B 0 B
build/edit-post/index.js 28.1 kB 0 B
build/edit-post/style-rtl.css 12.2 kB 0 B
build/edit-post/style.css 12.2 kB 0 B
build/edit-site/index.js 12.2 kB 0 B
build/edit-site/style-rtl.css 5.21 kB 0 B
build/edit-site/style.css 5.21 kB 0 B
build/edit-widgets/index.js 8.37 kB 0 B
build/edit-widgets/style-rtl.css 4.69 kB 0 B
build/edit-widgets/style.css 4.69 kB 0 B
build/editor/index.js 44.3 kB 0 B
build/editor/style-rtl.css 5.07 kB 0 B
build/editor/style.css 5.08 kB 0 B
build/element/index.js 4.65 kB 0 B
build/escape-html/index.js 734 B 0 B
build/format-library/index.js 7.63 kB 0 B
build/format-library/style-rtl.css 502 B 0 B
build/format-library/style.css 502 B 0 B
build/hooks/index.js 2.14 kB 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.56 kB 0 B
build/is-shallow-equal/index.js 710 B 0 B
build/keyboard-shortcuts/index.js 2.51 kB 0 B
build/keycodes/index.js 1.94 kB 0 B
build/list-reusable-blocks/index.js 3.12 kB 0 B
build/list-reusable-blocks/style-rtl.css 226 B 0 B
build/list-reusable-blocks/style.css 226 B 0 B
build/media-utils/index.js 5.29 kB 0 B
build/notices/index.js 1.79 kB 0 B
build/nux/index.js 3.4 kB 0 B
build/nux/style-rtl.css 616 B 0 B
build/nux/style.css 613 B 0 B
build/plugins/index.js 2.56 kB 0 B
build/primitives/index.js 1.5 kB 0 B
build/priority-queue/index.js 789 B 0 B
build/redux-routine/index.js 2.85 kB 0 B
build/rich-text/index.js 14.8 kB 0 B
build/server-side-render/index.js 2.67 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.28 kB 0 B
build/url/index.js 4.02 kB 0 B
build/viewport/index.js 1.84 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.18 kB 0 B

compressed-size-action

jasmussen added a commit that referenced this pull request May 8, 2020
This PR is a much smaller alternative to #22208. It sets the top and bottom margin of a group block to zero, in the editor only, to unset the margins that are otherwise inherited by the baseline margin that every block in the editor has.

This helps a great deal for when people create block patterns, where you might have several groups in a row, that should not have additional margin above and below.

Important also to remember that the margins of contents inside the group can still "bleed out" of the group, and even collapse with adjacent margins.

It would be nice to not have to unset these margins, and simply not have any margins in the first place. But per the discussions in #22208 removing those is nontrivial.
@kjellr
Copy link
Contributor

kjellr commented May 8, 2020

Because it will benefit themes that do provide an editor style, and it will simplify the CSS that blocks have to write to work in the editor. Right now even blocks containing of a div elements that are born without margins, have margins applied to them by this baseline margin.

Essentially, this is why we have all these themes that use wildcard selectors for the front end, right? To provide the same base margins on every element, regardless of whether or not it traditionally would need margins. That sets the front end up to mimic Gutenberg's editor styles by default. For example, this is from Twenty Nineteen's front-end styles:

.entry .entry-content > *,
.entry .entry-summary > * {
  margin: 32px 0;
}

As you noted, this is weird — many selectors don't traditionally need these default margins.

For instance, these margins make it unnecessarily difficult to have two blocks stacked up directly on top of each other:

Screen Shot 2020-05-08 at 8 47 37 AM

Or, in a block-based theme, to have a cover block touch the top of the page:

Screen Shot 2020-05-08 at 8 49 37 AM

I do think we should eventually do something about this, but I'm not sure what the best solution is. What was the reasoning behind adding this margin in the first place? Was it primarily to override the default WP-Admin styles? Or did it have to do with making blocks more easily selectable?

@kjellr kjellr added the [Feature] Themes Questions or issues with incorporating or styling blocks in a theme. label May 8, 2020
@jasmussen
Copy link
Contributor Author

Thank you Kjell, I knew you'd be able to provide valuable insights.

What was the reasoning behind adding this margin in the first place? Was it primarily to override the default WP-Admin styles? Or did it have to do with making blocks more easily selectable?

Two reasons:

  1. To override the margin inherited by wp-admin.
  2. To provide consistent margins between blocks at a time where editor styles were barely in its infancy.

1 remains an issue, but 2 is something to start to revisit.

jasmussen added a commit that referenced this pull request May 11, 2020
#22209)

* Group: Zero out the intrinsic margin set to every block in the editor.

This PR is a much smaller alternative to #22208. It sets the top and bottom margin of a group block to zero, in the editor only, to unset the margins that are otherwise inherited by the baseline margin that every block in the editor has.

This helps a great deal for when people create block patterns, where you might have several groups in a row, that should not have additional margin above and below.

Important also to remember that the margins of contents inside the group can still "bleed out" of the group, and even collapse with adjacent margins.

It would be nice to not have to unset these margins, and simply not have any margins in the first place. But per the discussions in #22208 removing those is nontrivial.

* Address feedback.
@karmatosed
Copy link
Member

The idea I love behind this, it makes sense to remove and set a clear point. It's also true themes have had to work around a lot of things. However, my concern is what breaks and how to communicate/ease this change. That doesn't mean we shouldn't do it and as someone that's had to hack around it before I think now might be a great time before full site editing is here. The potential to open easier layouts with this feels quite significant and worth exploring.

@jameskoster
Copy link
Contributor

Does replacing a 32px margin with a 0px margin feels a little arbitrary? 🤔

Some aspects of the editing experience are improved, but others are worsened.

Perhaps something to discuss elsewhere, but how do we see vertical rhythm being governed in the future? Should that come from the theme, global styles, or a combination of the two? Knowing this might help shape direction here.

From my perspective:

  • As a theme author, I would expect to be able to specify a single base-level "vertical rhythm" value. The spacing between all blocks should be based on that value.
  • As a user, I would expect to be able to highlight multiple blocks, and adjust the spacing between them, in units of the aforementioned base-level vertical rhythm. Crude example below

spacing

@jasmussen
Copy link
Contributor Author

Jay I think you're touching on some interesting aspects with that question, ones that did play some part in the original decision to add a baseline margin.

It remains a good question to discuss, but I wanted to clarify that as it is, this baseline margin is editor only, and has no effect on the frontend.

It's also important to remember that at the moment this affects every block, not just top level blocks, but also blocks that are meant to be transparent, like the singular column.

@ZebulanStanphill
Copy link
Member

In cases where blocks are right next to each other, like the Cover blocks in one of your images, @jasmussen, the in-between inserter never seems to appear. (Though perhaps it is appearing underneath the 2nd block and simply isn't visible.)

Situations like these were one of the reasons for my in-between inserter overhaul proposal in #13571. (The mockups there are out-of-date, but a lot of the points are still valid.)

@jasmussen
Copy link
Contributor Author

@ZebulanStanphill that's a good observation and definitely a point to consider.

I do think we need a better heuristic and behavior for the sibling inserter. I think the ticket you've created makes some strides on that, but I would love for there to be an even better approach that lets us keep the behavior of being able to add blocks even when none are selected. It's also an opportunity to revisit the behavior of being able to insert something before the first block in a page, which currently isn't possible. In that vein, I acknowledge and agree with the problems of the current heuristic — but I think if we really put our minds to it, we can go even further in improving it.

@ZebulanStanphill
Copy link
Member

One thing that would be nice (but only useful for mouse/touch users) would be the ability to drag-and-drop a block from the inserter. Of course, making it feel nice requires making drag-and-drop work better in general.

As for keyboard and screen-reader users, maybe just add another hidden keyboard-only inserter button below the current block?

@jasmussen
Copy link
Contributor Author

One thing that would be nice (but only useful for mouse/touch users) would be the ability to drag-and-drop a block from the inserter. Of course, making it feel nice requires making drag-and-drop work better in general.

Yep, I think with the coplanar inserter, this makes even more sense than before. And agree with the second bit — drag and drop in general is not good enough, and needs a holistic approach.

@richtabor
Copy link
Member

Provide a stylesheet that is loaded only for the editor when no editor styles are registered.

I really like this idea, though I imagine there would still need to be some base styling provided either way.

I know we're talking about CSS variables potentially for the global styles work. What if this margin applied was simply a CSS variable (with a fallback of whatever value) that the theme can set? Then that margin may be applied on the front-end and within the editor.

@jasmussen
Copy link
Contributor Author

I know we're talking about CSS variables potentially for the global styles work. What if this margin applied was simply a CSS variable (with a fallback of whatever value) that the theme can set? Then that margin may be applied on the front-end and within the editor.

Let's try it!

The only other benefit to removing all the margins by default, is that it doesn't add margins to anything that shouldn't have it. The column inside the columns block should not have it, for example, and there are a number of other cases where technically it's a block, but it should not be treated as one. We can mitigate this, but it's not as clean. Any ideas I'm overlooking here?

@kjellr
Copy link
Contributor

kjellr commented Jun 5, 2020

I know we're talking about CSS variables potentially for the global styles work. What if this margin applied was simply a CSS variable (with a fallback of whatever value) that the theme can set? Then that margin may be applied on the front-end and within the editor.

This seems like it would be an interesting starting point. Like you said, It would dovetail nicely into the Global Styles work.

The only other benefit to removing all the margins by default, is that it doesn't add margins to anything that shouldn't have it. The column inside the columns block should not have it, for example, and there are a number of other cases where technically it's a block, but it should not be treated as one. We can mitigate this, but it's not as clean.

I think this can be handled by the "smart defaults" philosophy. In the case of the single column, we should strip those margins out by default. For blocks like the Media & Text or Cover, maybe they have margins by default, but they have a toggle (alongside a Global Styles setting) to turn them off.

@richtabor
Copy link
Member

The column inside the columns block should not have it, for example, and there are a number of other cases where technically it's a block, but it should not be treated as one.

Except for if the columns are stacked on mobile.

@jasmussen
Copy link
Contributor Author

Delicious point 👌

@jasmussen
Copy link
Contributor Author

@nosolosw Andrés my friend, given your global styles involvement, may I ask your advice for a moment?

Currently, everything that registers itself as a block (paragraph, column, button, buttons, columns, column, etc.) is created with a fixed top and a bottom margin, $default-block-margin: 28px;. But only in the editor. On the front-end, just the block markup is output.

Recent conversations suggest that perhaps global styles could take over this "block margin", and have it be output not only in the editor, but on the frontend as well. For starters, it would be a CSS variable, something like --wp--preset--block-margin: 28px;, and then at some point it could be surfaced as configurable in the global style tools.

What are your thoughts on this? Caveats, challenges, problems? Not urgent, would appreciate your thoughts, thank you!

@oandregal
Copy link
Member

👋 friends, thanks for your patience here, I was a bit slow to catch up the past few days.

I can share what our experience is with CSS custom properties so far: we're using them for link color & presets (font-sizes, colors, gradients) and they work nicely there; we tried them for background & padding, but custom properties showed cascading issues that regular properties don't have, hence we paused that work until we can do more explorations with nested content. I'm personally excited to resume those explorations but we've been focused on other things first.

Thinking about the roadmap (address this sort of issues for margin is part of it) and what's the direction we're working towards, I guess the first step to fix this would be to add the design tools for margin for some blocks, and then try to absorb margin via theme.json (example).

I'm going to pull @youknowriad @ItsJonQ in case they have more thoughts.

@jasmussen
Copy link
Contributor Author

No urgency at all to this one, Andrés, I know you've been busy. Thanks for responding.

@richtabor
Copy link
Member

I guess the first step to fix this would be to add the design tools for margin for some blocks, and then try to absorb margin via theme.json (example).

I've experimented with CSS properties for block margins within the editor and the frontend with the Go theme; I'd be happy to help with any questions/ideas you may have.

I am a bit hesitant on block specific margin (padding is fine, but margin is tough). My original intent was suggesting the margin variable is used as a global function, for example a baseline, wide-aligned, and fullwidth aligned margin values. I'd suggest that's a good place to start, with custom block-specific margin controls as a follow-up.

@paaljoachim
Copy link
Contributor

paaljoachim commented Jun 12, 2020

As I read Joen's initial post what stuck to my mind was adding margin controls similar to the padding controls that were just merged for the Cover Block.

Having margins controls, and having specific blocks have a margin of a certain value it starts with, the negative impact of removing the default/baseline margins can be removed. As the blocks that would have the most negative impact would add back in the baseline margin as a start value the user can then easily modify through the Block margin controls.

Rephrasing.... this means removing all default/baseline/wp-admin/etc margins, and instead adding in start/new default values where it needs to be added with help of the Block margin controls.

@jasmussen
Copy link
Contributor Author

I am a bit hesitant on block specific margin (padding is fine, but margin is tough). My original intent was suggesting the margin variable is used as a global function, for example a baseline, wide-aligned, and fullwidth aligned margin values. I'd suggest that's a good place to start, with custom block-specific margin controls as a follow-up.

Thanks @richtabor for your comments, much appreciated. Can you expand a little bit on this paragraph, I want to make sure I understand it right. When you mention wide and fullwidth aligned margins, are you referring to horizontal margins? This aspect definitely needs work, and is tracked in #20650. But just to be totally clear, with this PR I've explored the to and bottom margins applied in a blanket way across everything that registers itself as a "block", in the editor, and none of those margins which are output on the frontend.

@richtabor
Copy link
Member

I am a bit hesitant on block specific margin (padding is fine, but margin is tough). My original intent was suggesting the margin variable is used as a global function, for example a baseline, wide-aligned, and fullwidth aligned margin values. I'd suggest that's a good place to start, with custom block-specific margin controls as a follow-up.

Thanks @richtabor for your comments, much appreciated. Can you expand a little bit on this paragraph, I want to make sure I understand it right. When you mention wide and fullwidth aligned margins, are you referring to horizontal margins? This aspect definitely needs work, and is tracked in #20650. But just to be totally clear, with this PR I've explored the to and bottom margins applied in a blanket way across everything that registers itself as a "block", in the editor, and none of those margins which are output on the frontend.

I'm actually referring to varying top/bottom margins for wide/fullwidth aligned items. For example, the TwentyTwenty theme adds additional margin if your block is set to fullwidth (giving bigger content more breathing room). In Go I've done the same thing. There's a baseline value for margin, then ~1.5x that for wide aligned blocks, and 6x that for fullwidth aligned blocks.

Those margins are exactly the same within the editor and on the front-end.

If we do the CSS variable idea, it'll be easy to carry that value across the editor to the front-end, where the theme (or default styling even) could apply it correctly.

@mtias mtias mentioned this pull request Jun 15, 2020
82 tasks
@jasmussen
Copy link
Contributor Author

Thank you for the clarification. That does seem like a totally fair use case. And it seems like converting it to a CSS variable would let you more easily do that, even if it's a single baseline CSS variable margin for all blocks.

Or, did you mean multiple margin variables, one for each state of alignment? Just making sure.

@jasmussen
Copy link
Contributor Author

Closing this one for now, we can always reopen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Custom Editor Styles Functionality for adding custom editor styles [Feature] Themes Questions or issues with incorporating or styling blocks in a theme. Needs Design Feedback Needs general design feedback.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants