Fix block gap in group blocks in classic themes #47451

carolinan · 2023-01-26T11:11:57Z

What?

In preparation for WordPress 6.2, appearance-tools theme support was made available.

With the theme support, but without theme.json, the block gap setting does not work with the group block, because the CSS is only applied to the inner container div, the wp-block-group__inner-container, not the inner blocks.
Update: This affects both classic themes and block themes with the above condition.

This PR adds the .wp-block-group__inner-container as an additional partial selector for block gap.

Closes #47386

Why?

To allow classic themes to take advantage of new features.

How?

Testing Instructions

With WordPress 6.2 alpha:

Install and activate Twenty Twenty.
Add add_theme_support( 'appearance-tools'); to the themes setup function in functions.php.
In the block editor, add one each of group, stack variation and row variation. (The latter two to confirm that nothing broke)
Set the block spacing setting to something visible, like 7.
Check if the block spacing is working. The editor and front should match.

Screenshots or screencast

andrewserong

Thanks for opening this PR @carolinan!

I'm wondering if we could support classic themes that opt in to appearance tools in a slightly different way. What if instead of adding additional layout rules, with the presence of the appearance tools setting, we instead skipped the wp_restore_group_inner_container behaviour (i.e. add a check to that function and return early)?

The support for the legacy group inner container markup was so that classic themes aren't adversely affected by new features in core/Gutenberg — however, if themes are opting in to use the appearance tools, I think it'd be good if they were also using the new markup, as it will help to keep the layout support a little more streamlined.

What do you think?

carolinan · 2023-02-01T00:43:24Z

I don't know how that would be possible in an already existing theme, where users already have been able to target this element with custom CSS. For example the bundled themes.

andrewserong · 2023-02-01T01:06:24Z

Ah, gotcha. In that case, though, if the themes are wishing to opt-in to new features, shouldn't they be decoupling from the legacy group block markup before adopting appearance tools? My understanding was that the goal for the back-compat code for legacy group blocks was to give themes more time before adopting new features. I think it'd be preferable to avoid that back-compat code growing to include additional rules being output by the layout support, as they're rules that would need to be maintained and would be redundant on block themes.

Apologies if I'm missing something, I haven't seen the discussions surrounding the appearance tools feature for classic themes, but my preference would be to try to avoid adding additional complexity to the layout support if it isn't needed, since the rules are already quite complex.

carolinan · 2023-02-01T01:15:34Z

I don't understand what "decoupling from the legacy markup" means and if that is something I should be able to do with a PR to the bundled themes.

andrewserong · 2023-02-01T01:22:43Z

I don't understand what "decoupling from the legacy markup" means and if that is something I should be able to do with a PR to the bundled themes.

Apologies, I'm not really too sure what the path forward would be here, as I'm still trying to understand the objective behind opting in to blockGap for Classic themes, as it seems like the wrong way around to me. My opinion is that for features that are designed for sites with theme.json files, such as block gap, it would be better for those themes wishing to adopt the features to iteratively adopt support for the blocks-theme way of doing things rather than add in extra code to the layout support for classic themes.

My main concerns are about the maintainability of the layout support, and the addition of code to target classic themes. Perhaps it would be better for the appearance tools feature in classic themes to skip blockGap?

carolinan · 2023-02-01T01:34:19Z

My main concern is that if blocks in the bundled classic themes are broken, that will discourage users from switching to using blocks. And broken blocks are what they are experiencing and reporting now.

The main problem with supporting blocks in the classic themes is not blockgap.
I think it would be a shame to not enable users to take advantage of it. But what is actually breaking right now is the other appearance tools.

carolinan · 2023-02-01T01:49:03Z

I would still like to understand what negative effects adding this selector would have. It is a small code change for a rather big win for users of classic themes.
It is also only affecting one block, and only one variation for that block.

andrewserong · 2023-02-01T02:42:40Z

I would still like to understand what negative effects adding this selector would have.

That's a very reasonable question. I'll try to share where I'm coming from. There are a few things to evaluate when deciding whether or not to introduce a change like this beyond the size of the code itself. While the size of the change is fairly minimal on the surface, some of the concerns that come to mind are:

The ability to change something once it's introduced
Whether the change is consistent with the intended scope of a feature and its future support
What the feature communicates about the scope of a feature
Whether or not folks will test for or maintain the feature once it's introduced

In this case, while the code change is small, it deviates from the intended use cases behind the feature it's a part of (layout) which was designed with block themes in mind, and it expands the scope of the backwards compatibility area of layout, which is an area that folks are attempting to reduce rather than increase.

So part of my concern is that this PR is less of a bug fix, and more an enhancement request, as it increases the intended scope of the block support. The way that's been recommended for themes to start using layout and block spacing features up until now has been to adopt theme.json — if that strategy has changed, then I think it would be better to look at the strategy behind design tools more holistically rather than adding in additional rules for classic themes, as once they're added, it likely won't possible to remove them.

This is all just my opinion, though, from working on the layout block support for a while, and running into lots of edge cases with it as it's a very complex feature. Small additions might appear to be simple in isolation, however I believe we need to be careful whenever we increase the scope to ensure that the direction is consistent with future intended usage.

Very happy to hear others' feedback if there are more opinions on all this!

tellthemachines · 2023-02-01T04:13:30Z

It's unclear what the best solution might be here because this is a feature that tries to meld two incompatible paradigms: classic themes and block themes. It may work well for design tools that apply to a single block, such as border, padding and line-height, but for layout-related controls such as block gap (and I'd argue even margin) it's not ideal, because classic themes already define such styles themselves.

I understand that it's useful to have Group, Row and Stack blocks in classic themes, but that tension between paradigms keeps popping up in the form of bugs and unexpected behaviour. For instance, Row block doesn't work at all in TT1 due to an overly-specific theme style.

But these thoughts can hardly help to solve the problem at hand 😅

I agree with @andrewserong that adding extra styles to the layout logic is not great, mainly due to the back-compat nature of the Group inner wrapper, and the transitional nature of the appearance tools support feature meaning this is a bit of an edge case.

Ideally, we would be able to scrap the group inner wrapper. But if we can't, how about adding the layout classes on the inner wrapper instead? We could add it somewhere in here, so the logic to support the edge case scenario will all be in one place.

carolinan · 2023-02-01T04:52:43Z

The feature, the theme support, was added in August: #43337

andrewserong · 2023-02-01T04:55:28Z

But if we can't, how about adding the layout classes on the inner wrapper instead? We could add it somewhere in here, so the logic to support the edge case scenario will all be in one place.

That sounds like a better compromise to me, in that the layout rules would be unchanged, it's just where they're being attached in Classic themes, so could be a better way to contain it.

carolinan · 2023-02-01T05:57:25Z

I think it is only this class that needs to be moved? is-layout-constrained

andrewserong · 2023-02-01T06:19:31Z

I think it is only this class that needs to be moved? is-layout-constrained

If you're injecting layout classnames to group, you might also need is-layout-flow since group can either be full width (flow) or constrained (also, would you need is-layout-flex if Row and Stack need to be considered?). You could probably manually look for whether the layout.type block attribute is set and output the hard-coded classname (either is-layout-flow or is-layout-constrained) in gutenberg_restore_group_inner_container (around where the inner container classname is output).

If you need to do a look-up from the layout definitions, though, another approach might be to borrow some of code from gutenberg_render_layout_support_flag to generate the classname from within gutenberg_restore_group_inner_container. It could be something like the following (I haven't tested this, this is just a quick copy / paste / edit 🙂):

	$global_settings = gutenberg_get_global_settings();
	$global_layout_settings = _wp_array_get( $global_settings, array( 'layout' ), null );
	$layout_definitions = _wp_array_get( $global_layout_settings, array( 'definitions' ), array() );
	$layout_type = _wp_array_get( $block, array( 'attrs', 'layout', 'type' ), null );
	if ( $layout_type && ! empty( $layout_definitions ) ) {
		$layout_classname = _wp_array_get( $layout_definitions, array( $layout_type, 'className' ), '' );
	} else {
		$layout_classname = _wp_array_get( $layout_definitions, array( 'default', 'className' ), '' );
	}

carolinan · 2023-02-01T07:07:22Z

The appearance tools enables margin, padding, block gap but not layout.
I think that means the flow class is never used?

Row and stack were implemented without the inner div, so they already work well with block gap.

carolinan · 2023-02-01T07:39:55Z

Only the default 24px spacing is working, and this should have been obvious to me: wp-container- class is on the outer div, and the gap value from the block setting is never output.

…ainer.

carolinan · 2023-02-01T12:30:17Z

...Or should I be using this as a reference? https://github.com/WordPress/gutenberg/pull/44600/files

andrewserong · 2023-02-02T00:47:11Z

and this should have been obvious to me: wp-container- class is on the outer div, and the gap value from the block setting is never output.

Ah, of course!

...Or should I be using this as a reference?

The HTML tag processor would be a neat way to do this actually, then you wouldn't need to construct the layout classnames manually. It should be possible to use get_attribute to grab the classnames from the outer wrapper, and then insert them to the inner wrapper. One potential caveat is that the tag processor is still under review for landing in core. For this Gutenberg PR, I think it's still a good time to use the tag processor, but I don't know if it's 100% confirmed that the tag processor will land in time for 6.2 (though the hope is that it will).

carolinan · 2023-02-02T06:20:15Z

I think it is better if we make a decision early to revert the appearance-tools theme support in core than to stress the solution.
The theme support is still in Gutenberg. So classic themes that use Gutenberg can still opt-in. But I don't think there is a big awareness of this, because there has been no error reports from users/developers. These problems were only found when testing for them specifically...

ndiego · 2023-02-07T12:42:13Z

@carolinan now that appearance tools are getting reverted via https://core.trac.wordpress.org/ticket/57649. Are you ok with me punting this to 6.3 on the project board?

carolinan · 2023-02-07T15:45:52Z

@carolinan now that appearance tools are getting reverted via https://core.trac.wordpress.org/ticket/57649. Are you ok with me punting this to 6.3 on the project board?

Yes, please, we need to discuss it further.

carolinan · 2023-02-08T05:10:38Z

So first step would be to decide if block gap should be supported in themes without theme.json.
From the conversations above, I believe the answer is no; it was never intended for these themes.

ndiego · 2023-06-12T19:25:35Z

Hey @carolinan, this PR is on the 6.3 project board since it was punted from 6.2, but it doesn't look like any progress has been made. It is ok if I remove it from the 6.3 board as well?

carolinan · 2023-06-13T02:49:10Z

@ndiego The problem is that the underlying issue still needs to be solved. I still need the project's decision-makers to look into the issue and decide whether classic themes can regain features like the link color controls. By removing it from the board the issue is only going to be more invisible.

ndiego · 2023-07-03T12:20:40Z

The problem is that the underlying issue still needs to be solved. I still need the project's decision-makers to look into the issue and decide whether classic themes can regain features like the link color controls. By removing it from the board the issue is only going to be more invisible.

cc @tellthemachines @ramonjd

carolinan · 2023-07-03T12:41:32Z

TLDR

The link color and border controls have their own theme support added in 6.3 and 16.2.
add_theme_support( 'appearance-tools'); which is already supported in Gutenberg, still enables block gap which is broken for the group block (and only the group block, it works elsewhere).

The remaining question is if that should be fixed, or if the block gap can be excluded from the theme support.

tellthemachines · 2023-07-04T01:01:54Z

This is not a straightforward bug fix, and it's not a regression from the previous WP release either, so it should be punted to the next release.

ramonjd · 2023-07-04T01:08:48Z

I've been testing this and noticed couple of things:

With the changes in this PR, Row and Stack do display block gap correctly in classic themes:

I might be doing something wrong, but the regular Group block however (constrained) isn't working for me. It looks like wp-block-group__inner-container needs to wp-container-5, or at least the styles:

.wp-container-5.wp-container-5.wp-container-5.wp-container-5 > * + * {
    margin-block-start: 100px;
    margin-block-end: 0;
}

To be honest I'm not confident enough to say either way. @tellthemachines and @andrewserong have good judgement in this area, which tells me it can probably be postponed until 6.4. That is, work on a fix in Gutenberg and stabilize it until then.

ramonjd · 2023-07-04T01:09:14Z

This is not a straightforward bug fix, and it's not a regression from the previous WP release either, so it should be punted to the next release.

Oh, just saw this was posted after I hit "Comment".

Thanks @tellthemachines

github-actions · 2023-08-07T11:14:42Z

This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress.

If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged.

If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack.

Thank you! ❤️

View changed files

❔ lib/block-supports/layout.php

carolinan · 2023-08-07T11:17:50Z

So is the consensus now that the block gap should be enabled for themes without theme.json?

github-actions · 2023-08-07T11:40:07Z

Flaky tests detected in 5fe9051.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5784393473
📝 Reported issues:

[Flaky Test] allows rotating using the crop tools #36904 in /test/e2e/specs/editor/blocks/image.spec.js

carolinan · 2023-09-12T06:33:07Z

This is not resolved, it is still unclear if the feature should be enabled for classic themes or not. Removing it from the 6.4 board.

lib/block-supports/layout.php

tellthemachines · 2023-11-22T01:32:28Z

Now that #56130 has been merged, we can close this PR. Thanks for all your work and patience with the endless back and forth on this one @carolinan!

Add group block inner-container as layout selector.

696a57b

carolinan marked this pull request as ready for review January 26, 2023 11:13

carolinan requested a review from spacedmonkey as a code owner January 26, 2023 11:13

carolinan requested a review from pbking January 26, 2023 11:14

carolinan added [Block] Group Affects the Group Block (and row, stack and grid variants) [Type] Bug An existing feature does not function as intended labels Jan 26, 2023

Update phpunit layout test

c7f8c79

carolinan requested a review from tellthemachines January 27, 2023 06:06

andrewserong reviewed Jan 31, 2023

View reviewed changes

Revert the selector changes and place the the class on the inner cont…

e654712

…ainer.

carolinan mentioned this pull request Feb 2, 2023

Add link color and border theme_support #47675

Closed

carolinan added Needs Decision Needs a decision to be actionable or relevant and removed [Type] Bug An existing feature does not function as intended labels Feb 8, 2023

This was referenced Jun 22, 2023

Add link color theme_support #51775

Merged

Add border theme_support #51777

Merged

Merge branch 'trunk' into try/classic-theme-block-gap

5fe9051

carolinan added the [Type] Bug An existing feature does not function as intended label Aug 7, 2023

Update layout.php

d0bc0d6

tellthemachines added the [Feature] Layout Layout block support, its UI controls, and style output. label Aug 8, 2023

carolinan and others added 3 commits September 12, 2023 08:34

Merge branch 'trunk' into try/classic-theme-block-gap

4de99fe

Update layout.php

d61e7a8

Merge branch 'trunk' into try/classic-theme-block-gap

d4d53c6

carolinan commented Sep 28, 2023

View reviewed changes

lib/block-supports/layout.php Outdated Show resolved Hide resolved

Update lib/block-supports/layout.php

8cb23c2

tellthemachines mentioned this pull request Nov 15, 2023

Add layout classes to legacy Group inner container #56130

Merged

andrewserong mentioned this pull request Nov 15, 2023

Classic themes and appearance-tools issues and blockers #56131

Closed

7 tasks

tellthemachines closed this Nov 22, 2023

youknowriad deleted the try/classic-theme-block-gap branch January 30, 2024 18:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix block gap in group blocks in classic themes #47451

Fix block gap in group blocks in classic themes #47451

carolinan commented Jan 26, 2023 •

edited

Loading

andrewserong left a comment

carolinan commented Feb 1, 2023

andrewserong commented Feb 1, 2023

carolinan commented Feb 1, 2023

andrewserong commented Feb 1, 2023

carolinan commented Feb 1, 2023 •

edited

Loading

carolinan commented Feb 1, 2023 •

edited

Loading

andrewserong commented Feb 1, 2023

tellthemachines commented Feb 1, 2023

carolinan commented Feb 1, 2023

andrewserong commented Feb 1, 2023 •

edited

Loading

carolinan commented Feb 1, 2023

andrewserong commented Feb 1, 2023

carolinan commented Feb 1, 2023

carolinan commented Feb 1, 2023

carolinan commented Feb 1, 2023

andrewserong commented Feb 2, 2023

carolinan commented Feb 2, 2023 •

edited

Loading

ndiego commented Feb 7, 2023

carolinan commented Feb 7, 2023

carolinan commented Feb 8, 2023

ndiego commented Jun 12, 2023

carolinan commented Jun 13, 2023

ndiego commented Jul 3, 2023

carolinan commented Jul 3, 2023 •

edited

Loading

tellthemachines commented Jul 4, 2023

ramonjd commented Jul 4, 2023

ramonjd commented Jul 4, 2023

github-actions bot commented Aug 7, 2023

carolinan commented Aug 7, 2023

github-actions bot commented Aug 7, 2023

carolinan commented Sep 12, 2023 •

edited

Loading

tellthemachines commented Nov 22, 2023

Fix block gap in group blocks in classic themes #47451

Fix block gap in group blocks in classic themes #47451

Conversation

carolinan commented Jan 26, 2023 • edited Loading

What?

Why?

How?

Testing Instructions

Screenshots or screencast

andrewserong left a comment

Choose a reason for hiding this comment

carolinan commented Feb 1, 2023

andrewserong commented Feb 1, 2023

carolinan commented Feb 1, 2023

andrewserong commented Feb 1, 2023

carolinan commented Feb 1, 2023 • edited Loading

carolinan commented Feb 1, 2023 • edited Loading

andrewserong commented Feb 1, 2023

tellthemachines commented Feb 1, 2023

carolinan commented Feb 1, 2023

andrewserong commented Feb 1, 2023 • edited Loading

carolinan commented Feb 1, 2023

andrewserong commented Feb 1, 2023

carolinan commented Feb 1, 2023

carolinan commented Feb 1, 2023

carolinan commented Feb 1, 2023

andrewserong commented Feb 2, 2023

carolinan commented Feb 2, 2023 • edited Loading

ndiego commented Feb 7, 2023

carolinan commented Feb 7, 2023

carolinan commented Feb 8, 2023

ndiego commented Jun 12, 2023

carolinan commented Jun 13, 2023

ndiego commented Jul 3, 2023

carolinan commented Jul 3, 2023 • edited Loading

tellthemachines commented Jul 4, 2023

ramonjd commented Jul 4, 2023

ramonjd commented Jul 4, 2023

github-actions bot commented Aug 7, 2023

carolinan commented Aug 7, 2023

github-actions bot commented Aug 7, 2023

carolinan commented Sep 12, 2023 • edited Loading

tellthemachines commented Nov 22, 2023

carolinan commented Jan 26, 2023 •

edited

Loading

carolinan commented Feb 1, 2023 •

edited

Loading

carolinan commented Feb 1, 2023 •

edited

Loading

andrewserong commented Feb 1, 2023 •

edited

Loading

carolinan commented Feb 2, 2023 •

edited

Loading

carolinan commented Jul 3, 2023 •

edited

Loading

carolinan commented Sep 12, 2023 •

edited

Loading