Make WordPress Core

Opened 19 months ago

Closed 19 months ago

Last modified 19 months ago

#56915 closed defect (bug) (fixed)

Global Styles: Not working for third-party blocks

Reported by: bernhard-reiter's profile Bernhard Reiter Owned by: davidbaumwald's profile davidbaumwald
Milestone: 6.1 Priority: high
Severity: major Version: 6.1
Component: Editor Keywords: has-patch has-unit-tests has-testing-instructions commit dev-reviewed
Focuses: css Cc:

Description

First reported in https://github.com/WordPress/gutenberg/issues/40808.

Quoting that issue's instructions to reproduce the issue:

  • Install WooCommerce.
  • Open the FSE Editor.
  • Add a WC Blocks (e.g: Feature Product Block).
  • Customize the global styles related to the block.
  • Save.
  • Notice that the Global Styles is not saved.

Note that 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.

Change History (43)

This ticket was mentioned in PR #3529 on WordPress/wordpress-develop by @Bernhard Reiter.


19 months ago
#1

  • Keywords has-patch added

Fixes https://github.com/WordPress/gutenberg/issues/40808.

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.

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

This ticket was mentioned in Slack in #core-editor by bernhardreiter. View the logs.


19 months ago

@ndiego commented on PR #3529:


19 months ago
#3

I can confirm this issue using my custom Icon Block.

Reproducing the issue

To reproduce, set a custom border style in Global Styles.
https://i0.wp.com/user-images.githubusercontent.com/4832319/198041195-c938910d-a11a-4d4c-a3c2-8f96d53b91ec.png

Add an icon in the Editor and the border will appear correct.
https://i0.wp.com/user-images.githubusercontent.com/4832319/198041374-0512ea54-4bac-41d9-9cef-d588cf4d1b7a.png

However, on the Frontend, the global style is not applied.
https://i0.wp.com/user-images.githubusercontent.com/4832319/198041469-9b3bb9b0-06fc-46d2-9720-04a58e218cdc.png

After the fix is applied

With this fix, everything appears correctly on the frontend.
https://i0.wp.com/user-images.githubusercontent.com/4832319/198042107-aa0e6589-92b7-4700-9893-c6a5f63e6c00.png

gigitux commented on PR #3529:


19 months ago
#4

I can confirm that this is a regression.

| Global Styles - Editor Side | Global Styles - Frontend Side - WP 6.1 | Global Styles - Frontend Side - WP 6.0 |

|

|https://i0.wp.com/user-images.githubusercontent.com/4463174/198041862-e6ddcc60-7136-4100-ba01-7da9db83563e.png|https://i0.wp.com/user-images.githubusercontent.com/4463174/198042829-abbe9dd4-92e8-49c6-b0f7-869dd98125c4.png|https://i0.wp.com/user-images.githubusercontent.com/4463174/198042047-f665c10f-4685-4dbf-842a-b321a0ce2430.png|

Reproduce issue

  1. Install WooCommerce. Import products from the CSV that is attached at the bottom.
  2. Open the FSE Editor.
  3. Edit the Product Catalog template.
  4. Add a WC Blocks (e.g.: Product Categories List).
  5. Customize the global styles related to the block.
  6. Save.
  7. Go to on /shop page. See that the customizations aren't applied (on WP 6.1)

sample_products.csv

@Bernhard Reiter commented on PR #3529:


19 months ago
#5

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:

| trunk | This branch |

|

| https://i0.wp.com/user-images.githubusercontent.com/96308/198045472-41f3fb38-7c88-4b3c-9c16-ff83c8b7dab0.png| https://i0.wp.com/user-images.githubusercontent.com/96308/198045647-b16b1ee1-472e-45d1-9c7e-4c3b53f27d7d.png |

@Bernhard Reiter commented on PR #3529:


19 months ago
#7

Verifying that styling did work in 6.0.3 for the Icon block ✅

https://i0.wp.com/user-images.githubusercontent.com/96308/198051312-777e20fb-0acb-4b7b-aaa9-c7a877de90ed.png

@cbravobernal commented on PR #3529:


19 months ago
#8

I can reproduce it also and check that is a regression with a really simple block plugin.
https://www.loom.com/share/72856d7db45b40ce9f12e225ea0f8f05

@Bernhard Reiter commented on PR #3529:


19 months ago
#9

To recap:

@desrosj commented on PR #3529:


19 months ago
#10

Could we please add some unit tests here? Especially this late in the cycle, it would be great to include these.

@Bernhard Reiter commented on PR #3529:


19 months ago
#11

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 wp_add_global_styles_for_blocks since that’s what we’re modifying here. And we'd need to check the output of wp_add_inline_style for the presence of the relevant styling 🤔

@Bernhard Reiter commented on PR #3529:


19 months ago
#12

Looks like there's no test coverage for wp_add_global_styles_for_blocks yet. However, there's e.g. `wp_get_global_stylesheet` in the same file, which has coverage in a dedicated `tests/phpunit/tests/theme/wpGetGlobalStylesheet.php` file.

So I guess we need to add a new tests/phpunit/tests/theme/wpGetGlobalStylesForBlocks.php file.

#13 @davidbaumwald
19 months ago

  • Focuses css added
  • Milestone changed from Awaiting Review to 6.1
  • Priority changed from normal to high
  • Severity changed from normal to major

Moving into the milestone for visibility.

@Bernhard Reiter commented on PR #3529:


19 months ago
#14

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 -- core/ and non-core/ blocks (see). In either case, they should check if wp_add_inline_style's output contains the expected styling.

(For non-core/ blocks, the assertion should fail without the fix in 1885efc00b11f7c374658a54f50d6a590021c90a.)

@ramonopoly commented on PR #3529:


19 months ago
#15

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.

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 wp_add_global_styles_for_blocks() we're calling $tree = WP_Theme_JSON_Resolver::get_merged_data();, which gets the merged theme.json data, including styles, and including user customisations.

To get user data for the merge, get_user_data() is called. That's where wp_theme_json_data_user lives.

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.

@andrewserong commented on PR #3529:


19 months ago
#16

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 themedir1 can be a good way to handle things that call global functions that gather theme data.

Although, maybe there's an option to filter the theme data via the wp_theme_json_data_blocks filter within a test (passing in some placeholder blocks in the structure there to match what needs to be tested), now that it exists? Then after calling wp_add_global_styles_for_blocks() look up the global $wp_styles to see if the inline style has been added as expected?

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.

@oandregal commented on PR #3529:


19 months ago
#17

:wave: I've taken a look and this is the gist we need to do for a test:

  1. Add styles for a 3rd party block in any test theme. We can reuse one of the existing themes. Because the block won't be registered for the other tests, it'll be ignored. Something like this in the theme.json of any test theme:

{{{json
{

"styles" : {

"blocks" :{

"my/third-party-block": {

"color": {

"background": "hotpink"

}

}

}

}
}}}

  1. Write a test that retrieves those styles and previously registers the block called my/third-party-block. Something along these lines:

{{{php
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?

}
}}}

@ramonopoly commented on PR #3529:


19 months ago
#18

how to test inline styles?

Not sure either.

Could we assert against registered styles?

wp_styles()->registered['some-inline-styles']->extra['after'],

@Bernhard Reiter commented on PR #3529:


19 months ago
#20

Hmm, looks like the test is passing on trunk 🤔 (I've cherry-picked 55212745fc4f074097553ab63a384d770ad9c91c to trunk locally and ran npm run test:php -- --group=themes.)

@oandregal commented on PR #3529:


19 months ago
#21

I've executed the test without the fix (npm run test:php -- --filter Tests_Theme_wpGetGlobalStylesForBlocks) and it's passing. I'm trying to understand why.

@oandregal commented on PR #3529:


19 months ago
#22

Pushed a change https://github.com/WordPress/wordpress-develop/pull/3529/commits/1099bce4b1ee13395181a78f67acf5e47c9495e0 that makes the test fail on trunk. According to the how the test passed without that, I understand this is the issue without this patch:

  • when block styles are loaded as part of the global-styles-inline-css stylesheet, 3rd party blocks styles worked fine. How to test: use a theme that disables separate block assets via add_filter( 'should_load_separate_core_block_assets', '__return_false' );.
  • when block styles are inlined to the own block stylesheet, this didn't work. My understanding is that block themes load assets separately, by default. This condition can be forced via add_filter( 'should_load_separate_core_block_assets', '__return_false' ).

@Bernhard Reiter commented on PR #3529:


19 months ago
#23

Pushed a change 1099bce that makes the test fail on trunk.

Great, thank you very much!

According to the how the test passed without that, I understand this is the issue without this patch:

  • when block styles are loaded as part of the global-styles-inline-css stylesheet, 3rd party blocks styles worked fine. How to test: use a theme that disables separate block assets via add_filter( 'should_load_separate_core_block_assets', '__return_false' );.
  • when block styles are inlined to the own block stylesheet, this didn't work. My understanding is that block themes load assets separately, by default. This condition can be forced via add_filter( 'should_load_separate_core_block_assets', '__return_false' ).

Yeah, I think that matches this logic here: https://github.com/WordPress/wordpress-develop/blob/b71a7bfcfff6532d6f83b38dbe52bcb24208d37f/src/wp-includes/global-styles-and-settings.php#L216-L219

@oandregal commented on PR #3529:


19 months ago
#24

I understand this is the issue without this patch:

Yeah, the issue only happens in trunk when the styles are loaded per block. Tested by:

  • using TwentyTwentyThree
  • added an icon block
  • go to the global styles sidebar and add a border

Using trunk and the existing theme, the border is not rendered in the front end.

Then, I did the following:

  • create a functions.php for the theme
  • add this:

{{{php
<?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.

@Bernhard Reiter commented on PR #3529:


19 months ago
#25

Update, I'm working to isolate the test to wp_add_global_styles_for_blocks (see).

@Bernhard Reiter commented on PR #3529:


19 months ago
#26

So I've replaced the call to wp_enqueue_global_styles with wp_add_global_styles_for_blocks:

{{{diff
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 `wp_add_inline_style( $handle, $data )` is doing. It's basically an alias for wp_styles()->add_inline_style( $handle, $data ). The wp_styles() function gives access to the global wp_styles object, which is of course of class WP_Styles. That class in turn is derived from WP_Dependencies.

Turns out that down in the call stack, this function is called : https://github.com/WordPress/wordpress-develop/blob/b71a7bfcfff6532d6f83b38dbe52bcb24208d37f/src/wp-includes/class-wp-dependencies.php#L288-L294

with the following args:

$handle: global-styles
$key: after
$value: Array
(
    [0] => .wp-block-my-third-party-block{background-color: hotpink;}
)

Turns out that ! isset( $this->registered[ $handle ] ) returns false for `$handle === 'after'.

I'll try to find out why after is not registered (and what the heck that even means).

@Bernhard Reiter commented on PR #3529:


19 months ago
#27

I'll try to find out why 'after' is not registered (and what the heck that even means).

If I'm not mistaken, the only way to register a handle with WP_Dependencies is via its `add()` method (rather than add_data()). So I'm thinking some other code must be doing that somewhere in the callstack of wp_enqueue_global_styles, which would explain why the test works for that, but not with explicitly calling wp_add_global_styles_for_blocks.

@Bernhard Reiter commented on PR #3529:


19 months ago
#29

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 global_styles not being registered anymore. Maybe I was looking at the wrong thing 🙄

@Bernhard Reiter commented on PR #3529:


19 months ago
#30

Oh, this actually does register global_styles in time for the add_data call 🤔

The test still isn't passing with this, though 😕

{{{diff
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' );

}}}

@hellofromTonya commented on PR #3529:


19 months ago
#31

@ockham I tweaked your example:

{{{php

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.

@Bernhard Reiter commented on PR #3529:


19 months ago
#32

Ah, I guess it’s the argument to wp_styles()->get_data() that’s key! If I’m reading WP_Dependencies::do_items() (called from wp_print_styles), WP_Dependencies::all_deps(), and friends correctly, the class is a bit picky if no $handles arg is passed to those methods. Seems like without that arg, do_items only prints styling for handles that were previously enqueued: https://github.com/WordPress/wordpress-develop/blob/b71a7bfcfff6532d6f83b38dbe52bcb24208d37f/src/wp-includes/class-wp-dependencies.php#L121-L126

@hellofromTonya commented on PR #3529:


19 months ago
#33

https://github.com/WordPress/wordpress-develop/pull/3529/commits/19f62712f1bc0287127560cc119c7ff025bede46 improves the tests to focus on the scope of this PR and isolated the integration tests to wp_add_global_styles_for_blocks() function.

Running locally:

  • Removing the bugfix, tests fail ✅
  • Adding the bugfix, tests pass ✅

These 2 states are the expected behavior ✅

@Bernhard Reiter commented on PR #3529:


19 months ago
#34

Running locally:

  • Removing the bugfix, tests fail ✅
  • Adding the bugfix, tests pass ✅

These 2 states are the expected behavior ✅

Confirmed!

@hellofromTonya commented on PR #3529:


19 months ago
#35

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.

#36 @hellofromTonya
19 months ago

  • Keywords has-unit-tests has-testing-instructions commit added
  • Integration tests added to the PR ✅
    • Expected tests fail without the bugfix patch ✅
    • Tests pass with the bugfix patch ✅
  • PR code LGTM 👍

Given that this bugfix was already released in Gutenberg, there are multiple manual test reports, integration tests are working as expected, and the PR's code is approved, marking for commit to trunk.

#37 @davidbaumwald
19 months ago

  • Owner set to davidbaumwald
  • Resolution set to fixed
  • Status changed from new to closed

In 54703:

Editor: Ensure global styles are rendered for third-party blocks.

This change ensures custom styles for all third-party blocks are rendered on the front end if assets are set to be loaded on a per-block basis. Additionally, this change includes new unit tests to help prevent a similar bug in the future.

Props scruffian, aristath, poena, wildworks, ajlende, andraganescu, ndiego, gigitux, cbravobernal, ramonopoly, andrewserong, oandregal, hellofromTonya, bernhard-reiter.
Fixes #56915.

#38 @davidbaumwald
19 months ago

  • Keywords dev-feedback added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for backport to the 6.1 branch pending a second committer's review.

@davidbaumwald commented on PR #3529:


19 months ago
#39

Thanks everyone! Merged into Core in https://core.trac.wordpress.org/changeset/54703.

#40 @hellofromTonya
19 months ago

  • Keywords dev-reviewed added; dev-feedback removed

[54703] is ready for backport to 6.1 branch.

#41 @Bernhard Reiter
19 months ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 54705:

Editor: Ensure global styles are rendered for third-party blocks.

This change ensures custom styles for all third-party blocks are rendered on the front end if assets are set to be loaded on a per-block basis. Additionally, this change includes new unit tests to help prevent a similar bug in the future.

Props scruffian, aristath, poena, wildworks, ajlende, andraganescu, ndiego, gigitux, cbravobernal, ramonopoly, andrewserong, oandregal, hellofromTonya, davidbaumwald.
Merges [54703] to the 6.1 branch.
Fixes #56915.

This ticket was mentioned in Slack in #core by bernhardreiter. View the logs.


19 months ago

Note: See TracTickets for help on using tickets.