Try: Be more specific about first/last child spacing overrides

[kjellr]

This PR solves #857 by rewriting some CSS rules that are unnecessarily global.

Currently, Twenty Twenty-One provides an override to Gutenberg's default block margins. These are used in the front end and in the editor. Here are the rules the theme currently uses in the editor:

twentytwentyone/assets/sass/05-blocks/utilities/_editor.scss

Lines 73 to 89 in 209d3b3

	/**
	* Spacing Overrides
	*/
	[data-block] {
	margin-top: var(--global--spacing-vertical);
	margin-bottom: var(--global--spacing-vertical);
	[data-block]:first-child {
	margin-top: 0;
	}
	// Needs to be the second-last child to avoid applying this to the appender.
	[data-block]:nth-last-child(2) {
	margin-bottom: 0;
	}
	}

In a handful of areas, we also want to clear the top margin of the first inner block, and the bottom margin of the last innerblock. That's what those :first-child and :nth-last-child(2) rules are for. (The :nth-last-child(2) rule is not always the correct rule to use here, but let's set that aside for the moment).

The rules we set here are actually stronger than we need. They're only necessary for a handful of blocks:

• Columns
• Cover
• Group
• Media & Text

Other blocks that accept child blocks (Social Links, Navigation) do not need these rules since they ship with their own specific styles to handle this already.

This PR removes these global :first-child and :last-child override rules, and applies them only to the blocks that needs them. That eliminates issues like #857.

It moves the :first-child and last-child into a mixin, and improves upon them too. The original rules only target he correct :last-child block when the block was selected. In it's deselected state (where there is no appender present), the rules incorrectly targeted the second-last block in the column. The new rules account for that:

twentytwentyone/assets/sass/02-tools/mixins.scss

Lines 81 to 97 in 70e6f3e

	@mixin innerblock-margin-clear() {
	// Clear the top margin for the first-child.
	> *:first-child {
	margin-top: 0;
	}
	// Last child that is not the appender.
	> *:last-child():not(.block-list-appender) {
	margin-bottom: 0;
	}
	// When selected, the last item becomes the second last because of the appender.
	&.has-child-selected > *:nth-last-child(2) {
	margin-bottom: 0;
	}
	}

[ryelle]

This isn't applied correctly to Groups, Media & Text, or Cover blocks, because the has-child-selected class is added to a parent container (ex, .wp-block-media-text, not .wp-block-media-text__content). It's the right element for a single column though, so changing this to .has-child-selected & wouldn't work across all cases (it also doesn't work for Media & Text as-is, since it becomes .has-child-selected .wp-block-media-text__content .wp-block-media-text__content ...)

This causes a pretty obvious content jump if you've got content bottom-aligned, so it would be nice to figure out something here 🤔

[kjellr]

In 37be8a3 I reworked the mixin to account for all these various containers. It mostly works, but there are two bugs:

• Sometimes the styles don't kick in immediately on click — I think this is because of a delay between when you select a block, and when .has-child-selected is added.
• Sometimes the spacing still jumps around a little bit because of the appender taking up space. This is what's happening in your GIF above).

Both of these existed before this PR, and I'm not totally sure how we'd solve them without styling the appender itself, which seems unwise.

In general, it doesn't look like Gutenberg zeroes any of these margins on its own... another route would be remove all this entirely and just let Gutenberg handle it eventually (if that ever happens). I think we're getting closer and closer to "weird css hack" territory here. 😅

[ryelle]

Yeah, I'm seeing both bugs you mentioned. The content jump-up is smaller at least, rather than the full 38px of margin + appender, it's just the appender (16px) now.

another route would be remove all this entirely and just let Gutenberg handle it eventually (if that ever happens). I think we're getting closer and closer to "weird css hack" territory here.

Yeahhh, we definitely are. If we remove the mixin, we end up with extra space between blocks - but it's the same amount of extra space, not the uneven alignment we were seeing before. It does totally get rid of the content jump.

[carolinan]

Thank you for very thorough descriptions and testing!

It sounds like removing the margin styles that affects the first and last child is the best solution?

[kjellr]

It sounds like removing the margin styles that affects the first and last child is the best solution?

Sort of. Technically it is: it would mean we're doing the least amount of hacky CSS to solve this in the theme.

But from a users + design perspective, removing all these rules is less than ideal. It means there's always some extra padding above and below inner blocks.

I pushed 67894cf, which removes it all so we can see what that's like. Here are a few examples:

Adjusted margins (as implemented today)	With our adjusted margins removed

cc @melchoyce for some thoughts here. Also @jasmussen, in case you have perspective on if it's been considered to have these margins fixed in core.

[melchoyce]

Just visually, I prefer the even spacing of the left column.

[jasmussen]

Thank you for the ping, thank you for the PR!

