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

Improve global padding to be more consistent #60308

Closed
richtabor opened this issue Mar 29, 2024 · 26 comments · Fixed by #60715
Closed

Improve global padding to be more consistent #60308

richtabor opened this issue Mar 29, 2024 · 26 comments · Fixed by #60715
Labels
[Feature] Layout Layout block support, its UI controls, and style output. [Type] Bug An existing feature does not function as intended

Comments

@richtabor
Copy link
Member

richtabor commented Mar 29, 2024

In exploring pattern interoperability across themes, I've noticed inconsistencies around how global padding is applied.

I'm just writing out a bit on each area patterns are leveraged, to lay out where the expectations are here.

I'm using base TT4 as a reference, with this markup (gist) as my pattern. The pattern is a fullwidth, constrained layout, Group block its inner blocks set to "wide". I'd consider this the standard "section" structure, where the pattern is fullwidth, but the contents are wide—representing the standard width of the page contents. There is no padding assigned to this pattern, as I'd expect global padding to be applied when it's in use.

Site Editor

Inserting the pattern on a template (meets expectations)

If we add that to the "Blog Home" template of TT4, inside the main wrapper Group block, the pattern performs as expected. It actually works as expected (with global padding) at the top-level as well—not just within the main Group.

Global padding is applied properly to the left and right bounds of the pattern—ensuring that the site's intended padding is represented within the pattern, without the pattern having to declare a value for left and right padding. Notice how the inner blocks of the pattern are as wide as the header template part's inner blocks (also using wide width).

CleanShot 2024-03-29 at 14 12 53

Isolated/focus mode (does not meet expectations)

If I take that same pattern and create a synced pattern from it, there are no changes to the display within the template, as expected, but once I select to edit this pattern (which invokes focus/isolated mode) the global padding is no longer present:

CleanShot 2024-03-29 at 14 26 14

As global padding is properly applied at the top level of a template, I'd expect the same behavior when in focus/isolated view, to have global padding applied.

Inserting the pattern on a page in the Site Editor (does not meet expectations)

TT4 uses a similar structure for the page template, as the blog home: a main wrapper Group block, set as "default ", with a core/post-content block set to a constrained layout. This way, editors can set blocks wide, or fullwidth—but the standard width is the site's contentWidth. This is a standard established template structure.

The problem is however, with this scenario, any patterns added to the post content will not receive the expected global padding values.

CleanShot 2024-03-29 at 14 41 46

The only way to have global padding assigned is to set the core/post-content block to a default layout—which is not appropriate, as users' text on a page will be 100% wide, and they'll loose the ability to choose width sizes.

I'd expect the global padding to apply in this scenario.

Post Editor (does not meet expectations)

Inserting the pattern on a page in the post editor

The pattern's global padding is omitted from the Post Editor, as it is within a page using Site Editor. I'd expect the global padding to apply in this scenario as well.

The difference between the Site Editor and the Post Editor, is that in the Site Editor when I changed the page template's core/post-content block layout to default, the global padding was applied to the pattern. There was no effect on global padding within the Post Editor.

CleanShot 2024-03-29 at 16 35 00

cc @tellthemachines, as I recall you worked on global padding at some point. :)

@richtabor richtabor added [Type] Bug An existing feature does not function as intended [Feature] Layout Layout block support, its UI controls, and style output. labels Mar 29, 2024
@richtabor
Copy link
Member Author

Ideally patterns should not have to define left and right padding values—in all scenarios—so that they look great inheriting the theme's left and right global padding values, as they inherit colors, typography, block spacing, etc.

This would help make the WordPress.org pattern directory much more purposeful, allowing for much more theme-agnostic patterns.

@fabiankaegy

This comment has been minimized.

@richtabor

This comment has been minimized.

@fabiankaegy

This comment has been minimized.

@richtabor

This comment has been minimized.

@fabiankaegy
Copy link
Member

@richtabor actually, I believe I found what the reason was I couldn't replicate.

In my barebones theme I'm using this setup for the settings.layout.contentSize & settings.layout.wideSize:

{
    "version": 2,
    "settings": {
        "layout": {
            "contentSize": "min(650px, calc(100vw - var(--wp--style--root--padding-left) - var(--wp--style--root--padding-right)))",
            "wideSize": "min(850px, calc(100vw - var(--wp--style--root--padding-left) - var(--wp--style--root--padding-right)))"
        }
    }
}

And because of that the thing that was supposed to be the root padding still works as you would expect it in my case. But it works because the max-width of all the elements is already taking that padding out.

With a plain tt4 installation I can replicate what you are describing and am confused why the root padding is not getting applied. But I tested with WordPress 6.4.3 and it is the same thing there.

@mikemcalister
Copy link

I've had the same experience with my Ollie theme. Early on, I thought for sure I must be doing something wrong, but after testing in multiple popular block themes, I noticed the same issue.

As Rich mentions, this breaks a fundamental feature of block themes by not relying on the root padding. Having to add the missing padding directly to patterns makes them far less portable and composable by other themes.

Users are also reporting confusion with this, expecting the root padding to work universally.

@tellthemachines
Copy link
Contributor

Global padding is only meant to apply to root or root-adjacent blocks, so I wouldn't expect it to be present on a pattern inserted into post content. I don't think patterns should rely on root padding as a rule, because it's context-specific and patterns should work when inserted an any point in the block tree.

When useRootPaddingAwareAlignments is enabled, the only thing it's meant to do is make sure that the root level padding (which without useRootPaddingAwareAlignments would be applied to the body element) is applied inside the outermost full-width block. In your post editor example, that block is Post Content, and adding another full width block inside it negates that padding. Unfortunately we can't support multiple nested blocks with root padding and full width, because that causes other types of unexpected behaviour, such as reported in #43095.

The behaviour in focus mode is also deliberate and was implemented to fix #51937.

@richtabor
Copy link
Member Author

I don't think patterns should rely on root padding as a rule, because it's context-specific and patterns should work when inserted an any point in the block tree.

The discrepancies on when padding is applied is not ideal. On one view, it's applied as expected (from the user's perspective), in the others, the result is not expected.

If patterns have to include their own left and right padding in this scenario, then patterns simply can't exist apart from a specific theme (as the Pattern Directory is attempting to allow). I don't think that's an acceptable omission.

@richtabor
Copy link
Member Author

Could Post Content not be treated as the top-level root?

@fabiankaegy
Copy link
Member

I always thought that any group block that has the layout set to constrained should get the root padding applied.

Constraining the max width without the root padding seems odd to me 🤔

@bridgetwes
Copy link

Constraining the max width without the root padding seems odd to me 🤔

I second this. When editing a Page, I have the same issue trying to get a full-width group block (to add full-width background color) with an inner paragraph (heading/etc) block. I have "Inner Block uses content width" checked on the containing Group block. The Inner paragraph block runs up to the edge on screens smaller than "contentSize" unless I manually give the inner block side padding. This grouping of blocks seems like a common scenario. Regular content editors will have difficulty remembering to add back (root/side) padding to inner blocks. When editing on a larger screen where the "content width" is shorter than your browser width, the missing (root/side) padding is not obvious.

I've created a couple Block Themes, and keep running into this issue. I thought it was something I was missing in my settings/template block configuration, but I realized TT4 behaves the same way.

@richtabor
Copy link
Member Author

Here's a follow-up video showing how outer padding is applied inconsistently, using this pattern for testing and TT4:

  1. Pattern inserted at the top level of a template.
  2. Pattern viewed in isolated/focus view.
  3. Pattern inserted at the top level of post content (same result on post editor).
layout.mp4

I'd expect all three to have outer padding.

The behaviour in focus mode is also deliberate and was implemented to fix #51937.

Yes, top/bottom editor padding should not be applied to focus mode. I found this related issue as well: #59742

@SaxonF
Copy link
Contributor

SaxonF commented Apr 3, 2024

I currently trip up a lot on this because of the lack of transparency around what's happening. I'm not sure I'm fully up to date with current behaviour but I'd like to see:

  • I don't like vertical and horizontal global padding results in different behaviours. From a glance, it looks as though if I set vertical padding it adds that to body, but horizontal padding is added to various blocks (like constrained layout groups). This relates to the point about transparency. I feel we need to create greater separation between these two settings because they do different things. e.g. "Set body padding" is one setting, and "Set default section(?) padding" as another. You can set horizontal and vertical padding for both. This would allow for greatest level of flexibility for theme authors. Default body padding is 0.
  • Always show padding value on blocks, even if its coming from global padding. If it is coming from global padding value, make sure we communicate that. I would hope the style inheritance work can help resolve that.
  • Full width patterns have a default padding set to whatever global is, this includes wrapper blocks in templates or post content block if set to full width
  • Constrained width patterns have a default padding set to whatever global is
  • Everything else has no padding by default

@fabiankaegy
Copy link
Member

Constrained width patterns have a default padding set to whatever global is

I just want to make sure that what you mean by that is that it uses the global setting. Not that it copies the global value into the pattern and saves it there. Updating this global value should affect all these instances you've described.

@SaxonF
Copy link
Contributor

SaxonF commented Apr 3, 2024

@fabiankaegy thats what I mean yep . The style inheritance work aims to show global values in inspector for greater transparency and highlight properties that have been updated locally. The same would apply here.

@richtabor
Copy link
Member Author

I feel we need to create greater separation between these two settings because they do different things. e.g. "Set body padding" is one setting, and "Set default section(?) padding" as another.

I see these as the same setting. Why would you have content different, whether it's a section of a site or not? Sure, if you are explicit in breaking out of the mold, but default behavior they're the same in the vast majority of cases.

@SaxonF
Copy link
Contributor

SaxonF commented Apr 4, 2024

@richtabor because the current behaviour is unpredictable (hence this discussion) and changes depending on horizontal vs vertical (I could be wrong here). Separating them into "body" and "section" settings is a bit more explicit and maps more closely to how someone thinks about CSS, which we don't always want to do but in this case I feel is beneficial for theme authors. Also lets you set global vertical padding on sections.

Just an idea and is not required for my other points to still stand. I don't feel strongly about it.

@bgardner
Copy link

Can someone shed some light on the purpose and use case of this CSS?

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

This seems to be the problematic piece, as far as I can tell. It is overriding this, which in the case of a full-width group that should have right/left padding applied:

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

@bgardner
Copy link

Worth noting that adding something like this to theme.json seems to work, overriding the first block in my comment above:

{
	"styles": {
		"blocks": {
			"core/group": {
				"css": ".alignfull {padding-left: var(--wp--custom--spacing--gap); padding-right: var(--wp--custom--spacing--gap);}"
			}
		}
	}
}

@bgardner
Copy link

Ping @mikemcalister to see if something like this would work for and solve the problems you've had with Ollie.

@mikemcalister
Copy link

@bgardner This might be a fix for this (and a clever one at that!), but I don't think this is the permanent fix we need here. We need a fix that doesn't require all themers to add this CSS to get consistent padding behavior.

I did a quick test but I'll dive in deeper shortly. Maybe there are some clues here to a bigger fix.

@bgardner
Copy link

@mikemcalister Yeah, I agree and that's exactly where @richtabor and I landed this afternoon. (We need the fix in core). Hence: #60715

@tellthemachines
Copy link
Contributor

I always thought that any group block that has the layout set to constrained should get the root padding applied.
Constraining the max width without the root padding seems odd to me 🤔

@fabiankaegy would it be acceptable to you if nested blocks with constrained layout resulted in duplicate padding? That was the initial behaviour implemented in #42085. Then bug reports like #43095 led us to add a check to the rule so that root padding only gets applied on the outermost block with constrained layout.

We can reverse the fix in #43842, but before we do that we should be very clear on the consequences:

  • breaking back compat for themes and websites relying on the current behaviour
  • nested blocks with constrained layout will acquire unexpected padding: this applies not only to Group but Cover, Post Content and even Column if it's set to constrained

The problem is useRootPaddingAwareAlignments attempts a "smart" behaviour, but it can never cater to every individual use-case. We can continue to tweak and try to improve it, but it's doubtful there will ever be a solution that pleases everyone, and it's not sustainable to seesaw back and forth forever between adding padding to nested blocks or not.

@fabiankaegy
Copy link
Member

I always thought that any group block that has the layout set to constrained should get the root padding applied.
Constraining the max width without the root padding seems odd to me 🤔

@fabiankaegy would it be acceptable to you if nested blocks with constrained layout resulted in duplicate padding? That was the initial behaviour implemented in #42085. Then bug reports like #43095 led us to add a check to the rule so that root padding only gets applied on the outermost block with constrained layout.

We can reverse the fix in #43842, but before we do that we should be very clear on the consequences:

  • breaking back compat for themes and websites relying on the current behaviour
  • nested blocks with constrained layout will acquire unexpected padding: this applies not only to Group but Cover, Post Content and even Column if it's set to constrained

The problem is useRootPaddingAwareAlignments attempts a "smart" behaviour, but it can never cater to every individual use-case. We can continue to tweak and try to improve it, but it's doubtful there will ever be a solution that pleases everyone, and it's not sustainable to seesaw back and forth forever between adding padding to nested blocks or not.

@tellthemachines I agree it's not ideal. And the back compat question is one I don't have a great answer for. To me, the useRootPaddingAwareAlignments should be applied to all constrained containers that themselves have an alignment. (Or that are loving completely outside of a constrained environment like top-level blocks in a template). Having to manually include horizontal padding in every single pattern creates a huge maintenance/consistency issue because you cannot simply change the outer spacing in one go.

The way I solved this issue in my block based themes is as follows:

My content & wide with layout options reference a custom property:

{
	"settings": {
		"layout": {
			"contentSize": "min(650px, var(--wp--custom--site-content-width))",
			"wideSize": "min(1200px, var(--wp--custom--site-content-width))"
		}
	}
}

This variable intern is powered by a few custom properties:

{
	"settings": {
		"custom": {
			"scrollbar-width": "0px", // this variable gets dynamically set with JS to ensure to factor in different scroll bar widths on different browsers 
			"full-viewport-width": "calc(100vw - var(--wp--custom--scrollbar-width, 0px))",
			"site-outer-padding": "max(var(--wp--preset--spacing--s, 1rem), env(safe-area-inset-left))",
			"site-content-width": "calc(var(--wp--custom--full-viewport-width) - (2 * var(--wp--custom--site-outer-padding)))",
			"main-content-width-side-spacing": "calc((var(--wp--custom--full-viewport-width) - var(--wp--style--global--content-size)) / 2)",
			"main-wide-width-side-spacing": "calc((var(--wp--custom--full-viewport-width) - var(--wp--style--global--wide-size)) / 2)"
		}
	}
}

The outer padding that is defined in that custom property here also gets referenced by the root padding:

{
	"styles": {
		"spacing": {
			"padding": {
				"bottom": "0",
				"left": "var(--wp--custom--site-outer-padding)",
				"right": "var(--wp--custom--site-outer-padding)",
				"top": "0"
			}
		}
	}
}

With that setup I then re-enable the root padding on all blocks that have the root padding class:

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

@richtabor
Copy link
Member Author

@fabiankaegy would it be acceptable to you if nested blocks with constrained layout resulted in duplicate padding? That was the initial behaviour implemented in #42085. Then bug reports like #43095 led us to add a check to the rule so that root padding only gets applied on the outermost block with constrained layout.

We can reverse the fix in #43842, but before we do that we should be very clear on the consequences:

breaking back compat for themes and websites relying on the current behaviour
nested blocks with constrained layout will acquire unexpected padding: this applies not only to Group but Cover, Post Content and even Column if it's set to constrained

I started exploring a potential solution in #60715 that does not result in duplicate padding.

justintadlock added a commit to x3p0-dev/x3p0-ideas that referenced this issue Jun 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Layout Layout block support, its UI controls, and style output. [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants