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

Theme.json: Margin for elements is overridden by layout margin #64785

Open
t-hamano opened this issue Aug 25, 2024 · 7 comments
Open

Theme.json: Margin for elements is overridden by layout margin #64785

t-hamano opened this issue Aug 25, 2024 · 7 comments
Labels
Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json [Type] Bug An existing feature does not function as intended

Comments

@t-hamano
Copy link
Contributor

t-hamano commented Aug 25, 2024

When developing a theme, I might define a style like this to allow more space above the Heading block element, for example:

{
	"version": 3,
	"settings": {
		"appearanceTools": true,
		"layout": {
			"contentSize": "840px",
			"wideSize": "1100px"
		}
	},
	"styles": {
		"spacing": {
			"blockGap": "1.5em"
		},
		"elements": {
			"heading": {
				"spacing": {
					"margin": {
						"top": "3em"
					}
				}
			}
		}
	}
}

What I expect from this definition is the following behavior:

  • The first element in the constrained layout has a top margin of zero.
  • The top margin of all elements other than the Heading block is 1.5em.
  • Only the Heading block has a top margin of 3em.

This worked as expected up until WP6.5 because the margin for the Heading block has a higher CSS specificity than the margin for the constrained layout:

// 0-0-1
h1, h2, h3, h4, h5, h6 {
    margin-top: 3em;
}
// 0-0-0
:where(body .is-layout-constrained) > * {
    margin-block-start: 1.5em;
    margin-block-end: 0;
}

image


However, in WP 6.6.1, the CSS selectors generated from the element level are less specific, so the margin of Heading block is no longer applied:

// 0-1-0
.is-layout-constrained > * {
    margin-block-start: 1.5em;
}
// 0-0-1
h1, h2, h3, h4, h5, h6 {
    margin-top: 3em;
}

image


The only solution I know is to define margin on blocks instead of elements:

{
	// ...
	"styles": {
		"blocks": {
			"core/heading": {
				"spacing": {
					"margin": {
						"top": "3em"
					}
				}
			}
		}
	}
}
// 0-1-0
:root :where(.wp-block-heading) {
    margin-top: 3em;
}

// 0-1-0
.is-layout-constrained > * {
    margin-block-start: 1.5em;
}
@t-hamano t-hamano added [Type] Bug An existing feature does not function as intended Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json labels Aug 25, 2024
@t-hamano
Copy link
Contributor Author

t-hamano commented Aug 25, 2024

As I understand it, the CSS specificity of element-level selectors was 0-1-0 in WP 6.6.0. Later, text underline was unintentionally applied to the a element, so to avoid this, the CSS specificity was changed back to 0-0-1 in #63403. If the problem reported in #63345 is limited to the a element, one solution might be to change the specificity of selectors other than the a element back to 0-1-0.

@aaronrobertshaw
Copy link
Contributor

Thanks for writing up the issue @t-hamano 👍

I might define a style like this to allow more space above the Heading block, for example:

The example theme.json provided adds heading element styles not styles for the Heading block as referred to here and in the following expected behaviour list.

This doesn't mean there isn't a problem but it is an important distinction to make for others coming to this issue.

The only solution I know is to define margin on blocks instead of elements:

Agreed. That's the only workaround that springs to mind for me also.

I'm not a layout support expert so perhaps @andrewserong could correct me here.

My understanding is that global block type styles should be able to override layout margins. It is less clear, to me at least, whether elements should be able to do the same and whether it was intended behaviour previously.

Backwards compatibility is key, so if there is a way to restore that while keeping all expected behaviour, count me in. In case it helps there is some extra history and context here.

If the problem reported in #63345 is limited to the a element

Unfortunately, the issue in #63345 could be any top level element not just a.

@andrewserong
Copy link
Contributor

I'm not a layout support expert so perhaps @andrewserong could correct me here.
My understanding is that global block type styles should be able to override layout margins. It is less clear, to me at least, whether elements should be able to do the same and whether it was intended behaviour previously.

That's my understanding, too.

As far as I understand, the work that went in to adjusting layout and global styles rules in order to support margin rules overriding layout was specifically about that happening at the block level rather than the element level. While it might have worked previously to do so at the element level, I don't believe that was the original intention. Arguably, depending on the theme, folks might expect that a layout container's default block spacing should override any element margins that might be set.

I think this might make it tricky to pursue a fix, as it doesn't seem clear to me that changing it here would always be perceived as a fix 🤔

The only solution I know is to define margin on blocks instead of elements:

Yes, I'd argue that defining margins on the heading block instead of elements is probably the more correct way to achieve this kind of styling in theme.json. Also, setting margins on heading elements is only supported in theme.json and not via the UI, which might make this issue less visible to folks as themes can use the heading block instead.

That's just my 2 cents, though. Thanks for the ping!

@t-hamano
Copy link
Contributor Author

Thank you everyone for the detailed explanations.

One idea: what do you think about an approach where only layout-related styles have a specificity of 0-1-0?

As an example, let me provide the following theme.json.

theme.json
{
	"version": 3,
	"settings": {
		"appearanceTools": true,
		"layout": {
			"contentSize": "840px",
			"wideSize": "1100px"
		}
	},
	"styles": {
		"elements": {
			"heading": {
				"spacing": {
					"margin": {
						"top": "3em"
					}
				}
			},
			"link": {
				"typography": {
					"textDecoration": "underline"
				}
			}
		}
	}
}

This definition will output the following styles:

// 0-0-1
h1,
h2,
h3,
h4,
h5,
h6 {
    margin-top: 3em;
}

// 0-0-1
a:where(:not(.wp-element-button)) {
    text-decoration: underline;
}

I'm wondering if we could change these styles to the following:

// 0-1-0
:root :where(h1),
:root :where(h2),
:root :where(h3),
:root :where(h4),
:root :where(h5),
:root :where(h6) {
    margin-top: 3em;
}

// 0-0-1
a:where(:not(.wp-element-button)) {
    text-decoration: underline;
}

Are there any potential problems with this approach?

@andrewserong
Copy link
Contributor

Are there any potential problems with this approach?

Good question. 🤔 From my perspective two potential problems might be:

  • Added complexity that some rules have different specificity to others: this could be an issue in terms of predictability of which styles do what. It might be unexpected that other spacing rules like padding don't get the same treatment.
  • If there are themes using element spacing styles effectively as a kind of reset, and so would prefer or expect lower specificity.

I might be overly cautious here, but I don't feel certain that upping the specificity of margin rules for elements is necessarily the right way to go. Conceptually, do we think layout rules should win out over element styles, or should element styles win out over layout? To put it another way — why would a theme prefer to use margin rules on the heading element instead of the heading block? I don't mean to be a blocker here, but just want to understand what the overall problem is that we're trying to solve, and whether it's a bug we need to address, or how we guide theme developers to the appropriate rules for the use case 🙂

@aaronrobertshaw
Copy link
Contributor

Are there any potential problems with this approach?

I think Andrew captures all my initial thoughts nicely 👍

I might be overly cautious here, but I don't feel certain that upping the specificity of margin rules for elements is necessarily the right way to go

+1

@webexpr-dhenriet
Copy link

webexpr-dhenriet commented Sep 11, 2024

@t-hamano @andrewserong

I have the same problem (hybrid theme), and it freezes the margins of all elements (titles, paragraphs, etc...) at the value of blockGap. When you've got a few pages to correct, it's not a problem, but when you've got hundreds, it's very problematic.

I don't understand why blockGap controls the vertical alignment of all page elements. In my opinion, this should only apply to level 1 groups (not elements of a group or elements of a group within a group).
It used to work fine before, but CSS targeting has become a bit uncontrollable and far too complex, and with each new version you wonder what's going to break again.
As far as I'm concerned, a simple style sheet is all I need to adjust the margin of elements other than sections - no need for anything unmanageable.
So in the theme.json I set blockGap to null

"blockGap": null,

with a schema https://schemas.wp.org/wp/6.5/theme.json in version 2, even though it tells me it expects a string value, this has the effect of completely disabling CSS writing of is-layout-constrained > *, which means I can finally get back to something more controllable.

.main-container {

  > .wp-block-group {
    padding-top: var(--wp--custom--block-gap);
    padding-bottom: var(--wp--custom--block-gap);
  }
}

As a web agency, we can't afford to take the risk of editing hundreds of pages to correct the margins with each update. So now I'm going to work without a default blockGap, hoping that the css rules won't come back on the next update with the null value declared in the theme.json...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

No branches or pull requests

4 participants