It's unfortunate that everything that registers itself as a block inherits intrinsic margins, and this is hopefully something the global styles project can revisit.

The tricky part about overriding the margins with high specificity is that some blocks, like core/column (singular) should never have margins, as it's just a container. And for horizontal blocks like Buttons, Social Links, Navigation, the top and bottom margins of the first and last children should not differ.

That table with screenshot is really helpful — eliminating that extra top and bottom padding, is that the primary purpose of those rules? If yes, then I can probably push a PR to address that directly in the block editor, so we can get the left picture for all themes. It won't make it to 5.6, but it will land in the plugin and obviously 5.7. If acceptable, this will likely save you some headaches.

[kjellr]

That table with screenshot is really helpful — eliminating that extra top and bottom padding, is that the primary purpose of those rules? If yes, then I can probably push a PR to address that directly in the block editor, so we can get the left picture for all themes. It won't make it to 5.6, but it will land in the plugin and obviously 5.7. If acceptable, this will likely save you some headaches.

Yeah, that's the only purpose of all these rules. Getting this fixed in Gutenberg would be great. Especially with block-based themes in the pipeline — this isn't the sort of thing those themes should be trying to solve for.

I think that rules for this in Gutenberg would end up having a higher specificity than the standard [data-block] [data-block] spacing rules we use to override the custom spacing. So that should theoretically work fine.

[kjellr]

I'm a little torn about what to do in the meantime, before a PR like that would land. (Presumably it wouldn't be ready for the 5.6 release).

Should we merge the changes as they exited here? And then just strip them out if/when core fixes this upstream? It's not perfect, but it does significantly improve the appearance of these blocks.

[carolinan]

With the larger, original editor margins, do we avoid the content jump? And the weird CSS additions for the social links block? Personally the content jump with the appender bothers me more than extra margins..

Extra space does not interrupt my focus when editing, but moving items do interrupt (One of the reasons why I am having so much trouble with the new link box for the buttons block...).

[jasmussen]

Adjusting the top and bottom margins of the first and last blocks in a container is likely to affect any block, present or future (including from plugins), that uses a horizontal mover control. In those containers, blocks are laid out side by side vs. in a vertical sequence.

If the first-child/last-child rules are removed (which would cause the larger margins shown on the right side) the first or last blocks in a horizontal container should not jump, layout wise.

[kjellr]

I took a little more time to think about the changes here, and have a new recommendation.

I echo @melchoyce's thoughts above — I definitely prefer the balanced margins to the unedited ones. Earlier in this PR, I had a very close version that achieved this, but had two "jumping" issues:

• Sometimes the styles didn't kick in immediately on click — I think this is because of a delay between when you select a block, and when .has-child-selected is added.
• The spacing jumped around a little bit because of the appender taking up space. This is what's happening in your GIF above).

I'm unable to reproduce the first bullet anymore. 🤔 Not sure if it's something about my setup that was causing it in the past?

As for the second bullet: I had a slack chat with @jasmussen about it, and he noted that the appender is not supposed to take up any space when it's invisible. So that's actually a Gutenberg bug that he will open a PR to fix.

Given that, I think our best bet for now is to keep these margin adjustments, along with the cleanup that I've done in this PR. It solves the issue in #857, while still zero-ing out the margins of the appropriate children on the cover, media & text, column, and group blocks. Here's a GIF of the current state of the PR, with these changes reinstated:

The only issue there is that jump, which will be fixed upstream in Gutenberg. Considering that, this seems like a decent approach for now.

(It's also only a small fix to what we had, which feels like a reasonable change to make during RC).

[jasmussen]

the appender is not supposed to take up any space when it's invisible. So that's actually a Gutenberg bug that he will open a PR to fix.

The only way we can remove the very real theme headaches that stem from the block editor, is to address them in the block editor, so I would appreciate if you avoid painting them over, but instead ping me about it. Most of the time it's easier to fix than you think. In this case, simply applying a float: left; to the appender seems to do what it needs to. But I need to test that and then I'll fix that directly in the block editor.

Thank you Kjell!

[ryelle]

I am still seeing the content jump - I think you're right that it's a delay in the .has-child-selected class, and it's more obvious if your browser is running slower. I did notice that I can trigger an in-between state by selecting the parent block (cover, in this case). You see first I click to select the Cover itself, and the bottom margin is not cleared, then I click into the paragraph and it applies has-child-selected to clear the margin. Seems to happen across all but columns.

Overall, I agree this is a good approach, and with the one suggestion I think this is good to merge.

[kjellr]

Awesome, thanks for the review and suggestion, @ryelle! That all sounds good.

[jasmussen]

This PR fixes the jump caused by the appender. CC: @ryelle @carolinan

It probably won't be backported, but it will land in the plugin, and it means eventually the jump will disappear. Hopefully that helps a little.