Fix: Theme.json application of custom root selector for styles

[aaronrobertshaw]

What?

Fixes assignment of custom root_selector when generating a theme.json stylesheet.

Why?

Theme.json stylesheets attempting to use a custom root selector are generated with in correct styles.

How?

Assigns the root selector to the correct node.

Testing Instructions

1. Run npm run test:unit:php:base -- --filter WP_Theme_JSON_Gutenberg_Test

[github-actions]

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/class-wp-theme-json-gutenberg.php
❔ phpunit/class-wp-theme-json-test.php

[aaronrobertshaw]

The PHP changes in this PR will need syncing to core. There are a few larger PRs in the works that may rely on this fix so there's a possibility this might be best backported, batched with the changes of one of the other PRs.

[andrewserong]

Great catch! This seems to be testing well for me.

One question about the behaviour for the custom root selector — by using a different root selector as in this PR, it will skip the output of the base layout styles, as the base layout styles are only output on body here (so aren't designed / intended to work directly with a custom root selector): https://github.com/WordPress/gutenberg/blob/238aa471bc8b3677dde706e93fdf8bd5b2659d5c/lib/class-wp-theme-json-gutenberg.php#L1507

I assume that's intentional? My understanding is that the different root selector would be for section-specific styling, where the base layout rules wouldn't be changing as they're only meant to be handled in one place at the very root of the document.

The main reason I ask, is that if we revert the changes in the theme JSON class, but keep the tests here, this is what the test failure reveals (prior to the change in this PR, the base layout styles would be included):

There was 1 failure:

1) WP_Theme_JSON_Gutenberg_Test::test_get_stylesheet_custom_root_selector
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'body { margin: 0;}.wp-site-blocks > .alignleft { float: left; margin-right: 2em; }.wp-site-blocks > .alignright { float: right; margin-left: 2em; }.wp-site-blocks > .aligncenter { justify-content: center; margin-left: auto; margin-right: auto; }.custom{color: teal;}'
+'body { margin: 0;}.wp-site-blocks > .alignleft { float: left; margin-right: 2em; }.wp-site-blocks > .alignright { float: right; margin-left: 2em; }.wp-site-blocks > .aligncenter { justify-content: center; margin-left: auto; margin-right: auto; }:where(.is-layout-flex){gap: 0.5em;}:where(.is-layout-grid){gap: 0.5em;}body .is-layout-flow > .alignleft{float: left;margin-inline-start: 0;margin-inline-end: 2em;}body .is-layout-flow > .alignright{float: right;margin-inline-start: 2em;margin-inline-end: 0;}body .is-layout-flow > .aligncenter{margin-left: auto !important;margin-right: auto !important;}body .is-layout-constrained > .alignleft{float: left;margin-inline-start: 0;margin-inline-end: 2em;}body .is-layout-constrained > .alignright{float: right;margin-inline-start: 2em;margin-inline-end: 0;}body .is-layout-constrained > .aligncenter{margin-left: auto !important;margin-right: auto !important;}body .is-layout-constrained > :where(:not(.alignleft):not(.alignright):not(.alignfull)){max-width: var(--wp--style--global--content-size);margin-left: auto !important;margin-right: auto !important;}body .is-layout-constrained > .alignwide{max-width: var(--wp--style--global--wide-size);}body .is-layout-flex{display: flex;}body .is-layout-flex{flex-wrap: wrap;align-items: center;}body .is-layout-flex > *{margin: 0;}body .is-layout-grid{display: grid;}body .is-layout-grid > *{margin: 0;}body{color: teal;}'

So, that all looks good to me, assuming the intention is correct that a custom root selector will skip base layout output 👍

LGTM! ✨

[aaronrobertshaw]

Thanks for the review and questions @andrewserong 👍

I assume that's intentional?

I'm not 100% clear on the intentions of the layout styles etc but if the base layout styles are only meant for use on body, as was my understanding, the check for that makes sense to me.

My understanding is that the different root selector would be for section-specific styling

This is what drove this fix. What I didn't include in this PR was that generating stylesheets from theme.json partials also needs to get rid of the pesky root layout styles.

I was going to address this separately as I hadn't been able to decide yet on if it should be another option that can be passed to opt out of them, or if the get_style_nodes also needs updating to handle the custom root selector, or both.

It appears whenever the root selector was added, the job was only half done. Probably due to this bug obscuring these issues.

Are you still happy for this fix to land and the rest fixed in a follow-up?

I couldn't see any uses of the custom root selector for styles (another reason this has gone unnoticed). The only use I did find was for the settings block support and generating preset styles which work off the settings node rather than the styles.

For that reason, if the root layout styles are still included and base layout styles omitted, I don't think it hurts anything. Before it gets used any follow-ups can tidy it up. Given this aspect of the custom root selector is just completely broken now, it's not going to be breaking anything if it works but with extra styles.

[github-actions]

Flaky tests detected in 957a22f.
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/7620028706
📝 Reported issues:

• [Flaky Test] the event listener is removed when the element is removed #57983 in /test/e2e/specs/interactivity/directive-on-document.spec.ts

[andrewserong]

Are you still happy for this fix to land and the rest fixed in a follow-up?

Absolutely! It sounds like we're both on the same page. The base layout styles are really only meant to be output once per page load as it's the styles associated with a particular layout type (e.g. is-layout-flow, etc), and they won't change elsewhere on a page, so it's good to exclude them from output if someone's using a custom selector.

All good to tidy things up further in follow-ups IMO 👍

Thanks for thinking all this through!

[aaronrobertshaw]

Backport PR available in: WordPress/wordpress-develop#5944

[getdave]

Thank you for working on this Aaron. I reviewed the backport PR and now waiting for commit.

Once the code is merged into WP Core we'd be grateful if you could:

• check off the line items in the PHP Sync Tracking Issue
• remove the Needs PHP backport label
• add the Backported to WP Core label

Thank you in advance 🙇