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 using a user-specific variable for blockGap #36680

Closed
wants to merge 2 commits into from

Conversation

richtabor
Copy link
Member

@richtabor richtabor commented Nov 19, 2021

Description

As reported in #36521, there's a bug with custom blockGap values overriding the margin-top applied to a block.

This proposed solution adds a CSS property --wp--style--block-gap--self that is only used if a custom blockGap exists. This allows the original --wp--style--block-gap to still function properly as a top-level vertical spacing mechanism between blocks, while also allowing users to customize the blockGap of a block, without inadvertently affecting the custom value affecting that block's top-level vertical spacing.

How has this been tested?

Tested with blocks that support blockGap (Buttons, Columns, Social Links, Navigation).

Screenshots

Editor, no blockGap assigned:
Screen Shot 2021-11-19 at 1 39 36 PM

Front-end, no blockGap assigned:
Screen Shot 2021-11-19 at 1 39 51 PM

Editor, custom blockGap:
Screen Shot 2021-11-19 at 1 40 59 PM

Front-end, custom blockGap:
Screen Shot 2021-11-19 at 1 41 07 PM

Types of changes

Bug fix

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • I've tested my changes with keyboard and screen readers.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).

@github-actions
Copy link

github-actions bot commented Nov 19, 2021

Size Change: +222 B (0%)

Total Size: 1.1 MB

