WordPress / gutenberg

The Block Editor project for WordPress and beyond. Plugin is available from the official repository.
https://wordpress.org/gutenberg/
Other
10.3k stars 4.11k forks source link

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

Closed kjellr closed 2 years ago

kjellr commented 2 years ago

The root-level padding introduced in https://github.com/WordPress/gutenberg/pull/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 https://github.com/WordPress/twentytwentytwo/pull/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.

jffng commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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:

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

CSS grid has indeed been considered

The exploration in #16998 appears broader in that it conflates managing inner layout, too, whereas https://github.com/WordPress/gutenberg/issues/35607#issuecomment-944848693 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 https://github.com/WordPress/gutenberg/pull/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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

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:

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 commented 2 years ago

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:

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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.

DaveBoydy commented 2 years ago

Make all top-level blocks ignore site padding by default with the option to turn ignore padding off.

Would love the choice of being able to switch between different grid layouts E.G. standard layout with the header at the top, content at the middle and footer at the bottom to header at the side with content spanning from the top to the footer at the bottom.

glendaviesnz commented 2 years ago

@kjellr, @youknowriad do you know if anyone has explored using a transform like this to force a .alignfull to ignore root level padding?

This avoids having to know the padding in order to calculate negative margins, but there may be some other gotchas that I am overlooking.

youknowriad commented 2 years ago

@glendaviesnz the main problem that I see there is the width: 100vw. Align full might not been 100% of the viewport if used inside a column or something like that.

glendaviesnz commented 2 years ago

the main problem that I see there is the width: 100vw

ah, yeh, I was thinking it was only needed for the root level blocks - the 100vw also has the potential to force a horizontal scroll bar, so πŸ—‘οΈ that.

tellthemachines commented 2 years ago

Align full might not been 100% of the viewport if used inside a column or something like that.

Align full doesn't seem to be available for blocks nested inside other blocks though (say, a Group within another Group or within a Column). Can that be changed by theme settings, or in any other way? If not, we'd be safe to ignore the problem of full-aligned blocks inside regular-aligned blocks, at least for now.

I'm thinking this could be solved by having a root padding variable. We could set a non-zero default value for it, themes would be able to configure its value from theme.json and users would be able to override it from the global styles panel. Then we could set negative margins to the same value for all alignfull blocks.

What complicates this is that blocks at the root level (direct children of wp-site-blocks) don't have alignment options either. Could we turn them on for these blocks though? That way, if we did want a root block to ignore the sitewide padding we could just set it to align full.

I'm sure there are complexities to this that I don't know about, so please let me know if/how this won't work!

youknowriad commented 2 years ago

Align full doesn't seem to be available for blocks nested inside other blocks though (say, a Group within another Group or within a Column). Can that be changed by theme settings, or in any other way?

It is available if the container block (in this case "column" defines a layout).

I'm thinking this could be solved by having a root padding variable. We could set a non-zero default value for it, themes would be able to configure its value from theme.json and users would be able to override it from the global styles panel. Then we could set negative margins to the same value for all alignfull blocks.

This is basically what I tried in #36214, a bit more generic though since I have to support nested levels too

tellthemachines commented 2 years ago

It is available if the container block (in this case "column" defines a layout).

Oh I see. I can't get that to work in Column, but it works in Group. Only I don't get the full width option unless the parent Group is also set to full width - in which case I'd expect both parent and child to stretch to the limits of the viewport.

Is it possible to have a situation in which we wouldn't expect a block with full width set to actually be as wide as the viewport?

youknowriad commented 2 years ago

Is it possible to have a situation in which we wouldn't expect a block with full width set to actually be as wide as the viewport?

yes, full width right now doesn't mean "as wide as the viewport", it means "as wide as the container (in a container that defined a "default content width" for its children)"

tellthemachines commented 2 years ago

Is it possible to have a situation in which we wouldn't expect a block with full width set to actually be as wide as the viewport?

Ok, I can reproduce this now, but only with a parent Group block. It's still not working for Column.

In that case, we could base the negative margins on the parent container's padding, and if that doesn't exist (if parent is a root block) provide the root padding as a fallback value.

cbirdsong commented 2 years ago

I don't know how helpful this is since it doesn't take FSE/layout into account, but the root block container of my themes uses a root padding variable combined with width variables via calc(), like this:

// Values are set in both Sass and custom properties so they can be directly used as fallbacks for older browsers.
$content-width: 40rem;
$wide-width: 60rem;
$site-gutter-width: clamp(1rem, 1rem + 0.25vw, 2rem);

:root {
  --content-width: #{$content-width};
  --wide-width: #{$wide-width};
  --site-gutter-width: #{$site-gutter-width};
}

// If the root block container uses an element selector instead of a class it helps keep specificity under control.
body > * {
  // Base-level values for layout so content does not go full width on old browsers without support for 
  // custom properties, calc() or clamp()
  width: $content-width;
  max-width: 96%;

  width: calc(var(--content-width) - (var(--site-gutter-width) * 2));
  max-width: calc(100% - (var(--site-gutter-width) * 2));
  // These calcs keep the left edge of the text aligned with text inside an element with .has-background: 
  // https://user-images.githubusercontent.com/1672206/160871426-d4b5c574-a5b2-427a-894a-69e79caab38a.png
  // They do not take into account nested items with backgrounds.

  margin-left: auto;
  margin-right: auto;
}

// Using :where here to reduce specificity, but that may not be necessary.
// That means wide/full align will not work in browsers without support for :where.
// However, they will get a basic functional layout from the previous rule.
body > *:where(.has-background) {
  width: var(--content-width);
  max-width: 100%;
}
body > *:where(.alignwide) {
  width: calc(var(--wide-width) - (var(--site-gutter-width) * 2));
}
body > *:where(.alignwide.has-background) {
  width: var(--wide-width);
}
body > *:where(.alignfull) {
  width: calc(100% - (var(--site-gutter-width) * 2));
}
body > *:where(.alignfull.has-background) {
  width: 100%;
}

// Not using :where() here to keep enough specificity to override the default .has-background padding.
body > .has-background {
  padding: var(--site-gutter-width);
}

Codepen sample: https://codepen.io/cbirdsong/full/wvpqJyP

ndiego commented 2 years ago

@kjellr and @tellthemachines this issue is on the 6.0 Project Board. I see we have a PR that is being actively worked on, but it looks like it adds some new functionality to theme.json, which would disqualify it for inclusion in 6.0 at this point. Can you confirm that this is not intended for 6.0? If so, I will remove it from the board. Thanks!

jffng commented 2 years ago

Can you confirm that this is not intended for 6.0?

I believe the ideal was for it to be included with 6.0, but at this stage it seems hasty since the PR is still open and could use more testing.

ndiego commented 2 years ago

Agreed, thanks @jffng!

scruffian commented 2 years ago

I think this is one way to solve it: https://github.com/WordPress/gutenberg/pull/41377, but I'd like more input

tellthemachines commented 2 years ago

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:

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:

Andrew-Starr commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

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 https://github.com/WordPress/gutenberg/issues/26407.

tellthemachines commented 2 years ago

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:

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 commented 2 years ago

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 commented 2 years ago

That leaves us with the first option, which is the direction I'm exploring in https://github.com/WordPress/gutenberg/pull/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 commented 2 years ago

The issue with the solution in https://github.com/WordPress/gutenberg/pull/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 https://github.com/WordPress/wordpress-develop/pull/2578. When you compare that PR + https://github.com/WordPress/gutenberg/pull/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 commented 2 years ago

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 https://github.com/WordPress/wordpress-develop/pull/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 https://github.com/WordPress/wordpress-develop/pull/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 commented 2 years ago

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 commented 2 years ago

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 (https://github.com/WordPress/gutenberg/pull/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 https://github.com/WordPress/gutenberg/pull/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 https://github.com/WordPress/gutenberg/pull/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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 https://github.com/WordPress/gutenberg/pull/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.