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

Consider a root-level site padding solution that still lets some items go full-width #35607

Closed
kjellr opened this issue Oct 13, 2021 · 50 comments · Fixed by #42085
Closed

Consider a root-level site padding solution that still lets some items go full-width #35607

kjellr opened this issue Oct 13, 2021 · 50 comments · Fixed by #42085
Assignees
Labels
Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json [Status] In Progress Tracking issues with work in progress [Type] Enhancement A suggestion for improvement.

Comments

@kjellr
Copy link
Contributor

kjellr commented Oct 13, 2021

The root-level padding introduced in #35241 lets you add padding around your entire site. This works great, but only if you do not want anything on the site to ever appear edge-to-edge.

In WordPress/twentytwentytwo#88, we tried to implement the site-wide padding value in Twenty Twenty-Two, but found it did not work for us for that reason. Twenty Twenty-Two is designed to let certain full width elements extend edge-to-edge on the screen. The most obvious example is its black homepage header, which is intended to extend to the screen edges:

This sort of thing is common to lots of themes, especially since Gutenberg rolled out its "full width" alignment. Themes want full-width elements to extend to the screen edges, and they want some sort of padding for everything else. This is how the past three default themes have functioned:

Today's implementation of the global padding setting wouldn't work for any of those themes.

That said, a global padding value in general would still be really useful. In the Twenty Twenty-Two homepage screenshot for example, you can see that there is a shared padding value inside of the group block header, and on the content below it, to ensure that non-full-width content doesn't bump up against the screen edges:

Generally, classic themes accomplished this sort of thing by setting a global padding rule, and then having alignfull items override that with negative margins and a modified max-width. If necessary, they'd apply those same rules inside of full-width group blocks too so the padding was consistent.

Let's discuss if there's a solution that can account for this scenario without requiring a block theme to supply custom CSS.

@kjellr kjellr added [Type] Enhancement A suggestion for improvement. Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json labels Oct 13, 2021
@jffng
Copy link
Contributor

jffng commented Oct 13, 2021

Thanks for articulating it so clearly @kjellr, it's a problem we run into on every theme we've made. @jasmussen summarized it at a high level nicely over in this comment:

ultimately we are bound by needing to output CSS in the end, and so zooming way out, the problem here remains one of deciding where padding is applied

Rather than applying padding to the root container, what if there was a way for an individual block to opt in to (or out of) using the global padding value? That value could be stored as a variable and configured in global styles, and a given block could choose to apply it. This way, blocks that are alignfull and blocks at the root level would still touch the edge of the screen, but apply a shared padding value to their children.

@jasmussen
Copy link
Contributor

Good issue to discuss, good outline of the problem. If I understand it correctly you are suggesting a single value that controls the inner padding of all root level blocks, correct?

As suggested, this value would likely be useful enough for a wide range of themes that leverage the same sectioned design style you outline. At the same time, the behavior it's difficult to picture how the value would convert across theme switches, since padding is such a widely and diversely used property.

Approaching it from a different angle, the block editor has a new blockGap property, which defines spacing between blocks, all the way down the hierarchy. You can (or will be able to) override this spacing on a per-container basis. This feels sort of natural, and feels like what you are suggesting for the padding as well, a "base padding" if you will. The tricky part is systematizing this. For example it seems sensible to embrance the vertical flow of content, and only apply this padding left and right — if we have to apply also a top padding on the first block, and a bottom margin on the last block, this is likely to get unwieldy.

@kjellr
Copy link
Contributor Author

kjellr commented Oct 14, 2021

Rather than applying padding to the root container, what if there was a way for an individual block to opt in to (or out of) using the global padding value?

Classic themes already have an established way to do this for the post and page editor: Standard and wide-aligned blocks use what is essentially a "global padding value" on either side, whereas full-width blocks don't. The full-width blocks are effectively opting out of the global padding. We don't have a mechanism like that for the root-level blocks though.

If I understand it correctly you are suggesting a single value that controls the inner padding of all root level blocks, correct?