Filename Size Change
build/block-editor/index.min.js 139 kB +9 B (0%)
build/block-library/blocks/button/style-rtl.css 569 B +9 B (+2%)
build/block-library/blocks/button/style.css 569 B +9 B (+2%)
build/block-library/blocks/columns/editor-rtl.css 216 B +10 B (+5%) 🔍
build/block-library/blocks/columns/editor.css 214 B +9 B (+4%)
build/block-library/blocks/columns/style-rtl.css 515 B +12 B (+2%)
build/block-library/blocks/columns/style.css 514 B +12 B (+2%)
build/block-library/blocks/navigation/style-rtl.css 1.65 kB +10 B (+1%)
build/block-library/blocks/navigation/style.css 1.64 kB +10 B (+1%)
build/block-library/editor-rtl.css 9.86 kB +7 B (0%)
build/block-library/editor.css 9.86 kB +8 B (0%)
build/block-library/style-rtl.css 10.5 kB +23 B (0%)
build/block-library/style.css 10.5 kB +24 B (0%)
build/blocks/index.min.js 46.3 kB +3 B (0%)
build/components/index.min.js 214 kB +89 B (0%)
build/components/style-rtl.css 15.3 kB -11 B (0%)
build/components/style.css 15.3 kB -11 B (0%)
ℹ️ View Unchanged
Filename Size
build/a11y/index.min.js 960 B
build/admin-manifest/index.min.js 1.1 kB
build/annotations/index.min.js 2.75 kB
build/api-fetch/index.min.js 2.21 kB
build/autop/index.min.js 2.12 kB
build/blob/index.min.js 459 B
build/block-directory/index.min.js 6.28 kB
build/block-directory/style-rtl.css 1.01 kB
build/block-directory/style.css 1.01 kB
build/block-editor/default-editor-styles-rtl.css 378 B
build/block-editor/default-editor-styles.css 378 B
build/block-editor/style-rtl.css 14.4 kB
build/block-editor/style.css 14.4 kB
build/block-library/blocks/archives/editor-rtl.css 61 B
build/block-library/blocks/archives/editor.css 60 B
build/block-library/blocks/archives/style-rtl.css 65 B
build/block-library/blocks/archives/style.css 65 B
build/block-library/blocks/audio/editor-rtl.css 58 B
build/block-library/blocks/audio/editor.css 58 B
build/block-library/blocks/audio/style-rtl.css 111 B
build/block-library/blocks/audio/style.css 111 B
build/block-library/blocks/audio/theme-rtl.css 125 B
build/block-library/blocks/audio/theme.css 125 B
build/block-library/blocks/block/editor-rtl.css 161 B
build/block-library/blocks/block/editor.css 161 B
build/block-library/blocks/button/editor-rtl.css 470 B
build/block-library/blocks/button/editor.css 470 B
build/block-library/blocks/buttons/editor-rtl.css 291 B
build/block-library/blocks/buttons/editor.css 291 B
build/block-library/blocks/buttons/style-rtl.css 275 B
build/block-library/blocks/buttons/style.css 275 B
build/block-library/blocks/calendar/style-rtl.css 207 B
build/block-library/blocks/calendar/style.css 207 B
build/block-library/blocks/categories/editor-rtl.css 84 B
build/block-library/blocks/categories/editor.css 83 B
build/block-library/blocks/categories/style-rtl.css 79 B
build/block-library/blocks/categories/style.css 79 B
build/block-library/blocks/code/style-rtl.css 90 B
build/block-library/blocks/code/style.css 90 B
build/block-library/blocks/code/theme-rtl.css 134 B
build/block-library/blocks/code/theme.css 134 B
build/block-library/blocks/cover/editor-rtl.css 546 B
build/block-library/blocks/cover/editor.css 547 B
build/block-library/blocks/cover/style-rtl.css 1.19 kB
build/block-library/blocks/cover/style.css 1.19 kB
build/block-library/blocks/embed/editor-rtl.css 488 B
build/block-library/blocks/embed/editor.css 488 B
build/block-library/blocks/embed/style-rtl.css 417 B
build/block-library/blocks/embed/style.css 417 B
build/block-library/blocks/embed/theme-rtl.css 124 B
build/block-library/blocks/embed/theme.css 124 B
build/block-library/blocks/file/editor-rtl.css 300 B
build/block-library/blocks/file/editor.css 300 B
build/block-library/blocks/file/style-rtl.css 255 B
build/block-library/blocks/file/style.css 255 B
build/block-library/blocks/file/view.min.js 322 B
build/block-library/blocks/freeform/editor-rtl.css 2.44 kB
build/block-library/blocks/freeform/editor.css 2.44 kB
build/block-library/blocks/gallery/editor-rtl.css 977 B
build/block-library/blocks/gallery/editor.css 982 B
build/block-library/blocks/gallery/style-rtl.css 1.62 kB
build/block-library/blocks/gallery/style.css 1.62 kB
build/block-library/blocks/gallery/theme-rtl.css 122 B
build/block-library/blocks/gallery/theme.css 122 B
build/block-library/blocks/group/editor-rtl.css 159 B
build/block-library/blocks/group/editor.css 159 B
build/block-library/blocks/group/style-rtl.css 57 B
build/block-library/blocks/group/style.css 57 B
build/block-library/blocks/group/theme-rtl.css 78 B
build/block-library/blocks/group/theme.css 78 B
build/block-library/blocks/heading/style-rtl.css 114 B
build/block-library/blocks/heading/style.css 114 B
build/block-library/blocks/html/editor-rtl.css 332 B
build/block-library/blocks/html/editor.css 333 B
build/block-library/blocks/image/editor-rtl.css 731 B
build/block-library/blocks/image/editor.css 730 B
build/block-library/blocks/image/style-rtl.css 507 B
build/block-library/blocks/image/style.css 511 B
build/block-library/blocks/image/theme-rtl.css 124 B
build/block-library/blocks/image/theme.css 124 B
build/block-library/blocks/latest-comments/style-rtl.css 284 B
build/block-library/blocks/latest-comments/style.css 284 B
build/block-library/blocks/latest-posts/editor-rtl.css 137 B
build/block-library/blocks/latest-posts/editor.css 137 B
build/block-library/blocks/latest-posts/style-rtl.css 528 B
build/block-library/blocks/latest-posts/style.css 527 B
build/block-library/blocks/list/style-rtl.css 94 B
build/block-library/blocks/list/style.css 94 B
build/block-library/blocks/media-text/editor-rtl.css 266 B
build/block-library/blocks/media-text/editor.css 263 B
build/block-library/blocks/media-text/style-rtl.css 493 B
build/block-library/blocks/media-text/style.css 490 B
build/block-library/blocks/more/editor-rtl.css 431 B
build/block-library/blocks/more/editor.css 431 B
build/block-library/blocks/navigation-link/editor-rtl.css 649 B
build/block-library/blocks/navigation-link/editor.css 650 B
build/block-library/blocks/navigation-link/style-rtl.css 94 B
build/block-library/blocks/navigation-link/style.css 94 B
build/block-library/blocks/navigation-submenu/editor-rtl.css 299 B
build/block-library/blocks/navigation-submenu/editor.css 299 B
build/block-library/blocks/navigation-submenu/view.min.js 343 B
build/block-library/blocks/navigation/editor-rtl.css 1.89 kB
build/block-library/blocks/navigation/editor.css 1.89 kB
build/block-library/blocks/navigation/view.min.js 2.74 kB
build/block-library/blocks/nextpage/editor-rtl.css 395 B
build/block-library/blocks/nextpage/editor.css 395 B
build/block-library/blocks/page-list/editor-rtl.css 377 B
build/block-library/blocks/page-list/editor.css 377 B
build/block-library/blocks/page-list/style-rtl.css 172 B
build/block-library/blocks/page-list/style.css 172 B
build/block-library/blocks/paragraph/editor-rtl.css 157 B
build/block-library/blocks/paragraph/editor.css 157 B
build/block-library/blocks/paragraph/style-rtl.css 273 B
build/block-library/blocks/paragraph/style.css 273 B
build/block-library/blocks/post-author/style-rtl.css 175 B
build/block-library/blocks/post-author/style.css 176 B
build/block-library/blocks/post-comments-form/style-rtl.css 444 B
build/block-library/blocks/post-comments-form/style.css 444 B
build/block-library/blocks/post-comments/style-rtl.css 492 B
build/block-library/blocks/post-comments/style.css 493 B
build/block-library/blocks/post-excerpt/editor-rtl.css 73 B
build/block-library/blocks/post-excerpt/editor.css 73 B
build/block-library/blocks/post-excerpt/style-rtl.css 69 B
build/block-library/blocks/post-excerpt/style.css 69 B
build/block-library/blocks/post-featured-image/editor-rtl.css 771 B
build/block-library/blocks/post-featured-image/editor.css 771 B
build/block-library/blocks/post-featured-image/style-rtl.css 153 B
build/block-library/blocks/post-featured-image/style.css 153 B
build/block-library/blocks/post-template/editor-rtl.css 99 B
build/block-library/blocks/post-template/editor.css 98 B
build/block-library/blocks/post-template/style-rtl.css 391 B
build/block-library/blocks/post-template/style.css 392 B
build/block-library/blocks/post-terms/style-rtl.css 73 B
build/block-library/blocks/post-terms/style.css 73 B
build/block-library/blocks/post-title/style-rtl.css 80 B
build/block-library/blocks/post-title/style.css 80 B
build/block-library/blocks/preformatted/style-rtl.css 103 B
build/block-library/blocks/preformatted/style.css 103 B
build/block-library/blocks/pullquote/editor-rtl.css 198 B
build/block-library/blocks/pullquote/editor.css 198 B
build/block-library/blocks/pullquote/style-rtl.css 378 B
build/block-library/blocks/pullquote/style.css 378 B
build/block-library/blocks/pullquote/theme-rtl.css 167 B
build/block-library/blocks/pullquote/theme.css 167 B
build/block-library/blocks/query-pagination-numbers/editor-rtl.css 122 B
build/block-library/blocks/query-pagination-numbers/editor.css 121 B
build/block-library/blocks/query-pagination/editor-rtl.css 262 B
build/block-library/blocks/query-pagination/editor.css 255 B
build/block-library/blocks/query-pagination/style-rtl.css 234 B
build/block-library/blocks/query-pagination/style.css 231 B
build/block-library/blocks/query/editor-rtl.css 131 B
build/block-library/blocks/query/editor.css 132 B
build/block-library/blocks/quote/style-rtl.css 187 B
build/block-library/blocks/quote/style.css 187 B
build/block-library/blocks/quote/theme-rtl.css 223 B
build/block-library/blocks/quote/theme.css 226 B
build/block-library/blocks/rss/editor-rtl.css 202 B
build/block-library/blocks/rss/editor.css 204 B
build/block-library/blocks/rss/style-rtl.css 289 B
build/block-library/blocks/rss/style.css 288 B
build/block-library/blocks/search/editor-rtl.css 165 B
build/block-library/blocks/search/editor.css 165 B
build/block-library/blocks/search/style-rtl.css 397 B
build/block-library/blocks/search/style.css 398 B
build/block-library/blocks/search/theme-rtl.css 64 B
build/block-library/blocks/search/theme.css 64 B
build/block-library/blocks/separator/editor-rtl.css 99 B
build/block-library/blocks/separator/editor.css 99 B
build/block-library/blocks/separator/style-rtl.css 245 B
build/block-library/blocks/separator/style.css 245 B
build/block-library/blocks/separator/theme-rtl.css 172 B
build/block-library/blocks/separator/theme.css 172 B
build/block-library/blocks/shortcode/editor-rtl.css 474 B
build/block-library/blocks/shortcode/editor.css 474 B
build/block-library/blocks/site-logo/editor-rtl.css 770 B
build/block-library/blocks/site-logo/editor.css 770 B
build/block-library/blocks/site-logo/style-rtl.css 165 B
build/block-library/blocks/site-logo/style.css 165 B
build/block-library/blocks/site-tagline/editor-rtl.css 86 B
build/block-library/blocks/site-tagline/editor.css 86 B
build/block-library/blocks/site-title/editor-rtl.css 84 B
build/block-library/blocks/site-title/editor.css 84 B
build/block-library/blocks/social-link/editor-rtl.css 177 B
build/block-library/blocks/social-link/editor.css 177 B
build/block-library/blocks/social-links/editor-rtl.css 670 B
build/block-library/blocks/social-links/editor.css 669 B
build/block-library/blocks/social-links/style-rtl.css 1.32 kB
build/block-library/blocks/social-links/style.css 1.32 kB
build/block-library/blocks/spacer/editor-rtl.css 307 B
build/block-library/blocks/spacer/editor.css 307 B
build/block-library/blocks/spacer/style-rtl.css 48 B
build/block-library/blocks/spacer/style.css 48 B
build/block-library/blocks/table/editor-rtl.css 471 B
build/block-library/blocks/table/editor.css 472 B
build/block-library/blocks/table/style-rtl.css 481 B
build/block-library/blocks/table/style.css 481 B
build/block-library/blocks/table/theme-rtl.css 188 B
build/block-library/blocks/table/theme.css 188 B
build/block-library/blocks/tag-cloud/style-rtl.css 146 B
build/block-library/blocks/tag-cloud/style.css 146 B
build/block-library/blocks/template-part/editor-rtl.css 560 B
build/block-library/blocks/template-part/editor.css 559 B
build/block-library/blocks/template-part/theme-rtl.css 101 B
build/block-library/blocks/template-part/theme.css 101 B
build/block-library/blocks/text-columns/editor-rtl.css 95 B
build/block-library/blocks/text-columns/editor.css 95 B
build/block-library/blocks/text-columns/style-rtl.css 166 B
build/block-library/blocks/text-columns/style.css 166 B
build/block-library/blocks/verse/style-rtl.css 87 B
build/block-library/blocks/verse/style.css 87 B
build/block-library/blocks/video/editor-rtl.css 569 B
build/block-library/blocks/video/editor.css 570 B
build/block-library/blocks/video/style-rtl.css 173 B
build/block-library/blocks/video/style.css 173 B
build/block-library/blocks/video/theme-rtl.css 124 B
build/block-library/blocks/video/theme.css 124 B
build/block-library/common-rtl.css 815 B
build/block-library/common.css 812 B
build/block-library/index.min.js 162 kB
build/block-library/reset-rtl.css 474 B
build/block-library/reset.css 474 B
build/block-library/theme-rtl.css 672 B
build/block-library/theme.css 677 B
build/block-serialization-default-parser/index.min.js 1.09 kB
build/block-serialization-spec-parser/index.min.js 2.79 kB
build/compose/index.min.js 10.9 kB
build/core-data/index.min.js 13.2 kB
build/customize-widgets/index.min.js 11.4 kB
build/customize-widgets/style-rtl.css 1.5 kB
build/customize-widgets/style.css 1.49 kB
build/data-controls/index.min.js 631 B
build/data/index.min.js 7.47 kB
build/date/index.min.js 31.5 kB
build/deprecated/index.min.js 485 B
build/dom-ready/index.min.js 304 B
build/dom/index.min.js 4.5 kB
build/edit-navigation/index.min.js 16 kB
build/edit-navigation/style-rtl.css 3.76 kB
build/edit-navigation/style.css 3.76 kB
build/edit-post/classic-rtl.css 492 B
build/edit-post/classic.css 494 B
build/edit-post/index.min.js 29.6 kB
build/edit-post/style-rtl.css 7.1 kB
build/edit-post/style.css 7.09 kB
build/edit-site/index.min.js 30.8 kB
build/edit-site/style-rtl.css 6.29 kB
build/edit-site/style.css 6.28 kB
build/edit-widgets/index.min.js 16.5 kB
build/edit-widgets/style-rtl.css 4.18 kB
build/edit-widgets/style.css 4.18 kB
build/editor/index.min.js 37.8 kB
build/editor/style-rtl.css 3.78 kB
build/editor/style.css 3.77 kB
build/element/index.min.js 3.29 kB
build/escape-html/index.min.js 517 B
build/format-library/index.min.js 6.57 kB
build/format-library/style-rtl.css 571 B
build/format-library/style.css 571 B
build/hooks/index.min.js 1.63 kB
build/html-entities/index.min.js 424 B
build/i18n/index.min.js 3.71 kB
build/is-shallow-equal/index.min.js 501 B
build/keyboard-shortcuts/index.min.js 1.8 kB
build/keycodes/index.min.js 1.39 kB
build/list-reusable-blocks/index.min.js 1.86 kB
build/list-reusable-blocks/style-rtl.css 838 B
build/list-reusable-blocks/style.css 838 B
build/media-utils/index.min.js 2.92 kB
build/notices/index.min.js 925 B
build/nux/index.min.js 2.08 kB
build/nux/style-rtl.css 747 B
build/nux/style.css 743 B
build/plugins/index.min.js 1.84 kB
build/primitives/index.min.js 924 B
build/priority-queue/index.min.js 582 B
build/react-i18n/index.min.js 671 B
build/redux-routine/index.min.js 2.65 kB
build/reusable-blocks/index.min.js 2.22 kB
build/reusable-blocks/style-rtl.css 256 B
build/reusable-blocks/style.css 256 B
build/rich-text/index.min.js 11 kB
build/server-side-render/index.min.js 1.57 kB
build/shortcode/index.min.js 1.49 kB
build/token-list/index.min.js 639 B
build/url/index.min.js 1.9 kB
build/viewport/index.min.js 1.05 kB
build/warning/index.min.js 248 B
build/widgets/index.min.js 7.15 kB
build/widgets/style-rtl.css 1.16 kB
build/widgets/style.css 1.16 kB
build/wordcount/index.min.js 1.04 kB

