Don't output base flow and constrained layout rules on themes without theme.json #60764

tellthemachines · 2024-04-16T03:27:23Z

What?

Fixes https://core.trac.wordpress.org/ticket/60981.

If the theme has no theme.json, outputting base rules for flow and constrained layout types doesn't make sense, because there's no content/wide width defined.

This change should work even for the edge case of block themes with no theme.json, because the absence of a theme.json implies the theme is providing all the necessary styles.

Apart from the auto left and right margin rule mentioned in the trac ticket, this change also affects the alignleft and alignright rules. These rules also contain margin values that can override those defined by classic themes, so I think it makes sense to remove them too.

Testing Instructions

Activate a theme with no theme.json.
Add a Group block to a post and add some blocks inside it.
Check that the children of the Group block aren't being targeted by the body .is-layout-constrained > :where(:not(.alignleft):not(.alignright):not(.alignfull)) CSS rule that sets left and right margins to auto !important.
Add a valid theme.json file to the theme and check that those rules are correctly output.
Check that block spacing rules are still output if the theme has support for appearance-tools.

Testing Instructions for Keyboard

Screenshots or screencast

github-actions · 2024-04-16T03:27:40Z

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: tellthemachines <[email protected]>
Co-authored-by: oandregal <[email protected]>
Co-authored-by: andrewserong <[email protected]>
Co-authored-by: ramonjd <[email protected]>
Co-authored-by: creativecoder <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

oandregal · 2024-04-16T08:13:17Z

lib/class-wp-theme-json-gutenberg.php

@@ -1608,6 +1608,11 @@ protected function get_layout_styles( $block_metadata ) {
 					foreach ( $base_style_rules as $base_style_rule ) {
 						$declarations = array();

+						// Skip outputting base styles for flow and constrained layout types if theme doesn't support theme.json.
+						if ( ! wp_theme_has_theme_json() && ( 'default' === $layout_definition['name'] || 'constrained' === $layout_definition['name'] ) ) {


Can we achieve the same differently, without using wp_theme_has_theme_json here? The WP_Theme_JSON class is WordPress agnostic: doesn't use "WordPress utilities" (theme supports, doesn't know what a theme is, etc.).

I don't know the details of how the "layout styles" work, so I may be missing tons of context. However, I'd think this kind of logic (whether a theme has or doesn't a theme.json) pertains to the public function gutenberg_get_global_stylesheet that then makes the proper calls to the utilities.

The WP_Theme_JSON class is WordPress agnostic

Oooh great to know, I hadn't realised that! Was there any thought of using the class outside of WP, or is it just a separation of concerns thing?

I guess if we want the logic to reside in gutenberg_get_global_stylesheet we can pass a flag down through get_stylesheet.

Hmm, we're using current_theme_supports further up in get_layout_styles; should the same logic apply there?

separation of concerns thing?

This. It simplifies a lot of things (testing, reasoning about the code, etc.).

Hmm, we're using current_theme_supports further up in get_layout_styles; should the same logic apply there?

In my view, we shouldn't have done that there.

Just wondering how we might implement it differently. Would it be something like moving the logic to the resolver around here?

gutenberg/lib/class-wp-theme-json-resolver-gutenberg.php

Line 297 in 2477a6d

if ( ! wp_theme_has_theme_json() ) {

If so, then it looks like the current approach is for keys to be set within the $theme_support_data array, so would we have something like $theme_support_data['settings']['layout']['disableLayoutStyles'] as a supported theme.json key?

Then we might also have a key to flag skipping the base layout styles for default and constrained layouts... but how to do that without getting stuck with awkward naming, or an awkward API shape of the layout settings object 🤔

Also, this bug fix is targeting 6.5.3 so we might not want to try to change too much in one go?

Is it something that could be returned from gutenberg_get_layout_definitions()?

So check for wp_theme_has_theme_json() in gutenberg_get_layout_definitions() and just remove them before returning? Or something? 😄

Thanks for the ideas folks!

but how to do that without getting stuck with awkward naming

This is the eternal problem 😂

Would something like disableContentWidthStyles work? It's not technically a theme support, but all classic themes that don't have a theme.json in practice don't support it. We haven't needed to check for the presence of a theme.json file in any other part of this class so far, so perhaps there isn't a need for a more generic disableThemeJson type thing.

Is it something that could be returned from gutenberg_get_layout_definitions()?

Hmm that might be messier because we don't want to output base flow/constrained styles but we still want to output gap styles if the theme supports appearance-tools.

Would something like disableContentWidthStyles work?

Feels pretty consistent with disableLayoutStyles, so I don't mind it!

I just thought of another possibility: we could leverage the base-layout-styles type because that only gets set when there's no theme.json. The name might be a bit confusing because we also have baseStyles in the layout definitions, but it's an existing property so would be a small change.

Oh, clever, I like it!

So long as there's a code comment alongside the logic for it, re-using base-layout-styles sounds reasonable to me 👍

andrewserong

For what it's worth this is testing nicely for me! Using wp_theme_has_theme_json (wherever it's called from) as the guard makes sense as it parallels the logic of when the group inner container is output for Classic themes. Each of the Classic themes I tested with provide their own rules for align left/right etc, so it feels natural to me that they wouldn't depend on the rules provided by layout for flow and constrained layouts.

Edit: also, confirmed that either adding a theme.json file or add_theme_support( 'appearance-tools' ); to a Classic theme results in these rules being output again, so there's a clear path for any Classic themes that do want these rules after all 👍

tellthemachines · 2024-04-18T04:53:39Z

Here's something interesting: all the test failures resulting from this change point to us running all of WP_Theme_JSON_Gutenberg_Test on a classic theme 🤔 should that be the case?

ramonjd · 2024-04-18T05:00:18Z

Here's something interesting: all the test failures resulting from this change point to us running all of WP_Theme_JSON_Gutenberg_Test on a classic theme 🤔 should that be the case?

Oh! Shouldn't we be testing it on both classic and block themes?

tellthemachines · 2024-04-18T05:03:13Z

Oh! Shouldn't we be testing it on both classic and block themes?

I don't think we need to test everything on both; maybe run a couple tests on classic themes where expected behaviour is different. Such as the behaviour this PR introduces 😄

tellthemachines · 2024-04-18T06:24:12Z

OK I've updated the PR to use base-layout-styles instead of check for the presence of theme.json directly.

Potentially, we could use another type flag to check if theme support for layout styles is disabled, or maybe that one makes more sense in the resolver as @andrewserong suggested? In any case, given it's an enhancement, let's leave it for a follow-up.

It would also be good to follow up with ensuring the tests are mostly run on a block theme; I'll add a note to #60842.

andrewserong · 2024-04-18T06:28:24Z

In any case, given it's an enhancement, let's leave it for a follow-up.

Agreed. The shape of this PR looks good to me for a 6.5.x fix 👍
I've run out of time for today, but I'll take this for another spin tomorrow if no-one beats me to it!

creativecoder · 2024-04-18T15:15:48Z

Given that this fixes a trac ticket slated for WP 6.5.3, I've added the "Backport to WP Minor Release" and "Needs PHP Backport" labels to this PR--but please let me know if I'm missing something and that isn't correct.

andrewserong

This is testing great for me, and works as expected across all the block and classic themes I tested it with (around half a dozen of each).

Just wanted to clarify one question:

Check that block spacing rules are still output if the theme has support for appearance-tools.

In this case, the margin-left / margin-right base layout rules are still skipped, but we get the vertical spacing rules for flow and constrained layouts. Is that the expected behaviour? I assume so, but just thought I'd double-check. I only get those left/right rules if a theme.json file is added 👍

LGTM! ✨

tellthemachines · 2024-04-19T03:04:01Z

@andrewserong correct, left/right margin rules are skipped if the theme has no theme.json, regardless of appearance-tools support, and vertical spacing rules are output if the theme supports appearance-tools.

… theme.json

tellthemachines · 2024-04-19T04:15:25Z

Thanks for the reviews everyone!

tellthemachines · 2024-04-19T06:26:07Z

I've opened a sync PR in core: WordPress/wordpress-develop#6410

… theme.json (#60764) Co-authored-by: tellthemachines <[email protected]> Co-authored-by: oandregal <[email protected]> Co-authored-by: andrewserong <[email protected]> Co-authored-by: ramonjd <[email protected]> Co-authored-by: creativecoder <[email protected]>

creativecoder · 2024-05-02T16:18:45Z

I manually cherry-picked this PR to the update/fixes-for-wp-6-5-3 branch to get it included in the next release: 84ac9e3

… theme.json (#60764) Co-authored-by: tellthemachines <[email protected]> Co-authored-by: oandregal <[email protected]> Co-authored-by: andrewserong <[email protected]> Co-authored-by: ramonjd <[email protected]> Co-authored-by: creativecoder <[email protected]>

tellthemachines added [Type] Bug An existing feature does not function as intended [Feature] Layout Layout block support, its UI controls, and style output. labels Apr 16, 2024

tellthemachines requested review from oandregal, ramonjd, carolinan and andrewserong April 16, 2024 03:27

tellthemachines self-assigned this Apr 16, 2024

oandregal reviewed Apr 16, 2024

View reviewed changes

andrewserong reviewed Apr 17, 2024

View reviewed changes

creativecoder added Backport to WP Minor Release Pull request that needs to be backported to a WordPress minor release Needs PHP backport Needs PHP backport to Core labels Apr 18, 2024

andrewserong approved these changes Apr 19, 2024

View reviewed changes

tellthemachines added 3 commits April 19, 2024 13:05

Don't output base flow and constrained layout rules on themes without…

df9e794

… theme.json

fix whitespace

d5c92de

Leverage 'base-layout-styles' type to flag absence of theme.json.

483e862

tellthemachines force-pushed the fix/classic-theme-layouts branch from d61edc1 to 483e862 Compare April 19, 2024 03:19

tellthemachines merged commit cc8fce2 into trunk Apr 19, 2024
60 checks passed

tellthemachines deleted the fix/classic-theme-layouts branch April 19, 2024 04:14

github-actions bot added this to the Gutenberg 18.2 milestone Apr 19, 2024

tellthemachines mentioned this pull request Apr 19, 2024

Don't output base flow and constrained layout rules on themes without theme.json. WordPress/wordpress-develop#6410

Closed

creativecoder added Backported to WP Core Pull request that has been successfully merged into WP Core and removed Needs PHP backport Needs PHP backport to Core labels Apr 23, 2024

creativecoder mentioned this pull request May 1, 2024

Cherry-picked bug fixes for WP 6.5.3 #61299

Merged

creativecoder removed the Backport to WP Minor Release Pull request that needs to be backported to a WordPress minor release label May 2, 2024

creativecoder mentioned this pull request May 2, 2024

Editor: update npm packages with bugfixes for 6.5.3 WordPress/wordpress-develop#6487

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't output base flow and constrained layout rules on themes without theme.json #60764

Don't output base flow and constrained layout rules on themes without theme.json #60764

tellthemachines commented Apr 16, 2024

github-actions bot commented Apr 16, 2024 •

edited

Loading

oandregal Apr 16, 2024

tellthemachines Apr 17, 2024

tellthemachines Apr 17, 2024

oandregal Apr 17, 2024

andrewserong Apr 17, 2024

ramonjd Apr 18, 2024 •

edited

Loading

tellthemachines Apr 18, 2024

andrewserong Apr 18, 2024

tellthemachines Apr 18, 2024

andrewserong Apr 18, 2024 •

edited

Loading

andrewserong left a comment •

edited

Loading

tellthemachines commented Apr 18, 2024

ramonjd commented Apr 18, 2024

tellthemachines commented Apr 18, 2024

tellthemachines commented Apr 18, 2024

andrewserong commented Apr 18, 2024

creativecoder commented Apr 18, 2024

andrewserong left a comment

tellthemachines commented Apr 19, 2024

tellthemachines commented Apr 19, 2024

tellthemachines commented Apr 19, 2024

creativecoder commented May 2, 2024

Don't output base flow and constrained layout rules on themes without theme.json #60764

Don't output base flow and constrained layout rules on themes without theme.json #60764

Conversation

tellthemachines commented Apr 16, 2024

What?

Testing Instructions

Testing Instructions for Keyboard

Screenshots or screencast

github-actions bot commented Apr 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramonjd Apr 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewserong Apr 18, 2024 • edited Loading

Choose a reason for hiding this comment

andrewserong left a comment • edited Loading

Choose a reason for hiding this comment

tellthemachines commented Apr 18, 2024

ramonjd commented Apr 18, 2024

tellthemachines commented Apr 18, 2024

tellthemachines commented Apr 18, 2024

andrewserong commented Apr 18, 2024

creativecoder commented Apr 18, 2024

andrewserong left a comment

Choose a reason for hiding this comment

tellthemachines commented Apr 19, 2024

tellthemachines commented Apr 19, 2024

tellthemachines commented Apr 19, 2024

creativecoder commented May 2, 2024

github-actions bot commented Apr 16, 2024 •

edited

Loading

ramonjd Apr 18, 2024 •

edited

Loading

andrewserong Apr 18, 2024 •

edited

Loading

andrewserong left a comment •

edited

Loading