WordPress.org

Make WordPress Core

Opened 12 months ago

Closed 8 months ago

Last modified 7 months ago

#48526 closed defect (bug) (fixed)

TwentyNineteen: Update margins in editor styles to address upcoming block editor margin changes

Reported by: Joen Owned by: ianbelanger
Milestone: 5.4 Priority: normal
Severity: normal Version: 5.0
Component: Bundled Theme Keywords: has-patch
Focuses: css Cc:

Description

In https://github.com/WordPress/gutenberg/pull/17877, the left and right margins of all blocks are refactored. This change will make it easier for block and theme developers to create more complex designs, because those margins are now effectively gone in most cases, meaning there's nothing to compensate for.

TwentyNineteen compensates for those margins that have now been removed. In doing so the layout now looks broken.

This patch updates the TwentyNineteen editor styles to address that.

Please test by running the latest master of the Gutenberg plugin, WordPress version doesn't matter, and TwentyNineteen with this patch applied. Test images, galleries, cover and other blocks that can receive full-wide alignment, and verify there is no horizontal scrollbar in the editor. Then test with a full-wide group block, with and without background, verify there's no horizontal scrollbar. Then test a fullwide image inside a fullwide group block and verify there are no horizontal scrollbars.

Finally verify that the little gray "tick" that sits above headlines, is also correctly aligned above the Title.

Attachments (12)

twentynineteen-margins.diff (3.1 KB) - added by Joen 12 months ago.
Git patch to fix margins.
before.png (1.6 MB) - added by Joen 12 months ago.
Image showing "before", notice the horizontal scrollbar and the offset title tick.
after.png (2.7 MB) - added by Joen 12 months ago.
After the patch is applied, notice correct margins and tick position.
groups.png (2.8 MB) - added by Joen 12 months ago.
Fullwide background colored groups, after this patch, notice the margins of text are flush.
twentynineteen-margins.1.diff (2.8 KB) - added by Joen 12 months ago.
Git patch to fix margins version 2.
group after patch 2.png (325.2 KB) - added by Joen 12 months ago.
Paragraph followed by a fullwide group with paragraphs inside. They should be flush in margins.
group after patch 2 b.png (162.7 KB) - added by Joen 12 months ago.
Same as previous, but with a background on the fullwide group.
48526.diff (3.3 KB) - added by kjellr 9 months ago.
48526.1.diff (18.9 KB) - added by kjellr 9 months ago.
48526.2.diff (18.8 KB) - added by kjellr 9 months ago.
48526.3.diff (19.7 KB) - added by kjellr 8 months ago.
48526.4.diff (20.2 KB) - added by kjellr 8 months ago.

Change History (36)

@Joen
12 months ago

Git patch to fix margins.

@Joen
12 months ago

Image showing "before", notice the horizontal scrollbar and the offset title tick.

@Joen
12 months ago

After the patch is applied, notice correct margins and tick position.

@Joen
12 months ago

Fullwide background colored groups, after this patch, notice the margins of text are flush.

#1 follow-up: @Joen
12 months ago

I hope this is the right place to patch TwentyNineteen.

I'm not sure about the best timing for this to go in — it should not be in 5.3 as the margins change is not part of that release. But the block editor change is very likely to land in the next version of the Gutenberg plugin release. I will defer to good advice on when and how to roll out this change.

To clarify, if this patch goes into the theme for a user that is not running the plugin, the editor will only look _slightly_ broken. Specifically, full wide blocks will not be quite as full as they should be, and the tick above the title will be slightly offset to the left. But this is cosmetic only, and notably there won't be any horizontal scrollbars. So I do think it's safe to patch into trunk right after the 5.3 final release, if that's how it's usually done.

For the record, here's the block editor dev note:

In <a href="https://github.com/WordPress/gutenberg/pull/17877">17877</a> a change was made to how blocks are laid out in the block editor. Previously, every block would come with padding built in, left and right, and negative margins to compensate. These margins and paddings have been refactored away. This should drastically simplify the CSS necessary to style blocks, for block developers and for WordPress themers wanting to style the editor.

The refactor may cause issues in existing block or editor styles that have compensated for the previous margins/paddings. If your block or editor style looks off, a likely fix is to remove the styles used to compensate.

#2 @SergeyBiryukov
12 months ago

  • Component changed from Themes to Bundled Theme
  • Milestone changed from Awaiting Review to 5.4

#3 in reply to: ↑ 1 @SergeyBiryukov
12 months ago

Replying to Joen:

I hope this is the right place to patch TwentyNineteen.

It is, thanks for the patch :)

There's a commented out rule in line 934 of style-editor.scss, is that intentional?

//left: calc(-12.5% - 72px);

@Joen
12 months ago

Git patch to fix margins version 2.

@Joen
12 months ago

Paragraph followed by a fullwide group with paragraphs inside. They should be flush in margins.

@Joen
12 months ago

Same as previous, but with a background on the fullwide group.

#4 @Joen
12 months ago

Great catch. This property was one I experimented with but it wasn't necessary.

In testing this, I realized a mistake in the previous patch where a full wide group with no background had incorrect margins. The patch I just uploaded should address that.

I'm making this patch on another machine with a different local version of WordPress, so I hope it still works as it should, otherwise let me know.

#5 @kjellr
9 months ago

I gave twentynineteen-margins.1.diff a test today, and I'm still seeing the horizontal scrollbars + cut off controls on full-width blocks:

http://cldup.com/FjHSE6Hmip.png

... and also missing margins on items within Group blocks:

http://cldup.com/REKfRR-HiA.png

Standard-width paragraph inside of a wide-width Group block w/background.

@Joen: I'll see if I can dig in later today, but I also want to confirm something: Should this patch also take care of the mis-aligned title issue noted here: https://github.com/WordPress/gutenberg/issues/20067

Thanks!

#6 @Joen
9 months ago

Thanks for looking, if you have time I'd be so grateful.

Yes the ticket was intended to fix all issues, including alignments to the title. If that's off now, something must've changed since the initial patch.

@kjellr
9 months ago

#7 @kjellr
9 months ago

Just added 48526.diff, which is a bit of a redo. It tackles these issues:

  • Scrollbars + misaligned full-wide blocks on mobile.
  • Scrollbars + misaligned blocks in general on tablet.
  • Scrollbars + misaligned full-wide blocks on desktop.
  • Misaligned title on desktop.
  • Misaligned "tick" mark above title on all screen sizes.
  • Misaligned default placeholder (before you click in, after creating a new post)
  • Incorrect font on the default placeholder (before you click in, after creating a new post)

Group blocks are still not working right, so I'm going to dive into those next. It appears that perhaps the group block markup has changed, and the super-specific CSS selectors need some updates.

@kjellr
9 months ago

#8 @kjellr
9 months ago

Alright, I got those Group block issues sorted out as well. The Group block selectors did change, and that seemed to be the root of the problem. A couple small issues still remain:

  • Full-width Group blocks still add a little horizontal scrolling on mobile. I couldn't nail down why this was happening, but eliminating ~10px of the width seemed to solve it? This requires some more sleuthing.
  • The previous Group rules targeted direct children of the .wp-block-group class, to ensure that the styles were correctly inherited when there are Group blocks inside of a Group block. I was unable to do that in this case, since sometimes there was an extra is-block-content div in there somewhere. I wasn't sure what that does, or why it showed up some times but not others. @Joen do you know more about that? We can work around it either way, but it'll result in some messier CSS.

That's all I have time for this week — If someone has a moment to test these changes out, that would be excellent. 🙌

(Sidenote: The fact that the theme has to rely on so many super-specific Group block selectors is very unsustainable! There's got to be a better way...)

@kjellr
9 months ago

#9 @kjellr
9 months ago

One final thing for today — I noticed this comment in the other thread:
https://github.com/WordPress/gutenberg/issues/20067#issuecomment-582954823

I'd fixed that new left padding in the last patch, but it sounds like that's a Gutenberg bug, not a theme bug. I've removed that part from the patch in 48526.2.diff.

#10 @Joen
9 months ago

Thank you so much for taking this on, Kjell. My day is complex and my environment is acting up, but I'll try and apply the patch later today to verify it. In the mean time:

Full-width Group blocks still add a little horizontal scrolling on mobile. I couldn't nail down why this was happening, but eliminating ~10px of the width seemed to solve it? This requires some more sleuthing.

I would not spend too much effort on this:

  • It's an edgecase that should not block the other improvements from landing
  • It's possible that the issue will disappear as the block editor itself improves. Ella has been doing amazing work reducing the dom, simplifying the CSS and reducing specificity, including https://github.com/WordPress/gutenberg/pull/19912.

The previous Group rules targeted direct children of the .wp-block-group class, to ensure that the styles were correctly inherited when there are Group blocks inside of a Group block. I was unable to do that in this case, since sometimes there was an extra is-block-content div in there somewhere. I wasn't sure what that does, or why it showed up some times but not others. @Joen do you know more about that? We can work around it either way, but it'll result in some messier CSS.

This is a new one that I believe is related to floats primarily: https://github.com/WordPress/gutenberg/blob/4857ad58c1241b3d63d21a6880c989b85746c3dc/packages/block-editor/src/components/block-list/block.js#L284 — the exact nuance is a little hard to explain and I don't recall all the details. If I'm not mistaken though, that wrapping div should ONLY wrap floats. Can you elaborate how you targetted the direct descendants? I think that should probably still be the correct thing to do.

(Sidenote: The fact that the theme has to rely on so many super-specific Group block selectors is very unsustainable! There's got to be a better way...)

Yep. I think we need to revisit exact assumptions around how the group block works, and look for ways to make it generic. Some thoughts in https://github.com/WordPress/gutenberg/pull/19912#issuecomment-579101507.

I'd fixed that new left padding in the last patch, but it sounds like that's a Gutenberg bug, not a theme bug. I've removed that part from the patch in 48526.2.diff​.

Nice, yeah we need to fix that in the editor. Thank you Kjell!

#11 @jffng
9 months ago

I tested this patch against Gutenberg 7.4 and it is looking good.

Before:

https://d1sz9tkli0lfjq.cloudfront.net/items/2E1U1n3y0I081I24033w/Screen%20Shot%202020-02-07%20at%2012.22.09%20PM.png

After:

https://d1sz9tkli0lfjq.cloudfront.net/items/1a171W0h3V0w3h3p0n1E/Screen%20Shot%202020-02-07%20at%2012.22.51%20PM.png

#12 @kjellr
8 months ago

Thanks for taking a look, @Joen and @jffng!

I would not spend too much effort on this

Thanks. So to be clear for anyone testing: The only known bug with this right now is that full-width group blocks seem to have a slight horizontal scrollbar on viewports that are under 600px wide.

Can you elaborate how you targetted the direct descendants? I think that should probably still be the correct thing to do.

I think I got this one sorted out. It appears that the full-width Group block gets the additional is-is-block-content class inside it, whereas other alignments do not. I've adjusted the selectors inside of 48526.3.diff to account for that, so this should be ready for testing again.

EDIT: I noticed a small-screen issue with the post title "tick" accent, and uploaded a fresh copy of 48526.3.diff with that fixed.

Last edited 8 months ago by kjellr (previous) (diff)

@kjellr
8 months ago

#13 @jffng
8 months ago

I tested this and verified that the horizontal scrollbar bug is gone for viewports under 600px.

However, I encountered a small visual error that I wasn't immediately able to pinpoint the cause of:

Between 600px and 768px, there is a small gap on the right for the full-width group block in the editor:

https://p5.f1.n0.cdn.getcloudapp.com/items/o0uDoB19/Screen+Shot+2020-02-11+at+9.10.58+AM.jpg

Without this patch, the behavior is an overflow causing obscured block tools and the horizontal scrollbar, which seems objectively worse.

@kjellr
8 months ago

#14 @kjellr
8 months ago

I tested this and verified that the horizontal scrollbar bug is gone for viewports under 600px.

However, I encountered a small visual error that I wasn't immediately able to pinpoint the cause of:

Between 600px and 768px, there is a small gap on the right for the full-width group block in the editor:

Thanks for testing, @jffng! I just pushed 48526.4.diff, which should fix that issue. Tested in FF, Chrome, and Safari.

This patch does seem to sometimes have the horizontal scroll on screens <600px, but I can't figure out why at this point. In any case, I think we can follow up with that later on. It would be great to get this into 5.4 Beta 1, and iterate on it from there.

#15 @jffng
8 months ago

I have tested the latest patch (48526.4.diff)​ and verified on FF, Chrome, and Safari that both the horizontal scrollbar below <600px and extra right padding issue is resolved.

Thanks @kjellr!

#16 @Joen
8 months ago

Thank you all for amazing work.

I would add that perfection is not necessarily the ultimate goal here. It was quite broken after the margin refactor, so long as it's not broken, there are often situations where the issue being addressed should actually be fixed in the block editor itself, as is the case with the default appender having too much left padding. Or possibly, by the sound of it, the group block issues.

#17 @allancole
8 months ago

Yup, this works for me as well @kjellr !
No horizontal scroll bars at all on full or wide blocks:

http://cldup.com/089dozLTQ8.gif

#18 @kjellr
8 months ago

  • Keywords commit added

Excellent, thanks for testing everyone. I think this is ready to go. Would someone mind committing so it's in for 5.4 Beta 2?

#19 @ianbelanger
8 months ago

  • Focuses css added
  • Owner set to ianbelanger
  • Status changed from assigned to reviewing
  • Version set to 5.0

Reviewing for commit

#20 @ianbelanger
8 months ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 47327:

Bundled Themes: Twenty Nineteen Update margins in editor styles to address upcoming block editor margin changes.

Fixes the margins in the block editor to address recent changes in block margins.

Props Joen, SergeyBiryukov, kjellr, jffng, allancole.
Fixes #48526.

#21 @ianbelanger
8 months ago

  • Keywords fixed-major added
  • Milestone changed from 5.4 to 5.3.3
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for backport.

#22 @audrasjb
8 months ago

  • Keywords commit fixed-major removed
  • Milestone changed from 5.3.3 to 5.4
  • Resolution set to fixed
  • Status changed from reopened to closed

Moving 5.3.3 Bundled Themes tickets back to milestone 5.4 as RC1 is approaching.
Please don't move them back to 5.3.3 :-)

#23 @SergeyBiryukov
7 months ago

In 47537:

Twenty Nineteen: Update style-editor.css after [47327].

Props ianbelanger.
See #48526.

#24 @SergeyBiryukov
7 months ago

In 47538:

Twenty Nineteen: Update style-editor.css and style-rtl.css after running the build task.

Follow-up to [47327], [47339].

Props ianbelanger.
Reviewed by desrosj, SergeyBiryukov.
Merges [47536] and [47537] to the 5.4 branch.
See #48526, #49410, #49743.

Note: See TracTickets for help on using tickets.