compressed-size-action

@richtabor
Copy link
Member Author

richtabor commented Nov 19, 2021

Same test, with Social Links (showing how the custom blockGap does not also add vertical spacing)
Screen Shot 2021-11-19 at 1 57 52 PM

@ZebulanStanphill ZebulanStanphill added the [Type] Bug An existing feature does not function as intended label Nov 19, 2021
@ndiego
Copy link
Member

ndiego commented Nov 19, 2021

@richtabor I think this approach works well, but an audit of all blocks will be needed if this direction is approved by the rest of the team. I honestly cannot think of a better way to handle this and have been racking my brain for the past few days.

I just did a bunch of testing and it looks great. I did find the Navigation block CSS will also need to be updated:

// Menu items with no background.
, and there may be a few more. If I find any, I will report here.

@ndiego
Copy link
Member

ndiego commented Nov 19, 2021

Confirmed Navigation block works as expected now.

@youknowriad
Copy link
Contributor

This is a bit unclear to me, can you share examples of theme.json and what output do you get?
If I'm reading the code properly --wp--style--block-gap seem to be replaced by --wp--style--block-gap--self during the generation of styles but it's unclear where the former is set?

@ndiego
Copy link
Member

ndiego commented Nov 23, 2021

The --wp--style--block-gap is currently set by the "Block Spacing" setting on Columns blocks, Navigation blocks, etc. The theme.json blockGap setting works as intended in this setting.

