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

Update custom CSS handling to be consistent with block global styles. #62357

Merged
merged 5 commits into from
Jun 13, 2024

Conversation

tellthemachines
Copy link
Contributor

@tellthemachines tellthemachines commented Jun 6, 2024

What?

Addresses several concerns on the PR that added conditional loading of block custom CSS.

  • Custom CSS for blocks should be loaded only when the block is present on the page when wp_should_load_separate_core_block_assets is true;
  • Global styles custom CSS should be handled by the same logic as other global styles;
  • Any custom CSS coming from the Customizer should be loaded before the global styles custom CSS, as has been the case since Move custom css to its own inline style generation method and combine with customizer CSS #47396.
  • Functions previously used for custom CSS handling and no longer needed have been deprecated.
    • Deprecation notice for functions in core is tagged with WP 6.7
    • For Gutenberg-only functions it's tagged with next plugin version.
  • Because base and block-specific custom CSS are now generated in different places, I replaced the test for the now-deprecated get_custom_css function with two different tests.

Testing Instructions

  1. Test with custom CSS added at base level and at block level, in theme.json or through the global styles UI
  2. Check that custom CSS for blocks is loaded only when the block is present on the page when wp_should_load_separate_core_block_assets is true (which it is by default in most block themes);
  3. Add add_filter( 'should_load_separate_core_block_assets', '__return_false', 11 ); to the theme's functions.php and check that custom CSS for all blocks always loads in the front end.

Testing Instructions for Keyboard

Screenshots or screencast

@tellthemachines tellthemachines added [Type] Enhancement A suggestion for improvement. Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json labels Jun 6, 2024
@tellthemachines tellthemachines self-assigned this Jun 6, 2024
Copy link

github-actions bot commented Jun 6, 2024

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-editor-settings.php
❔ lib/class-wp-theme-json-gutenberg.php
❔ lib/global-styles-and-settings.php
❔ lib/script-loader.php
❔ phpunit/class-wp-theme-json-test.php

@tellthemachines tellthemachines marked this pull request as ready for review June 7, 2024 02:03
Copy link

github-actions bot commented Jun 7, 2024

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: ramonjd <[email protected]>

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

@@ -2855,6 +2866,11 @@ static function ( $pseudo_selector ) use ( $selector ) {
$block_rules .= static::to_ruleset( ":root :where($style_variation_selector)", $individual_style_variation_declarations );
}

// 7. Generate and append any custom CSS rules.
if ( isset( $node['css'] ) ) {
$block_rules .= $this->process_blocks_custom_css( $node['css'], $selector );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this line allows adding custom CSS declarations to the root selector in theme.json

	"styles": {
		"css": "background-color:purple;"
	},

I can see the background color on the frontend but not in the site editor

Whereas in trunk only this works:

	"styles": {
		"css": "body{background-color:purple;}"
	},

Should Gutenberg allow both? Or is it a regression?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm that's a good question. This is how it was before #47396, but given that PR is over a year old it might be best to keep things as they were more recently. I'd expect to have to write complete rules in the top level custom CSS control and not necessarily for loose properties to be attached to the body element.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oooooh this is interesting; I think we've detected a bug in the existing behaviour. Testing in trunk, the loose properties are output outside any rule, so they don't apply because they're invalid, both in editor and front end:

Screenshot 2024-06-11 at 3 43 26 PM Screenshot 2024-06-11 at 3 47 01 PM

Whereas in this branch, those properties are nested under the root selector in the front end, though they're still invalid output in the editor. I think ideally we should fix things so they aren't output at all. I'll look into it!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I fixed things so that this branch behaves like trunk (no valid CSS output if only properties are added to the top level custom CSS control) but the loose properties are still being output (just like they are on trunk).

The problem with this is that any rule output straight after a loose property won't work. I can repro this in core trunk, and it's also a problem with the customizer additional CSS (which may not manifest in classic themes because additional CSS is enqueued after everything else, but on hybrid themes we're adding it before global styles custom CSS, so it can also break stuff)

We could maybe just check that there aren't any loose properties at the end of the custom CSS string? Checking the whole string is valid CSS might be overkill as site admins should be responsible for the validity of their CSS. We could also Do Nothing 😄 and leave things as they are.

If we do want to do anything though, it's probably better as a separate PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we do want to do anything though, it's probably better as a separate PR.

For sure. Thanks for looking into it.

We could maybe just check that there aren't any loose properties at the end of the custom CSS string?

Or maybe some light-weight parsing where the styles are added?

$stylesheet .= _wp_array_get( $this->theme_json, array( 'styles', 'css' ) );

@ramonjd
Copy link
Member

ramonjd commented Jun 11, 2024

Other than the question above, this PR tests well for me.

I tested in site editor and via theme.json 👍🏻

Copy link
Member

@ramonjd ramonjd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested again. LGTM

Styles loaded only when the block is present on the page 👍🏻

Otherwise behaves as it does on trunk.

Thanks!

@@ -1421,6 +1428,7 @@ public function get_custom_css() {
* @return string The global styles base custom CSS.
*/
public function get_base_custom_css() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So these never made it to Core, right? Should we remove the @since annotations and maybe add a "don't backport note"?

Doesn't have to be here, just wondering if it'll help later.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh good point, we should remove the @since on these!

@tellthemachines tellthemachines enabled auto-merge (squash) June 13, 2024 03:43
@tellthemachines tellthemachines merged commit 6ec0bda into trunk Jun 13, 2024
62 checks passed
@tellthemachines tellthemachines deleted the try/consolidating-custom-css branch June 13, 2024 04:09
@github-actions github-actions bot added this to the Gutenberg 18.6 milestone Jun 13, 2024
patil-vipul pushed a commit to patil-vipul/gutenberg that referenced this pull request Jun 17, 2024
@tellthemachines tellthemachines added the CSS Styling Related to editor and front end styles, CSS-specific issues. label Jul 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CSS Styling Related to editor and front end styles, CSS-specific issues. Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants