Try: Improve typographic rhythm

[jasmussen]

This PR is intended to explore adopting a change that is being explored for mobile Gutenberg in wordpress-mobile/gutenberg-mobile#550. It simply changes the lineheight from 1.8 to 1.6.

Before:

After:

It is very important to note that this change affects only the vanilla Gutenberg style — themes can, through editor styles, still customize this to their hearts content.

Thoughts?

CC: @iamthomasbishop

[kjellr]

I know I'm the one who pushed towards 1.6 in the other thread, but now that I'm using this more I'm changing my mind. 😄

When first loading up this PR and tested on desktop, I was using Twenty Nineteen. Twenty Nineteen uses a line height of 1.8, but did not actually hardcode that into the editor styles because Gutenberg traditionally uses that for paragraph text too (Note: we should hardcode this into Twenty Nineteen. 🙂).

So when loading up this PR, I saw the 1.6 line height applied to Twenty Nineteen's 22px tall text. I could tell the line height was shorter, but it felt okay. When I switched back over to a theme that doesn't ship with editor styles and looked again... things felt less great. With 16px text, that line height felt too tight to me.

After a bit more testing, here's my best guess on why: The difference lies in the number of characters per line. When using a mobile device (or a larger font size), I can afford to have a bit of a shorter line height: My eyes don't have to travel too far to get from one end of the screen to the other, so I can more readily keep track of which line I'm coming from and which line I'm going to. In fact, since I'm switching from line to line so much more frequently in this case, saving a few pixels of height is actually appreciated, as it means my eyes won't have to jump as much down the page each time.

When testing this on desktop however, my eyes have to travel a much longer distance to get from the end of one line back to the beginning of the next. In this case, my eyes will benefit from a bit more separation, otherwise I the multiple lines of text will bleed together, and I'll lose my place.

That's likely a lot of overthinking for a change that impacts a few pixels. I'd welcome some other input, but for now I'm not sure about this.

[jasmussen]

I don't think you're overthinking it. It's a bit change.

I would also argue that we should consider this mostly an issue for the vanilla styles. If this ships, for TwentyNineteen let's just push a patch to the editor style to add the line-height.

For the vanilla styles, I don't have stong opinions personally. I can see an argument for changing the lineheight beyond the mobile breakpoint, but perhaps that's added complexity for something that supposed to be vanilla? There's also a ticket to remove the Noto Serif font in favor of system fonts which means we'll likely end up looking at Georgia at some point (I know 😢) — does that change anything?

[jasmussen]

Also @iamthomasbishop again because ☝

[kjellr]

I would also argue that we should consider this mostly an issue for the vanilla styles. If this ships, for TwentyNineteen let's just push a patch to the editor style to add the line-height.

Definitely. This should be patched either way, just to be safe.

There's also a ticket to remove the Noto Serif font in favor of system fonts which means we'll likely end up looking at Georgia at some point (I know 😢) — does that change anything?

Yeah, we'd probably want to re-assess than too.

[kjellr]

If this ships, for TwentyNineteen let's just push a patch to the editor style to add the line-height.

I just looked into this, and Twenty Nineteen does assign a line height to the editor-styles-wrapper that's intended to be used everywhere, but it is overridden by the line height that Gutenberg assigns to .editor-styles-wrapper .mce-content-body.

gutenberg/packages/editor/src/editor-styles.scss

Lines 37 to 39 in 1a7354c

	.mce-content-body {
	line-height: $editor-line-height;
	}

This overwrites even styles applied to .editor-styles-wrapper p for instance. In order to fix this, we'd need to specify a line height for .mce-content-body inside of the theme's editor stylesheet. This seems wrong to me... if theme authors assign a global line height in their editor stylesheet, it'll shouldn't be overwritten.

Any idea why that style is located where it is?

[iamthomasbishop]

I agree, I don't think you're overthinking it, @kjellr .

When using a mobile device (or a larger font size), I can afford to have a bit of a shorter line height: My eyes don't have to travel too far to get from one end of the screen to the other, so I can more readily keep track of which line I'm coming from and which line I'm going to

Do you think it's worth limiting this to narrow breakpoints? Or would that not be worth the added complexity? While I originally proposed the line-height change as part of native-mobile GB, I think whatever solution we decide should align to all "small" breakpoints – aka between "vanilla" core GB on mobile web and native-mobile GB.

If that means it's 1.6 on smaller sizes and 1.8 on larger, maybe that's worth the trade-off, because it would feel more natural and readable on smaller devices. Although, I can understand the concern about adding complexity in terms of code maintainability and flexibility with themes. I'm not as well-versed in themes and editor-styles as y'all but I would expect custom editor styles to easily be able to override this "vanilla" declaration.

There's also a ticket to remove the Noto Serif font in favor of system fonts which means we'll likely end up looking at Georgia at some point

NOPE 😆 In all seriousness, if any change in font-face does happen we'd definitely want to re-asses line-height.

[iamthomasbishop]

Also something worth noting that I just remembered: The heading sizes on native-mobile GB are also smaller – so that will make a difference when looking at relative line-heights and font-sizes.

[jasmussen]

I just looked into this, and Twenty Nineteen does assign a line height to the editor-styles-wrapper that's intended to be used everywhere, but it is overridden by the line height that Gutenberg assigns to .editor-styles-wrapper .mce-content-body.

Hmmmm good question, and excellent catch. I just looked at the code, and it looks like mce-content-body was renamed in the last few days to editor-rich-text__editable, which gives me a clue that we might have added so much specificity to this back in the day before the editor styles wrapper was even a thing.

At a casual glance it looks like removing that extra specificity is totally fine, so we should absolutely do that in a separate PR — do you have bandwidth? I'm a bit pressed.

Important to make sure to test with a range of themes, and notably with the classic block, to make sure this one inherits correctly.
Also be sure to test with a range of blocks and even captions.

---

Back to the issue at hand, the lineheight being too tight. Given that Noto Serif might go (I can't recall the core trac ticket URL, but the reasoning to move to system fonts is around localization, which is fair enough especially for the vanilla styles), should we punt further work on this PR for now, and reassess at a later time?

[kjellr]

At a casual glance it looks like removing that extra specificity is totally fine, so we should absolutely do that in a separate PR — do you have bandwidth? I'm a bit pressed.

I just took a quick look and ended up diving down a confusing rabbit hole. From what I can tell, this rule is no longer being used at all in master. So it's probably safe to remove, but I'll do a bit more digging.

In any case, I noticed that this line height issue was reported in #10067 too, so there's an existing issue for it. 👍

Back to the issue at hand, the lineheight being too tight. Given that Noto Serif might go (I can't recall the core trac ticket URL, but the reasoning to move to system fonts is around localization, which is fair enough especially for the vanilla styles), should we punt further work on this PR for now, and reassess at a later time?

Yep, I think punting this one is fine. For quick reference, the ticket about font updates is: https://core.trac.wordpress.org/ticket/46169

[jasmussen]

I'm closing this one for now. We can always reopen or revisit.