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 missing on nested alignfull blocks #44404

Closed
robrobsn opened this issue Sep 22, 2022 · 6 comments
Closed

Root padding missing on nested alignfull blocks #44404

robrobsn opened this issue Sep 22, 2022 · 6 comments

Comments

@robrobsn
Copy link

Description

Follow-up for #43095
Might be relevant for #44336

When useRootPaddingAwareAlignments is set tot true in the theme.json, the double padding for nested .has-global blocks has been fixed 🙌 but the root padding is now missing for nested .alignfull blocks.

The main issue is that the padding of .alignfull blocks inside .has-global-padding blocks is missing because it is either not set or overwritten to zero.

Adding the following rule after the existing ones fixes the issue for me:

.has-global-padding > .alignfull {
  margin-right: calc(var(--wp--style--root--padding-right) * -1);
  margin-left: calc(var(--wp--style--root--padding-left) * -1);
  padding-right: var(--wp--style--root--padding-right);
  padding-left: var(--wp--style--root--padding-left);
}

Step-by-step reproduction instructions

  1. Create a new page
  2. Copy and paste the following content:
<!-- wp:heading -->
<h2>regular</h2>
<!-- /wp:heading -->

<!-- wp:heading {"align":"wide"} -->
<h2 class="alignwide">wide</h2>
<!-- /wp:heading -->

<!-- wp:heading {"align":"full"} -->
<h2 class="alignfull">full</h2>
<!-- /wp:heading -->

<!-- wp:group {"layout":{"type":"constrained"},"align":"wide","backgroundColor":"pale-pink"} -->
<div class="wp-block-group alignwide has-pale-pink-background-color has-background"><!-- wp:heading -->
<h2>regular in wide</h2>
<!-- /wp:heading --></div>
<!-- /wp:group -->

<!-- wp:group {"layout":{"type":"constrained"},"align":"full","backgroundColor":"pale-pink"} -->
<div class="wp-block-group alignfull has-pale-pink-background-color has-background"><!-- wp:heading -->
<h2>regular in full</h2>
<!-- /wp:heading -->

<!-- wp:heading {"align":"wide"} -->
<h2 class="alignwide">wide in full</h2>
<!-- /wp:heading -->

<!-- wp:heading {"align":"full"} -->
<h2 class="alignfull">full in full</h2>
<!-- /wp:heading -->

<!-- wp:group {"layout":{"type":"constrained"},"align":"full","backgroundColor":"vivid-red"} -->
<div class="wp-block-group alignfull has-vivid-red-background-color has-background"><!-- wp:heading {"align":"wide"} -->
<h2 class="alignwide">wide in full in full</h2>
<!-- /wp:heading --></div>
<!-- /wp:group -->

<!-- wp:group {"layout":{"type":"constrained"},"align":"full","backgroundColor":"vivid-red"} -->
<div class="wp-block-group alignfull has-vivid-red-background-color has-background"><!-- wp:heading {"align":"full"} -->
<h2 class="alignfull">full in full in full</h2>
<!-- /wp:heading --></div>
<!-- /wp:group --></div>
<!-- /wp:group -->
  1. Save the page and view it on the frontend
  2. Observe that that the 'full' heading as well as all blocks inside the pink full group have no visual gap on the left side event though a root padding has been set in the theme json.

Screenshots, screen recording, code snippet

TT3 before:


TT3 after patch:

If this fix turns out to be working as expected some of the old CSS rules can be removed or refactored.

Hope this helps! Root padding is definitely a tricky one.

Environment info

WordPress 6.0.2
Gutenberg 14.1.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

@gyurmey2
Copy link

I confirm that the problem still exists.

@tellthemachines
Copy link
Contributor

Thanks for your report @robrobsn!

The main issue is that the padding of .alignfull blocks inside .has-global-padding blocks is missing because it is either not set or overwritten to zero.

This only happens with .alignfull blocks which also have the .has-global-padding classname, and I'm afraid it's a necessary side-effect of fixing #43095 so that no blocks get double the amount of padding.

Adding the following rule after the existing ones fixes the issue for me

That's because the rule is overriding '.has-global-padding :where(.has-global-padding) > .alignfull { margin-right: 0; margin-left: 0; }' (part of the fix for #43095). It only works when placed after because both have the same specificity 😅

It should also be noted that padding isn't applied to .alignfull blocks themselves, but only to some of their children. This is to avoid breaking Image, Video and other blocks that should always have full width content.

In adding this feature to Core, the main concerns were providing some sensible defaults that can make theme building easier, and making the rules as generic as possible (avoiding targeting specific block types). This necessarily involves optimising for the most common cases, and given the amount of possible combinations of nested blocks with/without width constraints or alignments, it won't ever be possible to make it work in every single scenario.

One workaround for the code example above is to remove the width constraint for the parent Group, and nest any contents you wish to constrain in another Group. This allows the any non-.alignfull children of the Groups with red background to receive padding. Padding on the Groups with background-color would also need adjusting, as these get a default value added from the block stylesheet (this is something that needs improvement, see #43110). You'd get something like this:

Screen Shot 2022-09-28 at 4 19 40 pm

For the .alignfull headings though, the best thing to do is add an equivalent amount of padding to the block itself (this is possible to do under "Dimensions" in the block sidebar).

I'm going to go ahead and close this issue for now, but thanks again for looking into this!

@robrobsn
Copy link
Author

@tellthemachines Thank you for the detailed reply. The solution is disappointing, though.

  1. Isn't it still an error that nested 'regular in full' and 'wide in full' ( see first screenshot ) have no padding and are flush to the viewport if you reduce the viewport width? How I understand it, the idea of root padding and aware alignment is that non-alignfull blocks have a visual gap to the viewport. It's a common pattern to have group with a background color and an alignwide content.

  2. This is to avoid breaking Image, Video and other blocks that should always have full width content.

Good point! To remove those paddings I suggest adding the following CSS code after the previous patch
to ensure that alignfull blocks have full width content unless they are layout containers themselves.

.has-global-padding > .alignfull:where(:not(.is-layout-constrained)) {
  padding-left: 0;
  padding-right: 0;
}

In order to make .is-layout-flow.alignfull blocks work as well the following CSS rule should be removed, since the new .alignfull patch already manages the padding.

.has-global-padding > .alignfull:where(:not(.has-global-padding)) > :where([class*="wp-block-"]: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 added a .layout-is-flow group to the test content. Please see below for the updated test content.

Updated test content ```html

regular

wide

full

regular in wide

regular in full

wide in full

full in full

wide in full in full

full in full in full

regular in full layout-flow

```

For the .alignfull headings though, the best thing to do is add an equivalent amount of padding to the block itself

I would not expect the user to manually have to add padding in order to make it work after specifically being set in the theme.json. Just the hassle of changing the root padding and having to update blocks manually to match it sounds
very cumbersome.

In adding this feature to Core, the main concerns were providing some sensible defaults that can make theme building easier, and making the rules as generic as possible (avoiding targeting specific block types).

I agree a 100%. So, wouldn't it be more consistent/easier, code-wise and more user friendly if every element behaves the same, no exceptions by type. Meaning visually, if the user wants full width content, they chose alignfull, everything else has a visual root padding. Directly nested .alignfull container/layout blocks repeat having negative margin and padding and nested alignfull non-layout blocks have negative margin and zero padding to appear full width. This way there's no need to add rules for children and all their variations as you mentioned in your reply.

@gyurmey2
Copy link

gyurmey2 commented Oct 5, 2022

I agree 100% with @robrobsn. The current solution is disappointing.

@badg0003
Copy link

badg0003 commented Jun 7, 2023

Confirming this is still an issue? I believe I've stumbled across the same with 6.2.2.

@cr0ybot
Copy link
Contributor

cr0ybot commented Jul 2, 2023

I didn't really follow the fixes necessary for the "double padding" issue, but the current state of things leaves for some fairly unexpected results in the site editor. Most commonly, having a single root Group block with constrained layout that contains an alignfull constrained Post Content block or other nested alignfull constrained Group will result in no padding at all for those inner blocks. I've had to solve this by structuring my single post template with the Post Content block at the root, and wrapping each other part of the template in groups, which feels weirdly like how I used to do things manually with PHP templates and custom CSS before the site editor. I guess I don't mind it so much, but there isn't really any clarity while using the site editor that this will be the case (other than checking the mobile preview).

Note that in the screenshot below, the Group block with id "content" is not set to have a constrained layout. Each of the nested Groups and the Post Content block are set to constrained.

Screen Shot 2023-07-02 at 11 04 45 AM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants