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

Fix duotone filters in classic themes #4839

Closed
wants to merge 2 commits into from

Conversation

ajlende
Copy link

@ajlende ajlende commented Jul 12, 2023

The style engine runs before blocks get rendered in classic themes, and duotone queues up styles while blocks are rendered, so the styles never get output on the page.

I've fixed it in this patch by generating the duotone styles again in wp_footer for classic themes.

Trac ticket: https://core.trac.wordpress.org/ticket/58734


This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

Copy link

@jeryj jeryj left a comment

Choose a reason for hiding this comment

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

I tested this on Twenty Twenty with the reproduction steps from the trac ticket and this is working well 👍🏻

Comment on lines 1185 to 1190
if ( ! empty( self::$used_global_styles_presets ) ) {
wp_add_inline_style( 'core-block-supports', self::get_global_styles_presets( self::$used_global_styles_presets ) );
}
if ( ! empty( self::$block_css_declarations ) ) {
wp_add_inline_style( 'core-block-supports', wp_style_engine_get_stylesheet_from_css_rules( self::$block_css_declarations ) );
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if ( ! empty( self::$used_global_styles_presets ) ) {
wp_add_inline_style( 'core-block-supports', self::get_global_styles_presets( self::$used_global_styles_presets ) );
}
if ( ! empty( self::$block_css_declarations ) ) {
wp_add_inline_style( 'core-block-supports', wp_style_engine_get_stylesheet_from_css_rules( self::$block_css_declarations ) );
}
$handle = 'core-block-supports';
wp_register_style( $handle, false );
if ( ! empty( self::$used_global_styles_presets ) ) {
wp_add_inline_style( $handle, self::get_global_styles_presets( self::$used_global_styles_presets ) );
}
if ( ! empty( self::$block_css_declarations ) ) {
wp_add_inline_style( $handle, wp_style_engine_get_stylesheet_from_css_rules( self::$block_css_declarations ) );
}
wp_enqueue_style( $handle );

You can't add inline styles without registering and enqueuing style.

This fixes the issue in my testing.

Copy link
Author

@ajlende ajlende Jul 12, 2023

Choose a reason for hiding this comment

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

I thought it was being registered already in script-loader, but it only looks like that happens when wp_style_engine_get_stylesheet_from_context returns something when SCRIPT_DEBUG is false.

$core_styles_keys = array( 'block-supports' );
$compiled_core_stylesheet = '';
$style_tag_id = 'core';
// Adds comment if code is prettified to identify core styles sections in debugging.
$should_prettify = isset( $options['prettify'] ) ? true === $options['prettify'] : defined( 'SCRIPT_DEBUG' ) && SCRIPT_DEBUG;
foreach ( $core_styles_keys as $style_key ) {
if ( $should_prettify ) {
$compiled_core_stylesheet .= "/**\n * Core styles: $style_key\n */\n";
}
// Chains core store ids to signify what the styles contain.
$style_tag_id .= '-' . $style_key;
$compiled_core_stylesheet .= wp_style_engine_get_stylesheet_from_context( $style_key, $options );
}
// Combines Core styles.
if ( ! empty( $compiled_core_stylesheet ) ) {
wp_register_style( $style_tag_id, false );
wp_add_inline_style( $style_tag_id, $compiled_core_stylesheet );
wp_enqueue_style( $style_tag_id );
}

Would it be okay to always register core-block-supports?

Copy link
Member

Choose a reason for hiding this comment

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

@ajlende Didn't know about that. You will need to enqueue it as well. Like this

         wp_register_style( $style_tag_id, false );
	// Combines Core styles.
	if ( ! empty( $compiled_core_stylesheet ) ) {
		wp_add_inline_style( $style_tag_id, $compiled_core_stylesheet );
	}
	wp_enqueue_style( $style_tag_id );

Copy link
Author

@ajlende ajlende Jul 12, 2023

Choose a reason for hiding this comment

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

I just registered and enqueued a new stylesheet. The 'core-block-supports' name is semi-dynamic, so we'll run into issues in the future if someone tries to add to $core_styles_keys and doesn't update duotone.

It's unfortunate that the duotone block supports styles won't be grouped with the rest of block supports, but it's probably not that big of a deal.

Copy link
Member

@spacedmonkey spacedmonkey left a comment

Choose a reason for hiding this comment

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

This PR doesn't fix the issue in my testing. I have added a suggested solution in the comments.

Copy link
Member

@spacedmonkey spacedmonkey left a comment

Choose a reason for hiding this comment

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

Works in my testing.

Copy link
Contributor

@tellthemachines tellthemachines left a comment

Choose a reason for hiding this comment

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

Thanks, this fixes the issue for me!

Does this need to be fixed in Gutenberg too?

@ajlende
Copy link
Author

ajlende commented Jul 13, 2023

Does this need to be fixed in Gutenberg too?

Yeah, we'll want to port this change and #4831 back into the plugin once they're merged here to keep things in sync.

@tellthemachines
Copy link
Contributor

Committed in r56225 / 23a1516.

@t-hamano
Copy link

This commit doesn't seem to have been backported to Gutenberg, so I've submitted the PR below.

WordPress/gutenberg#54778

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

5 participants