Here is an example of a Columns block with the "Block Spacing" set to 20px. The --wp--style--block-gap variable is reset to 20px, which overwrites the value set in theme.json for the selected Columns block. Since blockGap adds a top-margin to all blocks, this is also reset to 20px.
image

Copy link
Contributor

@andrewserong andrewserong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for exploring this @richtabor, it's a really compelling idea! It's testing well for me when applied directly to the block within the editor, but I was wondering what the expected behaviour should be in theme.json when the blockGap is set for a particular block. For example, with this PR applied, if I set the following in my theme.json for just the Columns block:

	"styles": {
		"blocks": {
			"core/columns": {
				"spacing": {
					"blockGap": "6rem"
				}
			},

Then it results in the following styles being generated server-side:

.wp-block-columns {
  --wp--style--block-gap: 6rem;
}

That results in the same visual issue where the value is also being applied to the top margin:

image

Should this also be --wp--style--block-gap--self?

It also looks like this change has removed Block spacing from global styles for the blocks that have opted in. For example, go to the site editor, and select the columns block:

Missing block spacing input in global styles The block spacing input on trunk
image image

Could this be related to the change in api/constants.js?

Overall, though, I quite like the addition of the new CSS variable to work around the margin issue. @ramonjd also has an idea for adding in additional CSS variables for separate axial (horizontal/vertical) controls of block spacing in #35454 — this might be a question more for that PR, but if we add the additional self CSS variable, will that mean we need to create an additional self set or row / column variables for the axial support, too? The issue in #36521 is more pressing than axial support, though, so this consideration might not be a blocker.

@jasmussen
Copy link
Contributor

Thanks a bunch for the PR. I'm a little unclear on whether it's a good idea to add this separate value, though. Is there any way we can lower the specificity of the block gaps so the margins take effect instead?

CC: @scruffian @MaggieCabrera in case they have thoughts on it from a themer perspective.

@@ -133,7 +133,7 @@ export default {
display: flex;
gap: ${
hasBlockGapStylesSupport
? 'var( --wp--style--block-gap, 0.5em )'
? 'var(--wp--style--block-gap--self, var(--wp--style--block-gap, 0.5em))'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also update flow.js, why is this only applied to flex.js

@youknowriad
Copy link
Contributor

It's really still unclear to me how this works, If all styles are updated to be: use self variable otherwise If not defined use same variable without self.

And previously when a block selected a block gap (in the sidebar), it was affecting the latter variable but now it generates the latter, it seems this won't have any impact on the behavior unless there's something I'm missing?

In both situations, the applied gap will apply to that bock and all its children since that new variable has more priority anyway?

@richtabor
Copy link
Member Author

It's really still unclear to me how this works

What's happening is that the blockGap value from theme.json is being set inline on the parent block div, which then affects how much margin-top is added to the block, per how blockGap works with vertical spacing throughout the editor.

Instead, the user-generated value of blockGap on a specific block should only affect the space between blocks within itself — not the vertical space of the block itself.

So what this PR aims to do is try adding a CSS property fallback method that will allow the user-generated value to function within the block (falling back to the standard blockGap value) while not interfering with the blockGap set by the theme/Gutenberg. Otherwise, the blockGap set within the control also affects the vertical spacing applied to the block.

@richtabor
Copy link
Member Author

And previously when a block selected a block gap (in the sidebar), it was affecting the latter variable but now it generates the latter, it seems this won't have any impact on the behavior unless there's something I'm missing?

The key is that in the current version of Gutenberg, the user's blockGap value is applied as an inline CSS property that the following CSS inherits (resulting in the block having the blockGap value applied to the vertical space of the block, along with the intended actual "gap".

.wp-site-blocks > * + * {
    margin-top: var( --wp--style--block-gap );
}

@richtabor
Copy link
Member Author

Is there any way we can lower the specificity of the block gaps so the margins take effect instead?

@jasmussen Outside of this PR, we're currently overriding the --wp--style--block-gap CSS property inline on the block (which is causing that value to be applied to the space above this block as well).

The intent behind this PR is to that CSS property used, unless there is a user-generated value, (which I named --self for this PR). Having the CSS property not directly override the --wp--style--block-gap resolves the unintended affect of applying that value to the margin-top of this block.

<div style="--wp--style--block-gap: 100px" class="wp-block-columns has-primary-background-color has-background">
    <div class="wp-block-column"></div>
    <div class="wp-block-column"></div>
</div>

So with that markup above, the blockGap control has also set the Columns block to be 100px below the block above it:

.wp-site-blocks > * + * {
    margin-top: var( --wp--style--block-gap );
}

@richtabor
Copy link
Member Author

Overall, though, I quite like the addition of the new CSS variable to work around the margin issue. @ramonjd also has an idea for adding in additional CSS variables for separate axial (horizontal/vertical) controls of block spacing in #35454

I am curious if axial support would also resolve this particular issue though — as long as neither CSS property would be used to apply the vertical spacing between block as --wp--style--block-gap does today (or this issue remains).

@youknowriad
Copy link
Contributor

youknowriad commented Nov 23, 2021

The key is that in the current version of Gutenberg, the user's blockGap value is applied as an inline CSS property that the following CSS inherits (resulting in the block having the blockGap value applied to the vertical space of the block, along with the intended actual "gap".

@richtabor Oh I think I understand here, can we just update the block support do generate this instead:

.somecontainer > * { --wp--style--block-gap: value } ?

@richtabor
Copy link
Member Author

@richtabor Oh I think I understand here, can we just update the block support do generate this instead:

That may work — so when a block has a custom blockGap value, the direct children will inherit that value (i.e. the column blocks within the columns block, but not the blocks within the column?

@youknowriad
Copy link
Contributor

That may work — so when a block has a custom blockGap value, the direct children will inherit that value (i.e. the column blocks within the columns block, but not the blocks within the column?

No that solves the case for the gap not applied to the "columns" block itself but wouldn't prevent the cascade. (I think the current PR doesn't prevent the cascade either)

@youknowriad
Copy link
Contributor

Forget it actually, that solution only works for the "flow" layout, not "flex" ones

@youknowriad
Copy link
Contributor

Seems the root issue here is that we're kind of missing a "gap" property on the "flow" layouts forcing us to use styles that target its children and these styles are using the variable that is set on the child blocks instead of the parent. I don't have a solution yet but I'll keep thinking about it :)

@ndiego
Copy link
Member

ndiego commented Nov 23, 2021

Yeah it is a tricky one to come up with an elegant solution.

@youknowriad
Copy link
Contributor

Would something like this work? #36521 (comment)

@richtabor richtabor closed this Mar 14, 2023
@richtabor richtabor deleted the try/36521/better-blockgap branch March 14, 2023 15:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants