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

Group block with .alignfull.has-global-padding does not actually have global padding #48751

Open
felixarntz opened this issue Mar 4, 2023 · 12 comments
Labels
[Block] Group Affects the Group Block (and row, stack and grid variants) [Type] Bug An existing feature does not function as intended

Comments

@felixarntz
Copy link
Member

Description

As I was rebuilding my site using Twenty Twenty-Three, I ran into this bug.

When you create a core/group block that also has .alignfull, it may lack the padding, as the .alignfull padding reset overrides it.

Step-by-step reproduction instructions

  1. Use a block theme like Twenty Twenty-Three.
  2. Create a page or post.
  3. Put a core/group block inside it at the top level and make it alignfull.
  4. Put some content, e.g. a paragraph into the core/group block.
  5. Save and visit the frontend.

You will notice that there is no padding on the core/group block even though it has the has-global-padding class, and that leads to the paragraph content touching the edges of the browser viewport when using a narrow viewport.

I have recreated it on this InstaWP site.

Screenshots, screen recording, code snippet

Screen Shot 2023-03-03 at 5 54 18 PM

Environment info

No response

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

@felixarntz felixarntz added [Type] Bug An existing feature does not function as intended [Block] Group Affects the Group Block (and row, stack and grid variants) labels Mar 4, 2023
@carolinan
Copy link
Contributor

I can reproduce this in 6.11 and 6.2 RC.

The padding with the custom property is overridden and set to 0, which makes the final margin calculation incorrect.

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

.has-global-padding :where(.has-global-padding) {
	padding-right: 0;
	padding-left: 0;
}

.has-global-padding {
	padding-right: var(--wp--style--root--padding-right);  
	padding-left: var(--wp--style--root--padding-left);
}

@carolinan
Copy link
Contributor

@tellthemachines Where can we even start to try to troubleshoot this CSS issue?

@tellthemachines
Copy link
Contributor

I can reproduce the issue, but as far as I can tell it's working as expected 😅

To clarify: the logic behind how the global padding is applied can be read in full here but the gist of it is that if there are two or more nested blocks with has-global-padding, the padding will only be applied to the outer of those blocks. In this case, because the Post Content block also has that class (I'm assuming TT3 with its default markup), the Group block inside the post is already nested a level deep, so it won't get the global padding.

In retrospect, perhaps has-global-padding should have been named differently because the presence of the class doesn't imply padding, but there's been a lot of back and forth on this logic since its first implementation and iirc that wasn't always the case. It's a really tough one to get right because what works with a particular block structure won't work with another, and there's no way of detecting what the structure is or how nested the block is.

In any case, I'll have another look at the rules as this feels like a reasonable use case that should work.

@carolinan
Copy link
Contributor

carolinan commented Apr 26, 2023

Because the spacing was removed from the query, themers are pretty much forced to nest the group blocks.

@tellthemachines
Copy link
Contributor

Wait which spacing is this? It's hard to keep up with all the changes 😅

@carolinan
Copy link
Contributor

carolinan commented Apr 26, 2023

It is!
I have to take that back. Looking through the history of changes, it never had spacing, neither did the post template. It was the color settings that were removed.
But in order to allow any block in a template to be full width, it now needs to be in a group.
So you end up with a <main> group, a query that is full width, and a post template that is full width.

For larger screens you can have default width text and and full width media if you enable "Inner blocks use content width" on the post content block but it doesn't help on small widths, where the paragraph, headings etc will touch the edges.

@tellthemachines
Copy link
Contributor

But in order to allow any block in a template to be full width, it now needs to be in a group

Hmm, that shouldn't be necessary as root level blocks are full width by default. The Group should only be needed if we want them to be wide width or content width, or as you mention if we want them to have root padding on small viewports. But for a block like Post Content, we really want its children to have root padding, not the block itself, so it could be right at the root level. Or am I missing something?

@carolinan
Copy link
Contributor

carolinan commented Apr 27, 2023

Create a post with one default, wide and full-width group with some text, and for the sake of testing, a full-width image.
Place a query on the root of the template, set the post template to full width, and set the post content to "Inner blocks uses content width".

The full-width blocks are not full-width.

This is the resulting margins on the full width group in the post, when viewed as part of the query loop, on the front:

.has-global-padding :where(.has-global-padding) > .alignfull {
	margin-right: 0;
	margin-left: 0;
}

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

Even removing the margin:0 did not fully solve the problem because the post template has
max-width: 100%;

@carolinan
Copy link
Contributor

carolinan commented Apr 27, 2023

Aside: I know this is going to sound incredibly stupid but it is actually not clear that root level blocks are full width by default, because it doesn't have the full-width indicator -the alignment button, in black, in the toolbar.
I work on a small screen and normally I have both the list view and the block settings sidebar open, which means that in the editor, the difference between a full-width block and a default width block is minimal and I rely on the other visual queues that I have been trained on.

@tellthemachines
Copy link
Contributor

Place a query on the root of the template, set the post template to full width, and set the post content to "Inner blocks uses content width".
The full-width blocks are not full-width.

I can only reproduce this is the Query block is set to "Inner blocks uses content width", otherwise the Post Template doesn't have an alignment control and takes up full width by default.

I know this is going to sound incredibly stupid but it is actually not clear that root level blocks are full width by default, because it doesn't have the full-width indicator -the alignment button, in black, in the toolbar.

That doesn't sound stupid at all! I agree it's a pretty unintuitive UI. Even for folks who know HTML and CSS it's not immediately obvious that by default everything is full width because it's unstyled.

It would be good to have some clear indication of what the actual block width is at any time, though I'm not sure what the best way to approach this may be. If the existing alignment control were to display only the full width button, with no ability to change it, would that convey clearly enough that the block is full width by default? There should probably be a separate issue to discuss the options here.

@carolinan
Copy link
Contributor

carolinan commented Apr 27, 2023

I am just getting more and more confused. The query that I have at the root level has the class alignfull so its not really "unstyled".
I think what happened is that I had the query placed inside the group, and dragged it outside via the list view to place it on the correct level: and the class was not removed.
🤔

@tellthemachines
Copy link
Contributor

The query that I have at the root level has the class alignfull so its not really "unstyled".

Even if it has the alignfull class there shouldn't be any styles attached to it. I think alignfull only has styles when it's either a root container (Post Content block) or a child of a block with has-global-padding.

I think what happened is that I had the query placed inside the group, and dragged it outside via the list view to place it on the correct level: and the class was not removed

This sounds like a bug! I can only reproduce it partially: the class is removed in the editor but stays on the front end.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Group Affects the Group Block (and row, stack and grid variants) [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

No branches or pull requests

3 participants