Sort of. There are a lot of different ways to frame this, but I think the main thing I'm trying to communicate is that we just want to replicate the "default" behavior that the majority of classic themes have implemented already:

  • Standard and Wide blocks do not touch the screen edges.
  • Full-width blocks do touch the screen edges (unless they're nested, and their parent does not touch the screen edges).

Nesting makes this pretty complicated, but that's the gist of it. Getting this right was the most complicated issue for classic theme layout CSS, and we're kind of sidestepping it here.

@jasmussen
Copy link
Contributor

The use case is definitely valid. I'm purely thinking in terms of a solution that carefully threads the needle between user expectation and technical implementation. To an extent, if we could build a nice codepen that extracts a single CSS variable as the padding, that might help iron out the details.

@chthonic-ds
Copy link
Contributor

Has there been consideration given to using a CSS Grid layout to solve for root-level padding? The approach I've been using to manage this very issue looks like this: https://codepen.io/chthonic/pen/mdMeoGN?editors=1100

There's just the single padding value, and no juggling of negative margin values or accidental scrollbar overflow. It's by no means a perfect solution but it's the one I've kept coming back to when weighing up alternatives.

Apologies for jumping into your conversation like this. I found myself here checking out the progress of Twenty Twenty-Two 👍

@jasmussen
Copy link
Contributor

CSS grid has indeed been considered. There's a great deal of complexity to manage in order to strike a balance between intuitive UI and at the same time unlocking the full power of CSS Grid.

In this case, however, it would be a similar UX problem: currently the padding value is applied to the body element. Both CSS grid and negative margin solutions would then require the "root padding" to be a separate variable that is instead assigned elsewhere. Which might be the solution, but the question is whether it's intuitive for a user wanting to adjust their site padding using the Global Styles interface: where would they have to look to find this padding value?

@chthonic-ds
Copy link
Contributor

CSS grid has indeed been considered

The exploration in #16998 appears broader in that it conflates managing inner layout, too, whereas #35607 (comment) is solely concerned with how blocks should be constrained in relation to the edge of the browser. I've personally found it difficult to pass on having the ability to intrinsically determine a block's width without needing to ever specify actual padding properties.

But! I can see how my suggested approach is a no-go in the broader context of Global Styles. The UI control exposed by #35241 would need to be paired with the parent container of top-level blocks and not body for it to work with that Padding value.

Thanks for the insight :)

@jasmussen
Copy link
Contributor

It's not necessarily a no-go, it's just a difficult UI challenge to surface this padding we agree on in a place that intuitively communicates that it affects the inner spacing of multiple blocks, just not full-wide ones.

That said, conceptually I appreciate how CSS grid has a "gutter" property which feels like something you should be setting in one place, not multiple (even though you can). This is somewhat similar to the blockGap property, which even now you do set in one place, and it propogates down the hierarchy.

@fwazeter
Copy link
Member

fwazeter commented Nov 3, 2021

I'll have to look up the precise implementation I did (and I could very well be misremembering entirely, I'm on hour ~26 of a coding stint and could just have fantasy code fixes in my head), but I believe I had overridden this setting in one of my themes while using global padding.

The implementation was either to use padding-inline-start / end and padding-block-start / end explicitly on the .wp-site-blocks tag which override the padding-TRBL set by theme.JSON or some other version similar targeting a combination of WP-container and site-blocks. (I don't recall using negative margins, but I've been fighting so much with theme.JSON and spacing that I've gone through a million different versions.

If it's as simple as overriding the parent block with padding-inline/block explicit settings via the theme.JSON API, that could be a pretty viable solution as things are now.

@kjellr
Copy link
Contributor Author

kjellr commented Nov 18, 2021

@noisysocks I think we should keep this in 5.9 must-haves for now. It's still a must-fix issue in Twenty Twenty-Two, and I don't think we'll be able to come to a solution there without a pretty weird workaround.

@kjellr
Copy link
Contributor Author

kjellr commented Nov 22, 2021

I spent a bit of time on Friday talking through this problem with @jffng, and I have an idea that I think would be worth trying out. This is very similar to @youknowriad's exploration in #36214, but it's broken into a couple new controls to give users/theme authors more flexibility around when and how things break out of the site padding.

The root of the idea is a simple one:

  • Add a single new "override site padding" option that is available only for top-level blocks. This is available only when site padding is active, and it defaults to false. When this option is active, that top level block gets negative margins that allow it to take up the full width of the screen. If a top-level block is moved out of the top level, that option would disappear from the UI, and any saved markup for the option should have no effect.
  • Add a new "Use site padding for children" setting to any block that opts into layout. This is available only when site padding is active, and it defaults to false. When it's active, site padding is applied to the container, which means default + wide-aligned items take padding into account, and do not bump up against the edges of the screen. Full-aligned items will continue to take up the full width of their container.

The overarching idea here is that we only really need to override the site padding once on the top level of the page... but we also may want to add that padding in again later to a child block. 😅

Here's a quick review of the situation, since it helped me think through the problem. Most blocks already take up the full width of their parent:

Screen Shot 2021-11-22 at 8 54 08 AM

If a layout is active, full-width items take up the full width of their parent by default (children will take up the full width on small screens):

Screen Shot 2021-11-22 at 8 51 30 AM

When a theme opts into site padding, it adds some space to the left and right of the entire site. If we set a top-level block to eliminate that space, all of its child blocks continue to behave as described above: all blocks go edge-to-edge when layout is off, only full-width blocks go edge-to-edge when it's on:

layout

This will work for many use cases and doesn't require much other action. For example, if you have an image at the top of your site and you want to make it full-width, this should do it.

But there is one important case where it falls apart though: Oftentimes we want to re-use the padding values inside of a child block. So in Twenty Twenty-Two, we want the black box here to go full width, but we want the content inside of it to mirror the site padding, so that it doesn't bump up against the edges:

Screen Shot 2021-11-22 at 9 35 29 AM

For cases like that, and for the Post Content block (where we also want this behavior), we could leverage a new option to basically "Add the site padding to this child block instead". We've seen that this works in #36214, but it was a little too aggressive — it added the site padding by default all the time whenever a block opted into layout, and offered no way to opt-out. Adding it via manually as needed would give theme authors much more control:

layout-padding

It's a little hard to visualize this (even in GIF form), so I created a CodePen to try this out give this a spin — I'm eager to know if I missed anything important here...

https://codepen.io/kjellr/full/porBBdY

@jasmussen
Copy link
Contributor

Thanks for thinking about this, and thanks for the very helpful codepen as well!

It's definitely a lot to wrap your head around. I think that may also speak to my primary concern with this approach: I'm not sure where to start, and it doesn't feel obvious how this would affect my theme. It's clearer when trying the codepen:
single toggle

But in that GIF, I can't help but feel like many of these concepts can be absorbed. "Ignore site padding" doesn't feel useful on its own, I could just set the site padding to 0. However if it was folded into "Use site padding for children", it would immediately make more sense. That is, I could see it as a single toggle.

Mostly, I feel like we should not fragment which blocks are "top level" vs. "children", especially not in the markup. Any solution we arrive at seems like it should know this without additional controls or values.

I realize that takes us back towards #35919, or even #36214. What were the primary concerns that blocked those, and can we address them? (CC @youknowriad)

The utopian dream feels like a single toggle control that applies both your "ignore site padding" and "use site padding for children" at the same time, targets only to the top level blocks that need it, and doesn't need additional markup. As a strawman idea:

  • Keep the site padding variable.
  • Add a toggle below where that control sits, something like "Absorb padding for top level blocks"

With the toggle off, the padding is kept on the body element as is currently the case. But what happens when that toggle is flipped? Some ideas:

  1. Padding is kept on the body element, but fullwide elements get negative margins and inherited padding.
  2. Padding is removed on the body element entirely, and applied to the left and right of every top level block. Possibly also to the top of the first block and bottom of the last block.

I can imagine edgecases, pros, and cons for both 1 and 2. But I really think it's worth it to seek out solutions that an boil down to a single boolean toggle with no markup changes. What do you think?

@youknowriad
Copy link
Contributor

I realize that takes us back towards #35919, or even #36214. What were the primary concerns that blocked those, and can we address them? (CC @youknowriad)

Personally I think #36214 is my preferred option for the moment, I need buy in and input from everyone there.

@kjellr
Copy link
Contributor Author

kjellr commented Nov 23, 2021

But in that GIF, I can't help but feel like many of these concepts can be absorbed. "Ignore site padding" doesn't feel useful on its own, I could just set the site padding to 0. However if it was folded into "Use site padding for children", it would immediately make more sense. That is, I could see it as a single toggle.

The reason it's tied to a single block there (and not just set to 0) is because you probably don't want all top-level blocks to ignore the site padding. If you did, you wouldn't enter site padding in the first place. 😄

I actually did originally build it so that the "site padding for children" styles were applied automatically when the "Ignore site padding" toggle was active. But that didn't work in all cases either. Sometimes you don't want those paddings active. This is actually my primary concern with #36214: that you can't turn off or edit the padding that's added to child blocks. It ends up being especially awkward when you don't enter a top/bottom padding value for your site. In that case, every block that has layout applied cannot have top/bottom padding either.

Anyway, the fact that this didn't "just work" in your head is reason enough to move on from it. This whole layout feature feels far too hard to grasp as it is, so I don't want to add additional complicated controls on top of it.

@jasmussen
Copy link
Contributor

you probably don't want all top-level blocks to ignore the site padding

This is actually my primary concern with #36214: that you can't turn off or edit the padding that's added to child blocks

Forgive my silly question, consider it a provocation to make sure we look at all the corners of the problem: why wouldn't we want all top level blocks to ignore the site padding? Consider that this would be paired with said site-padding being applied inside the block instead, at least in strawman option number 2 in this comment.

I'd also like to think that padding as a control is something that can/should/will eventually come to all blocks, and if it did wouldn't that let you always be able to override whatever padding might be inherited on a top level block like this?

@kjellr
Copy link
Contributor Author

kjellr commented Nov 23, 2021

Forgive my silly question, consider it a provocation to make sure we look at all the corners of the problem: why wouldn't we want all top level blocks to ignore the site padding? Consider that this would be paired with said site-padding being applied inside the block instead, at least in strawman option number 2 in this comment.

It's a good question! Technically, all blocks in the site editor are "full-width" by default, so why wouldn't we have them all ignore the site padding? I'm pretty sure this is basically the approach that #36214 takes — when I activate it, all root-level blocks in the site editor still go edge-to-edge.

I'd also like to think that padding as a control is something that can/should/will eventually come to all blocks, and if it did wouldn't that let you always be able to override whatever padding might be inherited on a top level block like this?

Yes, that should be the case. 👍 The issue I have with #36214 is that it currently adds default padding and then specifically removes the padding control when layout is active.

@scruffian
Copy link
Contributor

I think this is one way to solve it: #41377, but I'd like more input

@tellthemachines
Copy link
Contributor

Before adding another PR on top of the existing ones, I want to make sure we're all on the same page about this problem. Here's my understanding of it:

  • We want root padding to Just Work for all children of blocks that stretch full width of the viewport, except those children that are also full width.
  • The problem is we don't have any way of telling if a full width block actually stretches the full viewport width, as opposed to being, say, inside a column.

So, while something like the below is achievable by making root padding work only for direct grandchildren (children of full-width children) of wp-site-blocks:
Screen Shot 2022-06-27 at 6 14 13 pm
It doesn't fully solve the problem, because we would want the root padding value to also be applied to the children of Group I.

But, if we where to do something like

.alignfull > *:not(.alignfull) {
    padding-left: 19px;
    padding-right: 19px;
}

Then we end up with this:
Screen Shot 2022-06-27 at 6 17 01 pm
Where we may not want Groups C, F and G to have that extra padding on their children.

So, my questions are:

  • Does this description of the problem reflect everyone else's views of it? (If not, please let me know!)
  • Given the above is correct, would applying root padding also to children of elements that only stretch full width of their container (not the viewport) be an acceptable compromise?
  • What else might I be missing? 😄

@Andrew-Starr
Copy link

How I have been giving an effective padding solution (FSE only) is to give default content and wide content a maximum width of 100% the viewport minus some value, half of said value effectively becomes the padding either side when the viewport width is less than the block width.

e.g.

"layout": {
    "contentSize": "min( calc(100vw - 64px), 600px)",
    "wideSize": "min( calc(100vw - 64px), 1200px)"
}

This lets full width blocks go edge to edge full width while still giving the appearance of 32px padding for default or wide blocks when needed in a narrower viewport.

One issue I have recently encountered is that safecss_filter_attr() doesn't recognise min() or max() and returns empty, resulting in a broken layout but that is a separate issue.
https://core.trac.wordpress.org/ticket/55966

@scruffian
Copy link
Contributor

@tellthemachines, that sounds about right. There are some additional complexities:

We want root padding to Just Work for all children of blocks that stretch full width of the viewport, except those children that are also full width.

This feels a bit ambiguous, so I thought I'd try to restate it.... When a full width block stretches to the edge of the viewport, there should always be a gap between the edge of the viewport and the text, equal to the root padding size. If multiple full width blocks are stacked together the padding between the edge of the viewport and the text should remain the same as the root padding.

My understanding of the problem is this:

  1. At root level, all blocks are full width and should reach to the edge of the viewport. Text inside these blocks should have a padding around it equal to the site padding.
  2. When blocks are stacked they should continue to behave the same as root level blocks, unless they have their width constrained by either having a layout set or a padding set.
  3. If a block has a width constraint (via layout or padding) then blocks inside it are (sometimes) given the option to alignfull.
  4. If a block has the alignfull option selected then its width will no longer be constrained by its parent and it will stretch to the limits of its parent. The padding around the text inside this alignfull block will be set by the parent block.
  5. When a stack of blocks have the "inherit default layout" option selected, alignfull blocks within these will reach the edge of the viewport, since the default layout is inherited all the way down the stack.

So, while something like the below is achievable by making root padding work only for direct grandchildren (children of full-width children) of wp-site-blocks:

There is another reason why this may not work - it is not correct to assume that direct grandchildren of wp-site-blocks are necessarily going to even have an option to go full width. In many themes (including TwentyTwentyTwo) the first alignfull block that goes to the edge of the viewport is deeper into the stack. This is one of the challenges with root padding as opposed to block level padding - there is an unpredictable number of layers of blocks between the level at which the padding is set and the level at which the blocks go full width.

Another thing that makes this complicated is that we want different behaviour for different types of blocks. Images within image blocks should stretch to the full width of the parent container, whereas in group blocks the text within the block should have padding between it and the edge of the block. We also don't want to target specific blocks in the CSS as we need it to work with third party blocks. This makes it much more complex to apply fullwidth consistently.

Given the above is correct, would applying root padding also to children of elements that only stretch full width of their container (not the viewport) be an acceptable compromise?

I think so, with the follow caveats:

  1. Images, videos (non-text content) should not have a padding, in the same way it doesn't at the root)
  2. If a parent block sets a padding it should be used in place of the root padding.

Hope that helps....

@tellthemachines
Copy link
Contributor

Thanks for the input @scruffian ! I still have a few questions 😅

If multiple full width blocks are stacked together the padding between the edge of the viewport and the text should remain the same as the root padding

By "stacked" do you mean nested within each other? Or one above the other?

If a block has a width constraint (via layout or padding) then blocks inside it are (sometimes) given the option to alignfull.

Setting padding on a block by itself doesn't currently enable its descendants to use full width alignment, there has to be a change to content size (either inherited from the theme settings or set on that block). As far as I can tell, setting a content size always enables the descendants to opt in to full width if their block type allows it (e.g. a Paragraph can't go full width, but an Image can).

