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 collapsing unexpectedly #56229

Closed
dabowman opened this issue Nov 16, 2023 · 8 comments
Closed

Root padding collapsing unexpectedly #56229

dabowman opened this issue Nov 16, 2023 · 8 comments
Labels
[Feature] Layout Layout block support, its UI controls, and style output. [Type] Bug An existing feature does not function as intended

Comments

@dabowman
Copy link

Description

Through some trial and error I think I've isolated why I've struggled with the rootPaddingAwareAlignments features that is supposed to help with managing the root application padding in regards to block alignment. Layout is a bigger conversation and I think there are a few issues here that bear discussing. But to keep this simple I think I've identified two situations in which this feature is behaving unexpectedly.

FIrst, If you group a set of blocks and that group has the combo of "layout":{"type":"constrained"} and "align":"full" then its children will breach the root padding. The group here has a blue background.

Screenshot 2023-11-16 at 1 36 39 PM

Second, if you group a set of blocks and that group has the combo of "layout":{"type":"default"} and "align":"full" then it's direct children will respect root padding but other direct child groups and their children will breach root padding. The blue group here has a nested group with a gray background and you can see it's children breaching root padding.

Screenshot 2023-11-16 at 1 36 16 PM

Step-by-step reproduction instructions

Set rootPaddingAwareAlignments to true
Add root padding in styles.spacing.padding
Group a set of blocks
Add "layout":{"type":"constrained"} and "align":"full"
check the preview at a narrow screen width.

set "layout":{"type":"default"} and add a nested group with any settings that contains blocks.
check the preview at a narrow screen width

Screenshots, screen recording, code snippet

No response

Environment info

WP 6.4.1
PHP 8
wp-now env

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

@dabowman dabowman added the [Type] Bug An existing feature does not function as intended label Nov 16, 2023
@jordesign jordesign added the [Feature] Layout Layout block support, its UI controls, and style output. label Nov 17, 2023
@tellthemachines
Copy link
Contributor

Thanks for writing out this issue!

Could you add some more info about which theme you're testing on, or, if it's a custom theme, what the settings of the Post Content block are? This is relevant because Post Content the alignment and layout type of Post Content will have an influence in the root padding of any blocks inside a post.

The blue group here has a nested group with a gray background and you can see it's children breaching root padding.

I'm not sure what you mean by "breaching" here. Is it the bullet on the List that's misaligned?

@dabowman
Copy link
Author

Yeah! the here's the post content block that's displaying the groups I show above:

Full width, with child alignment enabled.

I'm not sure what you mean by "breaching" here. Is it the bullet on the List that's misaligned?

I just mean that the root padding isn't being applied, so the content is being rendered outside of it. The red in my screenshot here is my root padding. I would expect the contents of the blue section to respect that and not render all the way to the edge of the screen.

Screenshot 2023-11-20 at 10 26 59 AM

Also yes, that bullet is incorrect. But I assumed that was a separate issue with the list block. I was going to open another issue for that.

@dabowman
Copy link
Author

dabowman commented Nov 20, 2023

Digging in a bit deeper on the front-end. I'm trying to pick apart this css selector and why it's applying correctly in some places and not in others. This is the wildest css selector I've ever seen ...

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

So, to attempt to translate ...

In order to be targeted by this and get global padding, the block needs to be inside the post content block with .has-global-padding

Furthermore it needs to be inside a container (group, columns, media/text, cover) that ...

  1. is aligned full
  2. and does not itself have .has-global-padding
  3. and does not have .is-layout-flex ( the row or stack variant of the group block in my case )
  4. and does not have .is-layout-grid ( I think the media/text block gets this? maybe the cover too? )

I think this means it has to be a group, or columns block in it's default state that is aligned full.

and the element inside this container is itself ...

  1. a block [class*="wp-block-"]
  2. and not aligned full :not(.alignfull)
  3. and not a sub-block of some kind to avoid recursive padding :not([class*="__"])

OR

  1. an element with the .wp-block class (does anything have this?)
  2. that is not aligned full

OR any of the following elements

p,h1,h2,h3,h4,h5,h6,ul,ol

I think I got that right ...

Inspecting my front-end I can see one issue right off the bat. If I toggle on Inner blocks use content width on my group block it gets the .has-global-padding class. By the logic above it seems like this opts it out of global padding since that class only targets groups within the post content block that don't have that class ... I'm not sure why that is.

I'm going to keep investigating and updating here.

@dabowman
Copy link
Author

Ok, ignore my above explanation I had to consult Chat GPT to break this one down for me.

The selector selects elements with the class .alignfull that are direct children of elements with the class .has-global-padding and also do not have the classes .has-global-padding, .is-layout-flex, or .is-layout-grid.

It then looks for children of those .alignfull elements ...

whose class attribute contains "wp-block-" but does not include .alignfull or any class with two underscores.

OR

who has the class .wp-block but does not have the class .alignfull. (does this match anything?)

OR

is any paragraph, heading or list element.

I think this is more accurate than what I wrote above. The commas and spaces in that selector are kinda hard to parse out.

In any case, it seems like adding .has-global-padding to groups with inner blocks use content width on is breaking this and causing global padding to not be applied. I'm not sure if this is intended? in my case and all cases I can think of this is at the very least, not clear when you select that toggle if it is indeed expected behavior. However, my gut tells me that that selector is so complex that most folks see it and run away to write a little css patch or something rather than try to figure out what's going on. So this might indeed be a bug.

@dabowman
Copy link
Author

Ahhhh ok, I think I've got another real bug here.

Knowing that toggling on inner blocks use content width can cause the children of that group to get skipped for global padding I tried turning it all off. This does indeed work, even for nested groups, UNLESS you previously selected alignfull for a nested group. If you toggle inner blocks use content width off on the parent group, the child group retains its .alignfull class even though the controls for that have been hidden and the resulting layout should be the same if the parent group has alignfull. This causes root padding to not be applied to that child group. If you toggle inner blocks use content width back on on the parent group, remove the alignment on the child group and toggle inner blocks use content width back off, then the child group gets global padding as expected.

@tellthemachines
Copy link
Contributor

Thanks for all the extra details @dabowman!

From what I can see, the padding on that nested group is being removed by this rule, which resets padding for nested has-global-padding classes, so that it doesn't get applied twice.

This might be fixable by excluding alignfull blocks from that rule. I can give it a try but anything we change here will have to be thoroughly tested to make sure we're not breaking anything else 😅

@tellthemachines
Copy link
Contributor

Hmm after looking into this a bit I don't think there's a way of targeting specifically alignfull children in a way that doesn't cause unexpected padding in other situations.

The problem is that there's no way of telling the nesting level of a block or group of blocks, so given the following markup:

<!-- wp:group {"backgroundColor":"accent-4","layout":{"type":"constrained"}} -->
<div class="wp-block-group has-accent-4-background-color has-background"><!-- wp:paragraph -->
<p>group aligned with content</p>
<!-- /wp:paragraph -->

<!-- wp:group {"align":"full","backgroundColor":"accent-2","layout":{"type":"constrained"}} -->
<div class="wp-block-group alignfull has-accent-2-background-color has-background"><!-- wp:paragraph -->
<p>nested group full aligned</p>
<!-- /wp:paragraph --></div>
<!-- /wp:group --></div>
<!-- /wp:group -->

what works in this case if these blocks are at root template level - the nested group gets padding applied - doesn't work if they are in any context where the nested group isn't actually full page width, because in that case we wouldn't expect it to have padding.

I also considered adding something like .has-global-padding:where(.alignfull) > :where(.has-global-padding.alignfull) { padding-right: var(--wp--style--root--padding-right); padding-left: var(--wp--style--root--padding-left); } but again, if we have two nested alignfull blocks within a column, for instance, there might be unwelcome padding on the innermost block.

I hope this makes sense. The rules have become a bit mind-boggling because so many exceptions had to be added over time.

@annezazu
Copy link
Contributor

Closing this out as it appears the questions have been asked and answered. With that said, I did want to note this effort for 6.6 to make root padding more consistent across the interfaces: #60715 Going forward, please open a new issue with more up to date info based on these recent changes and any new/current problems that remain.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Layout Layout block support, its UI controls, and style output. [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

No branches or pull requests

4 participants