-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Global Styles: Fix for third-party blocks #3529
Global Styles: Fix for third-party blocks #3529
Conversation
I can confirm this issue using my custom Icon Block. Reproducing the issue To reproduce, set a custom border style in Global Styles. Add an icon in the Editor and the border will appear correct. However, on the Frontend, the global style is not applied. After the fix is applied With this fix, everything appears correctly on the frontend. |
FWIW, I can also repro the issue and the fix with @ndiego's Icon block 😊 (using a 10px red border with 20px rounded edges). On the frontend:
|
Noting here that the Gutenberg fix has been part of Gutenberg 14.1 and all subsequent GB releases: |
I can reproduce it also and check that is a regression with a really simple block plugin. |
To recap: What/who is impacted? This issue affects third-party blocks. Does this meet the criteria of being fixed this late into the cycle?
|
Could we please add some unit tests here? Especially this late in the cycle, it would be great to include these. |
Yeah, makes sense! I’d have to look a bit where this would best fit in; I guess we probably have some unit test coverage for Global Styles already 🤔 I guess the test should be for |
Looks like there's no test coverage for So I guess we need to add a new |
I haven't figured out yet how to write a unit test for this; specifically, how to programmatically set some Global Styles for a block. Pinging @oandregal @scruffian @ramonjd @andrewserong in case you're able to help with this 🙏 😊 On the assertions side, if I read the code correctly, we should ideally cover two different scenarios -- (For non- |
Apologies in advanced if this is not anywhere near what you asked, but here's what I know without thinking too hard: If you want to set styles for blocks in theme.json, there's the option of creating a test theme and styling your blocks in theme.json. Here's an example: https://github.com/WordPress/gutenberg/blob/963d836b290ce5ec002de2027a8011d492c20420/phpunit/block-supports/typography-test.php#L39 If it's user-defined global styles (those added in the site editor) we want to test, then maybe we could hook into wp_theme_json_data_user to update the underlying data and styles in a unit test? In the function To get user data for the merge, That is all theoretical, I haven't tried it yet. If I get time today I will, but the universe has conspired against me so leaving these notes here in case they're helpful. |
Thanks for the ping! Unfortunately I can't think of much else add other than what Ramon mentioned, where sometimes adding a test theme to Although, maybe there's an option to filter the theme data via the I'll be AFK for the rest of the week so unfortunately can't dig any further right now, but happy to help test / review on Monday if this is still being worked on. |
👋 I've taken a look and this is the gist we need to do for a test:
{
"styles" : {
"blocks" :{
"my/third-party-block": {
"color": {
"background": "hotpink"
}
}
}
}
function test_third_party_styles( ) {
$name = 'my/third-party-block';
$settings = array(
'icon' => 'text',
'category' => 'common',
'render_callback' => 'foo',
);
register_block_type( $name, $settings ); // Register the block, so the theme.json styles are not ignored.
wp_add_global_styles_for_blocks();
unregister_block_type( $name ); // Unregister it, so it does not affect other tests.
// TEST STYLES HAVE BEEN INLINED: HOW DO WE TEST THIS?
} I'm looking at completing this (how to test inline styles?) but sharing in the interest of velocity, in case anyone else knows off the top of their head how to test the remaining things. |
Not sure either. Could we assert against registered styles?
|
Added a single test 5521274 We can iterate from here. |
@@ -69,6 +69,11 @@ | |||
"filter": { | |||
"duotone": "var(--wp--preset--duotone--custom-duotone)" | |||
} | |||
}, | |||
"my/third-party-block": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These styles will be ignored by any other tests because the block my/third-party-block
won't be registered.
wp_enqueue_global_styles(); | ||
unregister_block_type( $name ); | ||
|
||
$this->assertStringContainsString( '.wp-block-my-third-party-block{background-color: hotpink;}', get_echo( 'wp_print_styles' ), '3rd party block styles are included' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The styles added to theme.json
for the my/third-party-block
generate this style rule: .wp-block-my-third-party-block{background-color: hotpink;}
.
'render_callback' => 'foo', | ||
); | ||
register_block_type( $name, $settings ); | ||
wp_enqueue_global_styles(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are interested in testing wp_add_global_styles_for_blocks
. Though, because of how it's designed, I wasn't lucky using it directly. Hence, I had to resort to call this function (the one that orchestrates all the styles).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it'd be great if we could isolate the test to really just run wp_add_global_styles_for_blocks
🤔
Hmm, looks like the test is passing on |
I've executed the test without the fix ( |
Pushed a change 1099bce that makes the test fail on
|
Great, thank you very much!
Yeah, I think that matches this logic here: wordpress-develop/src/wp-includes/global-styles-and-settings.php Lines 216 to 219 in b71a7bf
|
Yeah, the issue only happens in
Following this steps, the border is not rendered in the front end. Then, I did the following:
<?php
add_filter( 'should_load_separate_core_block_assets', '__return_false' ); And the border is properly rendered. So, yeah, the test is covering the issue we had. |
Update, I'm working to isolate the test to |
So I've replaced the call to diff --git a/tests/phpunit/tests/theme/wpGetGlobalStylesForBlocks.php b/tests/phpunit/tests/theme/wpGetGlobalStylesForBlocks.php
index 627c4fc82d..350f6a0e06 100644
--- a/tests/phpunit/tests/theme/wpGetGlobalStylesForBlocks.php
+++ b/tests/phpunit/tests/theme/wpGetGlobalStylesForBlocks.php
@@ -83,7 +83,7 @@ class Tests_Theme_wpGetGlobalStylesForBlocks extends WP_UnitTestCase {
'render_callback' => 'foo',
);
register_block_type( $name, $settings );
- wp_enqueue_global_styles();
+ wp_add_global_styles_for_blocks();
unregister_block_type( $name );
$this->assertStringContainsString( '.wp-block-my-third-party-block{background-color: hotpink;}', get_echo( 'wp_print_styles' ), '3rd party block styles are included' ); I've then debugged what Turns out that down in the call stack, this function is called : wordpress-develop/src/wp-includes/class-wp-dependencies.php Lines 288 to 294 in b71a7bf
with the following args:
Turns out that I'll try to find out why Edit: I had mixed up |
If I'm not mistaken, the only way to register a handle with Edit: I had mixed up |
This looks promising:
|
Ah, or not. 😕 I think I need to step away from this for a moment. Upon re-running the test, I don't even see |
Oh, this actually does register The test still isn't passing with this, though 😕 diff --git a/tests/phpunit/tests/theme/wpAddGlobalStylesForBlocks.php b/tests/phpunit/tests/theme/wpAddGlobalStylesForBlocks.php
index c3286f813e..92d8dfcddf 100644
--- a/tests/phpunit/tests/theme/wpAddGlobalStylesForBlocks.php
+++ b/tests/phpunit/tests/theme/wpAddGlobalStylesForBlocks.php
@@ -83,7 +83,8 @@ class Tests_Theme_wpAddGlobalStylesForBlocks extends WP_UnitTestCase {
'render_callback' => 'foo',
);
register_block_type( $name, $settings );
- wp_enqueue_global_styles();
+ wp_register_style( 'global-styles', false, array(), true, true );
+ wp_add_global_styles_for_blocks();
unregister_block_type( $name );
$this->assertStringContainsString( '.wp-block-my-third-party-block{background-color: hotpink;}', get_echo( 'wp_print_styles' ), '3rd party block styles are included' );
|
@ockham I tweaked your example: wp_register_style( 'global-styles', false, array(), true, true );
$actual = wp_styles()->get_data( 'global-styles', 'after' );
$actual = is_array( $actual ) ? $actual : array();
$this->assertNotContains(
'.wp-block-my-third-party-block{background-color: hotpink;}',
$actual,
'Third party block inline style should not be registered before running wp_add_global_styles_for_blocks()'
);
wp_add_global_styles_for_blocks();
$actual = wp_styles()->get_data( 'global-styles', 'after' );
$this->assertSame(
'.wp-block-my-third-party-block{background-color: hotpink;}',
$actual[0],
'Third party block inline style should be registered after running wp_add_global_styles_for_blocks()'
); This fails before the fix and passes after the fix. I'll do a little cleanup and push changes shortly. |
Ah, I guess it’s the argument to wordpress-develop/src/wp-includes/class-wp-dependencies.php Lines 121 to 126 in b71a7bf
|
@@ -0,0 +1,66 @@ | |||
<?php | |||
|
|||
abstract class WP_Theme_UnitTestCase extends WP_UnitTestCase { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why add this base test case? Test classes in this group are repeating the same code. This base abstract test case encapsulates the repeating code, removing it from the test class itself. It's the starting point for improving the other tests in this same group.
Why in this PR? This "repeating set up and tear down code" is copied from the other tests in the same group. This means it's not part of the code review for the scope of this PR. By moving this copied logic to a base test case, it's signaling to focus the review on the new test class, not this copied setup/teardown code.
Note: A follow-up PR will address the other tests in this group.
19f6271 improves the tests to:
Running locally:
These 2 states are the expected behavior ✅ |
/** | ||
* @ticket 56915 | ||
*/ | ||
public function test_third_party_blocks_inline_styles_not_register_to_global_styles() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be
public function test_third_party_blocks_inline_styles_not_register_to_global_styles() { | |
public function test_third_party_blocks_inline_styles_not_registered_to_global_styles() { |
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests are looking great 👍 Thank you very much for the coverage @hellofromtonya
Confirmed! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
More tests should be added to the new Tests_Theme_WpAddGlobalStylesForBlocks
test class for all of the paths through the function. However, those tests are out-of-scope for this bug report.
37b6f74
to
25eaf26
Compare
Whoopsie, accidentally pushed my revert of the patch when testing the tests fail without the patch 🤦♀️ Reverted that temporary code and force pushed test only changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Tests passing locally for me as well. 👍
Thanks everyone! Merged into Core in https://core.trac.wordpress.org/changeset/54703. |
Merged into Core's 6.1 branch in https://core.trac.wordpress.org/changeset/54705. |
First reported in WordPress/gutenberg#40808.
Quoting that issue's instructions to reproduce the issue:
The Gutenberg PR that fixed this was flagged for inclusion in WP 6.1 a month ago, but we missed that these are PHP changes, which thus require a manual backport 😞 This was recently brought up by @oandregal to us.
First impressions indicate that this is a regression that was introduced during the 6.1 cycle. I'll try to ascertain.
Trac ticket: https://core.trac.wordpress.org/ticket/56915
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.