When a stack of blocks have the "inherit default layout" option selected, alignfull blocks within these will reach the edge of the viewport, since the default layout is inherited all the way down the stack.

This point isn't entirely clear. In my example above, Group D and all its children inherit default layout (I'm expressing that as "inherit content width" because the nomenclature has changed as of #41893). But, because Group E isn't configured to be full width, it and its children only stretch as far as the content width.

If we want a block within several layers of nesting to be full viewport width, then all its parents should both inherit content width (or set a custom one) AND be explicitly set to full width, all the way up to the direct child of wp-site-blocks (which should already be full width by default).

If the outermost block has no content width set, and there is a non-zero root padding value set, then its children won't be able to extend full width and will have to respect the root padding - such as with Group A in my above example.

Am I missing something?

Images, videos (non-text content) should not have a padding, in the same way it doesn't at the root)

I'm not sure I get this. I'd expect Image and Video blocks to have the same padding as Paragraph, unless they're explicitly set to full width.

@scruffian
Copy link
Contributor

scruffian commented Jun 29, 2022

By "stacked" do you mean nested within each other? Or one above the other?

Nested

Setting padding on a block by itself doesn't currently enable its descendants to use full width alignment, there has to be a change to content size (either inherited from the theme settings or set on that block). As far as I can tell, setting a content size always enables the descendants to opt in to full width if their block type allows it (e.g. a Paragraph can't go full width, but an Image can).

Agreed!

If we want a block within several layers of nesting to be full viewport width, then all its parents should both inherit content width (or set a custom one) AND be explicitly set to full width, all the way up to the direct child of wp-site-blocks (which should already be full width by default).

You're right I wasn't entirely clear about that, but this is correct.

If the outermost block has no content width set, and there is a non-zero root padding value set, then its children won't be able to extend full width and will have to respect the root padding - such as with Group A in my above example.

This is also correct, but this behaviour is what we want to change.

I'm not sure I get this. I'd expect Image and Video blocks to have the same padding as Paragraph, unless they're explicitly set to full width.

The difference is that text content inside fullwidth group and column blocks should have some padding between it and the edge of the viewport, where images and videos do not.

Expressed another way, we should maybe think about site padding as content padding - this padding should be applied to text content within block, but not to graphical content such as image, videos and background colors. This is difficult because we don't really have a way to detect the difference between these different types of block. This is something I suggested in #26407.

@tellthemachines
Copy link
Contributor

If the outermost block has no content width set, and there is a non-zero root padding value set, then its children won't be able to extend full width and will have to respect the root padding - such as with Group A in my above example.

This is also correct, but this behaviour is what we want to change.

Can you elaborate on that @scruffian? That's not what I'm getting from the conversation on this issue so far.

