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

Root padding is doubled on child blocks #43095

Closed
richtabor opened this issue Aug 9, 2022 · 16 comments · Fixed by #43842
Closed

Root padding is doubled on child blocks #43095

richtabor opened this issue Aug 9, 2022 · 16 comments · Fixed by #43842
Assignees
Labels
Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json [Type] Bug An existing feature does not function as intended

Comments

@richtabor
Copy link
Member

Description

A lot of effort has gone into consolidating and simplifying block theme support for making blocks fullwidth, while also supporting root padding, particularly in #42085.

I've been testing this out with TT2 and it looks like padding is inaccurately applied to child blocks, doubling up the padding within.

Do we need the following generated styles — or should we be zeroing out padding if .has-global-padding > .has-global-padding occurs?

.has-global-padding > .alignfull > :where([class*="wp-block-"]:not(.alignfull):not(.alignfull):not([class*="__"]),p,h1,h2,h3,h4,h5,h6,ul,ol) {
    padding-right: var(--wp--style--root--padding-right);
    padding-left: var(--wp--style--root--padding-left);
}

I think we're really close on getting a root padding/fullwidth that "just works" 🤞

Step-by-step reproduction instructions

  1. Start with the TT2 theme.
  2. Comment out/remove all the custom "Alignment styles" from the theme (source).
  3. Add "settings.useRootPaddingAwareAlignments": true to the theme.json
  4. Update the theme.json styles.spacing.padding values as such, which emulates what TT2 was achieving with custom alignment styles:
"padding": {
       "top": "0px",
        "right": "var(--wp--custom--spacing--outer)",
       "bottom": "0px",
        "left": "var(--wp--custom--spacing--outer)"
} 
  1. Add the TT2 "Video with header and details" pattern.
  2. View the front-end/editor, resizing the viewport to see the issue (as double the proper padding is applied).

Screenshots, screen recording, code snippet

.has-global-padding is being applied to subsequent fullwidth blocks, which doubles the amount of left/right padding. This doubled padding is only perceptible on mobile as the viewport is small enough to see the effect:

Frame 3

Frame 2

Environment info

WordPress 6.0.1
Twenty Twenty Two (modified from above)

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
Copy link
Member

ndiego commented Aug 9, 2022

I can confirm. See screenshot. The key appears to be the "layout":{"inherit":true} on the inner group that is causing the class to get added.

image

To replicate, implement Rich's theme.json settings above and use the following block markup:

<!-- wp:group {"align":"full","backgroundColor":"cyan-bluish-gray"} -->
<div class="wp-block-group alignfull has-cyan-bluish-gray-background-color has-background"><!-- wp:paragraph {"backgroundColor":"vivid-cyan-blue"} -->
<p class="has-vivid-cyan-blue-background-color has-background">This is an example page. It's different from a blog post because it will stay in one place and will show up in your site navigation (in most themes).</p>
<!-- /wp:paragraph -->

<!-- wp:group {"layout":{"inherit":true}} -->
<div class="wp-block-group"><!-- wp:paragraph {"backgroundColor":"vivid-cyan-blue"} -->
<p class="has-vivid-cyan-blue-background-color has-background">This is an example page. It's different from a blog post because it will stay in one place and will show up in your site navigation (in most themes).</p>
<!-- /wp:paragraph --></div>
<!-- /wp:group --></div>
<!-- /wp:group -->

@ndiego ndiego added [Type] Bug An existing feature does not function as intended Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json labels Aug 9, 2022
@ndiego
Copy link
Member

ndiego commented Aug 9, 2022

@tellthemachines and @andrewserong pinging you both on this. 🙏

@tellthemachines
Copy link
Contributor

Thanks for the report! I can reproduce this in Empty theme by adding "useRootPaddingAwareAlignments": true to theme.json settings and nesting two blocks which inherit content width inside each other.

I'll have a think about how best to fix this - removing the padding from .has-global-padding > .has-global-padding might work, as long as we make sure we're not accidentally overriding any custom padding the child block might have.

@tellthemachines
Copy link
Contributor

Hmm, seems like the extra padding we're seeing in those examples is actually coming from a rule that targets blocks with background-color, and not from the root padding styles themselves:

Screen Shot 2022-08-10 at 2 53 10 pm

What's happening is that the current TT2 custom logic for root padding overrides that background-color padding with a more specific rule:

Screen Shot 2022-08-10 at 2 55 19 pm

This is because TT2 logic works by adding a negative margin to alignfull blocks, and then compensating by adding padding inside them. It's that padding rule that's overriding the background-color one.

In core, instead of adding padding to alignfull blocks directly, we add it to some of their children, targeted by the somewhat abstruse has-global-padding > .alignfull > :where([class*="wp-block-"]:not(.alignfull):not([class*="__"]),p,h1,h2,h3,h4,h5,h6,ul,ol). This is because we want to avoid adding padding to the inner contents of alignfull blocks such as Image and Video. There doesn't seem to be any good way of creating such a system of exclusions, but we deliberately avoided targeting specific blocks by name (as TT2 does) because that's likely to cause problems with non-core blocks.

I'm afraid there's no straightforward fix for this 😅 but it might be a good time to re-evaluate that background-color rule, because it also interferes with nested alignfull blocks, and as theme.json adoption increases we want to move away from hardcoding those values in stylesheets.

In the meantime, if you're looking to adopt the core root padding system in TT2, maybe popping a temporary override in the theme stylesheet is the best way to unblock this. I'm thinking something like .has-global-padding > .alignfull.has-background and set the left/right padding to 0 might work.

@richtabor
Copy link
Member Author

@bgardner and I did some more testing on this front, looking at Twenty Twenty Three. I don't think this doubled-up padding is due to the background color, but rather the targeting of padding on contents within alignfull blocks.

What's happening

The has-global-padding rule is global padding to constrained layout blocks (which includes Group blocks within other constrained blocks). That's fine, but then there's no need to apply padding yet again, to blocks within those constrained layout blocks. Doing so doubles up the padding, which you can see by shrinking the viewport down — forcing the doubled padding to show.

CleanShot 2022-09-01 at 16 00 38@2x

Suggestion

We should get more eyes on this, but if we remove this rule entirely, the double padding is no longer applied to blocks within fullwidth, constrained blocks. This ensures that all blocks have the proper left/right padding from the styles.spacing.padding values — as other top-level blocks do.

In the screenshot below I have two shots on the left of the doubled padding displaying, with and without background colors applied to the group block. On the right is the result of commenting out this rule.

CleanShot 2022-09-01 at 15 54 21@2x

@ndiego
Copy link
Member

ndiego commented Sep 1, 2022

In some testing, I think modifying the rule to the following should work. The issue with removing it completely is that it impacts blocks with .is-layout-flow. @richtabor, can you give this a try and see if it works for you?

.has-global-padding > .alignfull:not(.has-global-padding) > :where([class*="wp-block-"]:not(.alignfull):not(.alignfull):not([class*="__"]),p,h1,h2,h3,h4,h5,h6,ul,ol)

@ndiego
Copy link
Member

ndiego commented Sep 1, 2022

@andrewserong sorry for the ping, but this is the last piece of the "root site padding" puzzle. .has-global-padding is now showing up where it should, but this extra padding is now throwing things off. It's a bit mindbending 😅

@richtabor
Copy link
Member Author

The issue with removing it completely is that it impacts blocks with .is-layout-flow.

What does it cause?

@tellthemachines
Copy link
Contributor

Thanks for the report!
The has-global-padding rule works by applying padding to its children, so that the contents of a full-width block can have the same padding as other top-level blocks. The problem we're seeing here is that if we have nested blocks that both have the has-global-padding class, the padding is duplicated. This can be fixed by adding an override along the lines of .has-global-padding > .has-global-padding { padding-left: 0;padding-right:0 }. I'll have a play with it and see if I can put a PR together.

@ndiego
Copy link
Member

ndiego commented Sep 1, 2022

What does it cause?

Padding on inner blocks is not applied. Blocks with .is-layout-flow don't get the .has-global-padding class, and therefore need that complicated CSS.

This can be fixed by adding an override along the lines of .has-global-padding > .has-global-padding { padding-left: 0;padding-right:0 }.

This almost works, but not quite. The padding on blocks inside of the child block with has-global-padding still get padding applied. While this looks ok on mobile, on all other screen sizes where the content is not constrained, the extra padding is visible, which it should not be. This is a bit hard to explain but see the screenshot below.

Changing the CSS to the following seems to solve the issue, but I don't love it.

.has-global-padding > .alignfull:not(.has-global-padding) > :where([class*="wp-block-"]:not(.alignfull):not(.alignfull):not([class*="__"]),p,h1,h2,h3,h4,h5,h6,ul,ol)

spacing

@bgardner
Copy link

bgardner commented Sep 1, 2022

To help illustrate what is going on, here's a link: https://bg.design/powder/test/

Both instances have groups with content that should be aligned with the content in the header/footer, but the double padding is bumping it in another 30 px.

image

@tellthemachines
Copy link
Contributor

These rules were arrived at after a lot of back-and-forth both on #42085 and #35607. For any fix we try, we'll need to make sure that we're not breaking any expected behaviour. I think it would help to have our expectations clearly documented (which I now wish I'd done while I was wrangling the PR 😅 ) so going to address that now along with trying out a fix.

@richtabor
Copy link
Member Author

richtabor commented Sep 2, 2022

100% agree, but in its current state this doesn’t work.

I’d expect not to have 2x padding applied to nested fullwidth blocks.

That’s how Twenty Twenty Two works, and how I’d expect any theme to work. Removing that CSS glob and opting into padding with TT2 would be a regression.

@tellthemachines
Copy link
Contributor

I’d expect not to have 2x padding applied to nested fullwidth blocks.

Sure, I'm looking at a fix for that now.

@richtabor
Copy link
Member Author

🙏🚀

@bgardner
Copy link

bgardner commented Sep 3, 2022

Sure, I'm looking at a fix for that now.

This would be lovely!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants