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

In 12.1, blockGap appears partially enabled when it shouldn't be #37232

Closed
ndiego opened this issue Dec 8, 2021 · 19 comments · Fixed by #37254
Closed

In 12.1, blockGap appears partially enabled when it shouldn't be #37232

ndiego opened this issue Dec 8, 2021 · 19 comments · Fixed by #37254
Assignees
Labels
CSS Styling Related to editor and front end styles, CSS-specific issues. [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended [Type] Regression Related to a regression in the latest release

Comments

@ndiego
Copy link
Member

ndiego commented Dec 8, 2021

Description

In 12.1, something changed (still trying to locate) that enables blockGap related CSS even if blockGap is not enabled in theme.json. I can also confirm that this issue exists in 5.9 Beta 2 without the Gutenberg plugin active.

In Gutenberg 12.0.2, I have a Group block with a Paragraph and Buttons block inside. It looks something like this:
image

Now in 12.1, the same collection of blocks looks like this.
image

Notice all the additional CSS applied to the paragraph block. In theme.json, blockGap is not enabled on this theme.

Step-by-step reproduction instructions

  1. Install Twenty Twenty-Two and disable blockGap in theme.json
  2. Install Gutenberg 12.0.2
  3. Create a Group block and place a few paragraph blocks inside.
  4. View the markup on the frontend.
  5. Upgrade Gutenberg to 12.1
  6. Review the markup again and see all the additional blockGap related CSS added to the frontend.

Screenshots, screen recording, code snippet

No response

Environment info

WordPress 5.9 Beta 2, Gutenberg 12.1

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

@ndiego ndiego added the [Type] Bug An existing feature does not function as intended label Dec 8, 2021
@bgardner
Copy link

bgardner commented Dec 8, 2021

I can confirm that this is happening. Updated to Gutenberg 12.1 to work on updating Frost, but found it impossible because of what @ndiego reported. Had to revert to Gutenberg 12.0.2 and unfortunately have to wait.

@ndiego ndiego added the CSS Styling Related to editor and front end styles, CSS-specific issues. label Dec 8, 2021
@skorasaurus skorasaurus added the [Type] Regression Related to a regression in the latest release label Dec 8, 2021
@ndiego
Copy link
Member Author

ndiego commented Dec 8, 2021

Upon more investigation, this issue might have something to do with this commit: 96a0daf. Not 100% sure, but regardless, this is the line in the file that controls the blockGap styling:

$has_block_gap_support = isset( $block_gap ) ? null !== $block_gap : false;

If you simply disable this line, themes that have blockGap disabled behave the nearly same way they did in 12.0.2. The autogenerated CSS related to blockGap is not displayed, however the blockGap global styles are still printed:

.wp-site-blocks > * {
    margin-top: 0;
    margin-bottom: 0;
}

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

@bgardner
Copy link

bgardner commented Dec 8, 2021

It appears the CSS you shared above is coming from this:

$has_block_gap_support = _wp_array_get( $this->theme_json, array( 'settings', 'spacing', 'blockGap' ) ) !== null;

Which is also using $has_block_gap_support from the layout.php file.

The code in full:

$has_block_gap_support = _wp_array_get( $this->theme_json, array( 'settings', 'spacing', 'blockGap' ) ) !== null;
if ( $has_block_gap_support ) {
	$block_rules .= '.wp-site-blocks > * { margin-top: 0; margin-bottom: 0; }';
	$block_rules .= '.wp-site-blocks > * + * { margin-top: var( --wp--style--block-gap ); }';
}

@ndiego
Copy link
Member Author

ndiego commented Dec 8, 2021

Sorry for the ping @oandregal, but it appears that perhaps some of the changes in #36907 might have caused this issue? Happy to keep investigating, but thought you might be able to spot it quicker if it is indeed related 😉

@noisysocks noisysocks added this to 📥 To do in WordPress 5.9 Must-Haves via automation Dec 8, 2021
@github-actions github-actions bot added the [Status] In Progress Tracking issues with work in progress label Dec 9, 2021
@oandregal
Copy link
Member

👋 I've bisected between 12.0.2 (worked well) and 12.1 (no longer works) and it started failing at #36646 Thanks for providing that info, btw, it makes debugging a lot easier.

I've started a bugfix at #37254 (needs more work).

@bgardner
Copy link

bgardner commented Dec 9, 2021

@oandregal FWIW, here's what I have found. If you remove support for "appearanceTools": true, (in other words, do not include that at all in theme.json), the following seems to disable blockGap:

"spacing": {
	"blockGap": null,
	"margin": true,
	"padding": true,
	"units": [ "px", "em", "rem", "vh", "vw", "%" ]
},

While the following does not:

"spacing": {
	"blockGap": false,
	"margin": true,
	"padding": true,
	"units": [ "px", "em", "rem", "vh", "vw", "%" ]
},

Which basically means that once "appearanceTools": true, is added to theme.json, no solution seems to disable blockGap. Neither of the code blocks above can turn it off. (After "appearanceTools": true, turns it on.)

@bgardner
Copy link

bgardner commented Dec 9, 2021

Also worth noting... even without "appearanceTools": true, being in theme.json, blockGap seems to be turned on.

@oandregal
Copy link
Member

#37254 is now ready and fixes this issue.

This is how it works:

  1. When appearanceTools is false or doesn't exist and the theme doesn't set blockGap: the styles are not enqueued.
  2. When appearanceTools is true
    2.1 And the theme doesn't set blockGap or sets it to true, the styles are enqueued.
    2.2 And the theme sets blockGap to null, the styles are not enqueued.
    2.3 And the theme sets settings.spacing.blockGap to false, the styles are enqueued.

Note that 2.2 and 2.3 is how it works in Gutenberg 12.0.2. I don't know why blockGap set to false still enqueued the styles but it's a different issue anyway. cc @andrewserong and @aaronrobertshaw for thoughts.

@ndiego
Copy link
Member Author

ndiego commented Dec 9, 2021

Thanks so much for working on this @oandregal!

@youknowriad
Copy link
Contributor

2.1 And the theme doesn't set blockGap or sets it to true, the styles are enqueued.
2.2 And the theme sets blockGap to null, the styles are not enqueued.
2.3 And the theme sets settings.spacing.blockGap to false, the styles are enqueued.

This is the expected behavior btw. The reason is that there's a difference between:

  • Adding support to block gap entirely (enqueuing styles)
  • and allowing users to define block gap per blocks (show the block gap UI for blocks)

This difference between null and false is only available in the blockGap setting but it's something we might also need for future additions.

@ndiego
Copy link
Member Author

ndiego commented Dec 9, 2021

I have tested #37254 and it works as expected! 🙌

The null versus false thing is strange, but it does work with null. This issue can be closed with the merge of #37254. Thanks again @oandregal!

@bgardner
Copy link

bgardner commented Dec 9, 2021

Yes, thanks to all who helped resolve this issue!

WordPress 5.9 Must-Haves automation moved this from 📥 To do to ✅ Done Dec 10, 2021
@oandregal
Copy link
Member

oandregal commented Dec 10, 2021

This difference between null and false is only available in the blockGap setting but it's something we might also need for future additions.

Got it, thanks for sharing the rationale.

As a data point for future additions, I don't find the current system very obvious. In my view, to encode three different states I'd avoid using booleans + null. I'd use strings or something like that. Something along the lines of "disabled", "only-enqueue", "enabled" is more obvious.

@ndiego
Copy link
Member Author

ndiego commented Dec 10, 2021

☝️ Agree with the above null versus false is confusing.

@nickfmc
Copy link

nickfmc commented Jul 1, 2022

WP 6.0 seams to have this issues again? unless I have "appearanceTools": false, then the block gap styles are enqueued if I set blockGap to null or not. Can anyone verify this @oandregal

@spencer-j
Copy link

Yes still an issue in 6.0, and very confusing, as official docs
https://developer.wordpress.org/block-editor/how-to-guides/themes/theme-json/#what-is-blockgap-and-how-can-i-use-it
and other
https://wpdevelopment.courses/articles/block-gap-margin-padding/#understanding-block-gap
say its true, false and null is not mentioned.

@ndiego
Copy link
Member Author

ndiego commented Aug 8, 2022

@spencer-j the official documentation has a detailed guide on the various settings:

  • true: Opt into displaying Block spacing controls in the editor UI and output blockGap styles.
  • false: Opt out of displaying Block spacing controls in the editor UI, with blockGap styles stored in theme.json still being rendered. This allows themes to use blockGap values without allowing users to make changes within the editor.
  • null (default): Opt out of displaying Block spacing controls, and prevent the output of blockGap styles.

@nickfmc
Copy link

nickfmc commented Aug 8, 2022

Ahh thanks @ndiego blockgap: null and appearanceTools: true is working for me now!

@medesigngood
Copy link

Is there some non-technical documentation for a designer that just doesn't want his layouts messed up by blockgap? Perhaps some custom css that one can add to override this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CSS Styling Related to editor and front end styles, CSS-specific issues. [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended [Type] Regression Related to a regression in the latest release
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

8 participants