As I understand it, three different scenarios have been suggested above:

  • Have the root padding apply only inside of blocks that stretch to full viewport width (this from the opening comment);
  • Have the root padding apply inside all blocks that have layout/content width defined, whether they are full width or not (I think this was the purpose of Alternative approach to the layout outer padding #36214);
  • Allow blocks to opt in to root padding (I'm guessing not all blocks? this wasn't specified)

I'm going to ignore the third option for now, because it seems to have less proponents 😅 , and also because it would be cumbersome to have to apply padding to every single block where we needed it.

The problem with the second option is that blocks with layout have content width already, so the padding will only really have an effect on small viewports. But on large viewports, where the padding comes in really handy is in blocks without layout, such as in the TT2 header example above.

That leaves us with the first option, which is the direction I'm exploring in #42034.

Outermost blocks (direct children of wp-site-blocks) are full-width by default, so with the first option I'd expect them to have root padding inside them. So their children would always be limited by that padding. Personally I think that's fine, because if we want the children of an outermost block to go full width and have some root padding applied, we can always define layout/content width for the outer block.

(The exception to this is Template parts, where we can't define layout. That feels a bit awkward, especially as the template part is generating an HTML wrapper. Perhaps we could add the ability for template parts to define layout?)

The difference is that text content inside fullwidth group and column blocks should have some padding between it and the edge of the viewport, where images and videos do not.

I'm afraid I still don't get it 😅 . I'd expect images and videos to align with the text content, unless explicitly set to full or wide width. That's what usually happens on blog posts and articles with mixed text and media content. Could you give me an example of a situation where we would need all media blocks to be full width by default?

@scruffian
Copy link
Contributor

I'm afraid I still don't get it 😅 . I'd expect images and videos to align with the text content, unless explicitly set to full or wide width. That's what usually happens on blog posts and articles with mixed text and media content. Could you give me an example of a situation where we would need all media blocks to be full width by default?

This is what happens for these two "alignfull" blocks in TwentyTwentyTwo:
Screenshot 2022-06-30 at 13 02 19
Screenshot 2022-06-30 at 13 00 53

As you can see, "alignfull" images stretch to the edge of the viewport, whereas the text inside the group block has padding.

@scruffian
Copy link
Contributor

That leaves us with the first option, which is the direction I'm exploring in #42034.

The issue with the solution in #42034 is the assumptions it makes about the level at which alignfull block are nested. You can see this by testing it on TwentyTwentyTwo with this PR. When you compare that PR + #42034 with the results on trunk, the results for alignfull blocks are quite different.

@scruffian
Copy link
Contributor

The issue with the solution in #42034 is the assumptions it makes about the level at which alignfull block are nested. You can see this by testing it on TwentyTwentyTwo with WordPress/wordpress-develop#2578. When you compare that PR + #42034 with the results on trunk, the results for alignfull blocks are quite different.

I tried combining the solution in #42034 with #39871 and still found the same issues.

@tellthemachines
Copy link
Contributor

The issue with the solution in #42034 is the assumptions it makes about the level at which alignfull block are nested. You can see this by testing it on TwentyTwentyTwo with WordPress/wordpress-develop#2578. When you compare that PR + #42034 with the results on trunk, the results for alignfull blocks are quite different.

The only assumption #42034 makes is that alignfull blocks need to be nested inside blocks with layout all the way down. Testing that PR on TT2 with WordPress/wordpress-develop#2578 applied, the only problem I see is alignfull items in the post content aren't stretching to the full width of the viewport. That happens because the outer Group block that post content is nested in (the one outputting the main tag) doesn't have layout set, so its children can't be explicitly set to full width. If you were to add layout to the main group and set its children to alignfull, everything would work as expected.

This is not a super intuitive solution to the problem, but it's better than what the custom code in TT2 is doing. Why?

TT2 sets padding at the root level, and then adds negative margins to all alignfull blocks, with some exceptions. The problem is alignfull blocks don't always go the full width of the viewport, and while the exceptions in the TT2 code take some of these scenarios into account - such as alignfull blocks inside columns - they can't possibly cover all of them.

For instance, this is is an alignfull image inside a group with layout inside a row:

Screen Shot 2022-07-01 at 2 00 32 pm

It's a somewhat abstruse example, but it would work fairly well without those negative margins:

Screen Shot 2022-07-01 at 2 15 47 pm

What I mean is we shouldn't try to second guess what layouts our users are going to come up with. Adding a CSS rule that doesn't quite work, and then hard coding a bunch of exceptions to it is always going to be fragile. Doing that in a theme just means the theme will be a little bit broken (which it also shouldn't, especially given it's a default theme) but doing it in Core will break that behaviour for everyone, independent of which theme they use. We can't afford to do that.

If, instead of #42034, we were to go with solution 2 I outlined above (apply root padding only to blocks with layout, and negative margins to alignfull children of those blocks), then we'd have a different kind of breakage on TT2: blocks with no layout defined would suddenly have their content bumping up against the sides of the viewport. (This could also be solved easily by setting layout on the wrapper block, and possibly giving it a custom content width if we don't want it to have the default one.)

Screen Shot 2022-07-01 at 2 02 56 pm

I'm not sure if having root padding not work at all on blocks without layout is ideal, but if that's what everyone thinks is best, then I'm happy to work on it 🤷

I tried combining the solution in #42034 with #39871 and still found the same issues.

Yeah, #39871 only addresses padding in blocks with layout, so it wouldn't work to anull the root padding #42034 is providing to blocks without layout. I'm also not convinced about the implementation in #39871 - the way it overrides user-set padding seems a bit too aggressive to me.

@tellthemachines
Copy link
Contributor

Ok I've created a draft in #42085 with solution 2: applying root padding only to blocks with layout. Feel free to test and give feedback!

@scruffian
Copy link
Contributor

If you were to add layout to the main group and set its children to alignfull, everything would work as expected.

I've tested #42034 against the TT2 PR, set the main group to alignfull and post content to alignfull, and as you say it works as expected. The only exception is the same issue that I have in my suggested PR (#41377), which might suggest that the test case itself is faulty.
Screenshot 2022-07-01 at 14 16 28

As you say, this solution relies on:

The only assumption #42034 makes is that alignfull blocks need to be nested inside blocks with layout all the way down.

I feel like this is not ideal for a couple of reasons:

  1. Until they are inside a "layout" container blocks already fill the full viewport width, so it feels like an unnecessary and meaningless addition to add layout to all the blocks in the nested structure. You being to wonder if there's ever a case when a block wouldn't need to have layout defined!

  2. It will be hard to understand and explain to themers that they need to add layout to all nested blocks and set them to alignfull, when blocks outside of layout are already full width.

If this is the only available solution then it's a compromise we'd have to accept, but I believe there are other options.

TT2 sets padding at the root level, and then adds negative margins to all alignfull blocks, with some exceptions. The problem is alignfull blocks don't always go the full width of the viewport, and while the exceptions in the TT2 code take some of these scenarios into account - such as alignfull blocks inside columns - they can't possibly cover all of them.

I agree, we can't rely on custom CSS on a per-block basis, not least because we also need to support third party blocks.

What I mean is we shouldn't try to second guess what layouts our users are going to come up with

However, this I do not agree with. I think we should try to build a solution which is flexible enough to work as expected without enforcing layout constraints on our users.

Ok I've created a draft in #42085 with solution 2: applying root padding only to blocks with layout. Feel free to test and give feedback!

Thanks for creating this. I'm not sure this idea works either. The problem is that headers will often need to span the viewport and will want to use root padding. Of course we could add layout to them too, but then we basically would have layout on everything!

Let me try to summarize where I think we're at:

  1. Blocks fill the width of their parent,
  2. Unless they have a constrained width via a layout setting.
  3. Only blocks inside a "layout" block have the option to set "full" and "wide" alignments
  4. When a block has a "full" alignment it will stretch to the edge of its parent block (the edge here includes the internal padding of the parent block)

The exception to point 4 is that for the first alignfull block, the padding which needs to be compensated for is not the padding of the parent but the padding at the root level, since there could be multiple levels of blocks between the first alignfull blocks and the root padding.

@scruffian
Copy link
Contributor

Here's another idea....

Rather than trying to apply the padding to alignfull blocks in a global way, what if we set it as a property of the block itself. For example:

  1. If I create an alignfull block which stretches to the viewport edge, we hook into this action and also set a padding value on the block equivalent to the root padding.
  2. If I create an alignfull block which stretches to the edge of its parent, we hook into this action and also set a padding value on the block equivalent to the parent padding.

One advantage of this approach is that the padding is set explicitly and visible to the user in the block controls. It's also editable.

@DaveBoydy
Copy link

What if there was another variant to the group block with semantic naming that would distinguish it from generic group blocks and row, stack variants?

The intent behind the new variant group block would be to specifically stand out as the block which sets the sites root padding and as a default has 'Inherit default layout' set to on. Theme authors could specifically target the new block from theme.json to set custom padding values.

This is just a little suggestion i'm not sure it's a good idea though perhaps it could make the application of side padding a little more intuitive for theme authors.

I'm against hard coding side padding like was done in Twenty twenty two, how can you possibly hard code padding that works for every theme, each having multiple possible variations in block layout?

That's why i believe it's best for theme authors to manually set padding at their own discretion, creating a variant group block specifically for setting the outer side padding could assist theme authors in visually distinguishing group blocks that should apply layout and padding from group blocks that should not.

@DaveBoydy
Copy link

Rather than trying to apply the padding to alignfull blocks in a global way, what if we set it as a property of the block itself.

This is how it should be done ! and i think what i've been trying to say, if i could set a custom padding value I.E. not set with the units px, %, rem, vw etc. But with a custom value like 'max(3.5vw, 16px)' from the property of the block itself that would be great !

I can define my own font size in theme.json like
{
"slug": "max-48",
"size": "clamp(2rem, 4vw, 3rem)",
"name": "minmax:32-48"
},
and then set it from a blocks typography size dropdown.

I would like to be able to do the same for padding.

@tellthemachines
Copy link
Contributor

Until they are inside a "layout" container blocks already fill the full viewport width, so it feels like an unnecessary and meaningless addition to add layout to all the blocks in the nested structure. You being to wonder if there's ever a case when a block wouldn't need to have layout defined!

They fill the viewport width, unless their container has padding 😅 but yeah, it does feel awkward to add content width to a bunch of blocks just to get a child somewhere down the tree to be full width.

Thanks for creating this. I'm not sure this idea works either. The problem is that headers will often need to span the viewport and will want to use root padding.

This is the case where we wouldn't need to have layout defined if we went with the first option 😁

If we don't want blocks without layout to get root padding by default, but neither do we want them to never have root padding, then the only compromise I can think of is adding a setting to these blocks that allows us to toggle root padding on them 🤷 (which was one of the initial ideas on this issue, I think from @jffng )

The exception to point 4 is that for the first alignfull block, the padding which needs to be compensated for is not the padding of the parent but the padding at the root level, since there could be multiple levels of blocks between the first alignfull blocks and the root padding.

Blocks without layout can still have padding set on them though. If one of the multiple levels of blocks between root and our first alignfull block has a custom padding set, how do we expect the alignfull block to behave? I'm not sure that it should still go full viewport width in that case.

Rather than trying to apply the padding to alignfull blocks in a global way, what if we set it as a property of the block itself. For example:

  1. If I create an alignfull block which stretches to the viewport edge, we hook into this action and also set a padding value on the block equivalent to the root padding.
  2. If I create an alignfull block which stretches to the edge of its parent, we hook into this action and also set a padding value on the block equivalent to the parent padding.

The problem is we have no way of knowing whether an alignfull block stretches to the viewport edge or not. This is why whatever we implement will need to work equally for all alignfull blocks.

I think the best we can do is use root padding as a base or default value for alignfull blocks, and then make sure that value can be overridden in the custom padding settings. That's sort of what was discussed here.

@tellthemachines
Copy link
Contributor

Ok, I added a global padding toggle to blocks without content width set in #42085:
Screen Shot 2022-07-04 at 3 56 54 pm

What remains to be done on that draft PR is making sure alignfull still works when the parent has custom padding (which currently doesn't work on trunk either).

@scruffian
Copy link
Contributor

Thanks for adding this @tellthemachines. What I like about this approach is:

  1. It's declarative - there's no behind the scenes magic
  2. It's controllable - so users can turn it on or off

@tellthemachines
Copy link
Contributor

Ok, I've fixed the issue with custom padding so alignfull blocks can still stretch full width, and marked #42085 ready for review. Would be good to get it tested with all possible scenarios, to make sure no edge cases have been missed.

@DaveBoydy
Copy link

DaveBoydy commented Jul 5, 2022

Applies right and left padding to all blocks with content width set (whether default theme widths or custom values), and optionally to blocks that can use flow layout but don't have a content width set. Opt-in via a toggle in the sidebar

I don't think I agree with this in #42085: if i understand it correctly.

The padding should be set independently and manually, there are niche situations I believe where you want content width set but not padding.

Or you should be able to turn the padding off if it's automatically set with content width set. If 'Inherit default layout' is set to true on a post content block and padding automatically gets applied to it then there needs to be an option to turn the padding off.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment