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

Layout: layout styles overriding block axial margin values in theme.json #43404

Closed
ramonjd opened this issue Aug 19, 2022 · 13 comments
Closed

Layout: layout styles overriding block axial margin values in theme.json #43404

ramonjd opened this issue Aug 19, 2022 · 13 comments
Assignees
Labels
[Feature] Layout Layout block support, its UI controls, and style output. 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

@ramonjd
Copy link
Member

ramonjd commented Aug 19, 2022

Possibly related to:

Description

Blocks with top or bottom margin values specified in theme.json will have those values overwritten by flow layout CSS.

Layouts can specify CSS in theme.json.

These layouts are printed to a stylesheet here: https://github.com/WordPress/gutenberg/blob/c76c87e9f1ec22578436d8e24969a29c568c9cd0/lib/compat/wordpress-6.1/class-wp-theme-json-6-1.php#L1369-L1368

The flow layout styles body .is-layout-flow > * + * and body .is-layout-flow > * contain margin-block-start and margin-block-end declarations and have a specificity score of 0,1,1.

These rules are overwriting the .wp-block-{ $blockName } rule.

Screen Shot 2022-08-19 at 8 41 40 am

Which should have precedence in this case?

Context: #43365 (comment)

Step-by-step reproduction instructions

In a block theme create a post with the following HTML containing heading blocks:

<!-- wp:heading -->
<h2>A heading</h2>
<!-- /wp:heading -->

<!-- wp:group -->
<div class="wp-block-group"><!-- wp:heading -->
<h2>A heading inside a group block</h2>
<!-- /wp:heading --></div>
<!-- /wp:group -->

In your theme.json give heading blocks a top and bottom margin:

{
	"version": 2,
	"settings": {
		"appearanceTools": true
	},
	"styles": {
		"blocks": {
			"core/heading": {
				"spacing": {
					"margin": {
						"top": "30px",
						"bottom": "40px"
					}
				}
			}
		}
	}
}

What I expected

That the heading, both the heading in the group block and the heading outside, had a top and bottom margin equal to that defined in theme.json.

What happened

The wrapper's layout spacing rules take precedence.

Local expert @andrewserong offered the following advice:

From my perspective, I think the spacing of the wrapper that a block is in should take precedence over block-level-in-theme.json margin, and block level in individual block in post content should take precedence over the wrapper block's spacing.

I reckon it might be worth us opening an issue to see if it's worth coming up with a solution for the Group block — possibly an "unset" setting for spacing / block gap, or a layout type that is "no layout"?

Screenshots, screen recording, code snippet

No response

Environment info

Gutenberg 13.9
WP 6.0

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

@ramonjd ramonjd 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 19, 2022
@andrewserong
Copy link
Contributor

andrewserong commented Aug 19, 2022

Thanks for writing this up!

For a little more context, back in #42518 I had a go at removing the body prefix from the base layout rules. Unfortunately that resulted in individual block's margin rules that are shipped as part of the block CSS taking precedence over the Layout rules, which is undesirable — in that case, we want the wrapper / container block's block spacing settings to take priority. There's an example of what happened there in this comment: #42518 (comment).

So, if memory serves me correctly, that kind of suggests to potential (and different) directions forward to come up with a solution for it.

  1. Remove all margin rules from core block CSS (this would be a breaking change, so doesn't sound very do-able or easily achievable at first glance), and then lower the specificity of the base layout rules or:
  2. Explore whether it'd be desired to have either a "no layout" layout type, or the ability to (somehow) set an explicit "unset" value for block spacing on a particular container, so that the children don't have margins or other rules applied to them.

I think there's probably a bit of a balancing act in ensuring that Layout rules "just work", while also allowing all other cases of blocks (either in theme.json or at the individual level in post content) to override things where appropriate, and margin appears to be a pretty tricky area 😅

Curious to hear if anyone has thoughts about the ideal relationship between all these styles / how Layout could play nicely with its style neighbours 🤔

@tellthemachines
Copy link
Contributor

Related: #44736 #33795 and #40813

@andrewserong
Copy link
Contributor

Just an update to say I haven't forgotten about this one! I've opened up a fresh draft PR (#45927 — it currently does not work) to try to dig into the issue of specificity of layout styles. Even with the reduction in specificity, it still doesn't work properly, so I'm wondering if there might be more to it than just specificity (for example, if the order in which global styles is output from base layout versus block-level margin rules is also a factor).

I might not have much time immediately to dig into this further (I'm currently focusing on other Phase 2 tasks), but hopefully at least having a PR up will help nudge along the investigation.

@t-hamano
Copy link
Contributor

t-hamano commented Jan 11, 2023

Maybe I am missing something, but what about using :where pseudo-class?
I would expect that the global style margin would take precedence while overriding the browser's default margins.

:where(body .is-layout-constrained > * + *) {
    margin-block-start: 1.5rem;
    margin-block-end: 0;
}

:where(body .is-layout-constrained > *) {
    margin-block-start: 0;
    margin-block-end: 0;
}

@antonyjsmith
Copy link

Since this is a known issue and the control basically does nothing it's super misleading to a user, surely the global margin control should have been disabled until it's fixed?

@t-hamano your :where solution seems solid to me!

@andrewserong
Copy link
Contributor

Maybe I am missing something, but what about using :where pseudo-class?

Interesting idea! Thanks for the suggestion — it'd be worth trying out, but we'd need to be careful to look at blocks' default margin rules, as the layout rules need to override any block default margins, but not override block margins set at the block level in global styles, so it can be a bit tricky to work with there.

@t-hamano
Copy link
Contributor

I tried the :where pseudo-element approach with the constrained layout for a start.

  • Child elements of the constrained box respect the axial margins defined in theme.json or global styles
  • Only the first child element resets the top margin
  • Only the last child element resets the bottom margin

test_style

The following is the code that was changed in the test. In practice, we will need to consider all layouts and various cases, but this may be one solution.

Diff
diff --git a/lib/class-wp-theme-json-gutenberg.php b/lib/class-wp-theme-json-gutenberg.php
index 9acef6006d..ac12447f1b 100644
--- a/lib/class-wp-theme-json-gutenberg.php
+++ b/lib/class-wp-theme-json-gutenberg.php
@@ -1249,7 +1249,7 @@ class WP_Theme_JSON_Gutenberg {
                                                                                $spacing_rule['selector']
                                                                        );
                                                                } else {
-                                                                       $format          = static::ROOT_BLOCK_SELECTOR === $selector ? '%s .%s%s' : '%s.%s%s';
+                                                                       $format          = static::ROOT_BLOCK_SELECTOR === $selector ? ':where(%s .%s%s)' : '%s.%s%s';
                                                                        $layout_selector = sprintf(
                                                                                $format,
                                                                                $selector,
diff --git a/lib/theme.json b/lib/theme.json
index 6221873a03..a16346945d 100644
--- a/lib/theme.json
+++ b/lib/theme.json
@@ -299,12 +299,24 @@
                                                                "margin-block-end": "0"
                                                        }
                                                },
+                                               {
+                                                       "selector": " > *:first-child",
+                                                       "rules": {
+                                                               "margin-block-start": "0!important"
+                                                       }
+                                               },
                                                {
                                                        "selector": " > * + *",
                                                        "rules": {
                                                                "margin-block-start": null,
                                                                "margin-block-end": "0"
                                                        }
+                                               },
+                                               {
+                                                       "selector": " > *:last-child",
+                                                       "rules": {
+                                                               "margin-block-end": "0!important"
+                                                       }
                                                }
                                        ]
                                },

@andrewserong
Copy link
Contributor

Thanks for diving in further @t-hamano! Building on your idea there, I'm wondering if there's potentially another path forward that could avoid having to add the !important rules to the existing layout output.

I haven't tried this yet, but I'm imagining something like:

  • We continue to output the current layout rules as-is, without the :where().
  • For blocks that set margin rules, we output an additional layout rule specifically for that block. So, if a paragraph block were set to have a top margin of 2rem, we'd output an additional rule like: .is-layout-constrained > * + p { margin-top: 2rem; }?
  • If it works, we could contextualise it as either layout-aware margin rules, or margin aware layout rules.

What do you think of that kind of direction as an idea?

@t-hamano
Copy link
Contributor

@andrewserong

For blocks that set margin rules, we output an additional layout rule specifically for that block.

This may be a good approach! This could be achieved by simply adding new output rules without changing the existing output logic.

@andrewserong
Copy link
Contributor

This may be a good approach! This could be achieved by simply adding new output rules without changing the existing output logic.

I've had a go at this and opened up a draft PR over in #47399 — happy for any feedback or ideas if anyone has time to take a look 🙂

@t-hamano
Copy link
Contributor

t-hamano commented Feb 2, 2023

I have found two topics related to this issue, both of which are relevant to this issue. #47399 may solve these problems as well.

@tresorama
Copy link

Related discussion about blockGap #47935

@tellthemachines
Copy link
Contributor

Closing this issue now that #47858 is merged.

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. 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

7 participants