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

[WP 6.1] Block-specific global styles don't seem to save #44434

Closed
RahiDroid opened this issue Sep 24, 2022 · 29 comments
Closed

[WP 6.1] Block-specific global styles don't seem to save #44434

RahiDroid opened this issue Sep 24, 2022 · 29 comments
Labels
Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json [Priority] High Used to indicate top priority items that need quick attention [Type] Bug An existing feature does not function as intended

Comments

@RahiDroid
Copy link
Contributor

Description

  • Original trac ticket: https://core.trac.wordpress.org/ticket/56644 by @jorgefilipecosta
  • Moving here along with some added visuals to help out better understand the issue.
  • I could reproduce it with multiple blocks like paragraph, heading, button, site title, etc. It doesn't seem to be happening with all of the blocks, though.
  • The issue isn't specific to only colors. It's happening with other properties of a block too.
  • Block-specific global styles don't seem to save. Changing the global styles not under the Blocks section still works.
  • Was able to reproduce with both TT2 and TT3 themes.

Step-by-step reproduction instructions

Copy pasting from the trac ticket,

  1. Go to the site editor.
  2. Open the global styles sidebar.
  3. Open the blocks section.
  4. Navigate to the paragraph block (or button, heading, etc, I think most static blocks).
  5. Apply a background color.
  6. Save the global styles.
  7. The background color is wholly discarded on save.

Screenshots, screen recording, code snippet

wp6.1-global-styles.mov

Environment info

  • WP(6.1-beta1-54298)
  • TT2 or TT3 theme
  • PHP 7.4
  • Gutenberg plugin deactivated

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

@jorgefilipecosta jorgefilipecosta moved this from Triage to Todo in WordPress 6.1 Editor Tasks Sep 26, 2022
@ndiego ndiego added [Type] Bug An existing feature does not function as intended Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json [Priority] High Used to indicate top priority items that need quick attention labels Sep 26, 2022
@ndiego
Copy link
Member

ndiego commented Sep 26, 2022

I can confirm this issue. It's a pretty major one 😬
global-style-no-saving

@glendaviesnz
Copy link
Contributor

glendaviesnz commented Sep 27, 2022

Looks like the values are being saved to the styles custom post type, but are not being retrieved for some reason, will see if I can work out why that is.

@glendaviesnz
Copy link
Contributor

It looks like in wordpress-develop this call is not returning the custom global styles data. The correct data is there in the preceding config var, ran out of time to try and work out why it is working in GB trunk and not 6.1 beta.

@oandregal may have some thoughts - this PR touched on some of these areas, but it all seems to have been ported to 6.1

@andrewserong
Copy link
Contributor

andrewserong commented Sep 27, 2022

Interestingly, in wordpress-develop I can save styles for server rendered blocks in global styles (e.g. site title), but not static blocks (like Group). Just in case that points in any particular direction for this one or the linked trac issue.

@andrewserong
Copy link
Contributor

andrewserong commented Sep 27, 2022

I think this one will likely need to be fixed from the core side, but I noticed (and commented here on the trac issue) that if we update the priority of the register_core_block_types_from_metadata callback on the init action to have a priority of 1, that appears to resolve the issue (this line). So it sounds like it's likely related to a caching issue / order of calls with get_blocks_metadata being called too early, like @jorgefilipecosta described? (While updating the priority worked in my local environment, I imagine that won't work as a proper fix, as it possibly obfuscates the real bug 🤔)

@ndiego
Copy link
Member

ndiego commented Sep 27, 2022

I can confirm that this issue does not occur with Gutenberg active. Only in 6.1 Beta. So it seemly like something might not have been ported over properly? 🤔

@oandregal
Copy link
Member

oandregal commented Sep 27, 2022

I've done some testing: go to the global styles sidebar and set the global background color and the background color of the heading block. This is how I'm tracing this data usage:

  • the database has the actual user data
{
  "styles": {
    "color": { "background": "var:preset|color|vivid-red" },
    "blocks": {
      "core/heading": {
        "color": { "background": "var:preset|color|luminous-vivid-amber" }
      }
    }
  },
  "isGlobalStylesUserThemeJSON": true,
  "version": 2
}
  • the result of calling WP_Theme_JSON_Resolver::get_user_data() is this (notice how the block data is missing):
(
    [theme_json:protected] => Array
        (
            [styles] => Array
                (
                    [color] => Array
                        (
                            [background] => var:preset|color|vivid-red
                        )

                )

            [version] => 2
        )

)

So the data is properly saved but is discarded when rendering.

@oandregal
Copy link
Member

oandregal commented Sep 27, 2022

What happens is that, at some point, the theme.json class receives the data saved in the database. It wants to sanitize it. One of the things it does is removing data for blocks that are not registered. And, apparently, the core/heading is not, hence its data is removed and the styles are generated without it. These are the "valid blocks" at the point the user data is first calculated:

(
    [0] => core/legacy-widget
    [1] => core/widget-group
    [2] => core/archives
    [3] => core/avatar
    [4] => core/block
    [5] => core/calendar
    [6] => core/categories
    [7] => core/comment-author-name
    [8] => core/comment-content
    [9] => core/comment-date
    [10] => core/comment-edit-link
    [11] => core/comment-reply-link
    [12] => core/comment-template
    [13] => core/comments
    [14] => core/comments-pagination
    [15] => core/comments-pagination-next
    [16] => core/comments-pagination-numbers
    [17] => core/comments-pagination-previous
    [18] => core/comments-title
    [19] => core/cover
    [20] => core/file
    [21] => core/gallery
    [22] => core/home-link
    [23] => core/image
    [24] => core/latest-comments
    [25] => core/latest-posts
    [26] => core/loginout
    [27] => core/navigation
    [28] => core/navigation-link
    [29] => core/navigation-submenu
    [30] => core/page-list
    [31] => core/pattern
    [32] => core/post-author
    [33] => core/post-author-biography
    [34] => core/post-comments-form
    [35] => core/post-content
    [36] => core/post-date
    [37] => core/post-excerpt
    [38] => core/post-featured-image
    [39] => core/post-navigation-link
    [40] => core/post-template
    [41] => core/post-terms
    [42] => core/post-title
    [43] => core/query
    [44] => core/query-no-results
    [45] => core/query-pagination
    [46] => core/query-pagination-next
    [47] => core/query-pagination-numbers
    [48] => core/query-pagination-previous
    [49] => core/query-title
    [50] => core/read-more
    [51] => core/rss
    [52] => core/search
    [53] => core/shortcode
    [54] => core/site-logo
    [55] => core/site-tagline
    [56] => core/site-title
    [57] => core/social-link
    [58] => core/tag-cloud
)

@oandregal
Copy link
Member

I've quickly looked at the places where we require global styles data. Inspected the usage of the wp_get_global_stylesheet (and similar functions) as well as the classes (WP_Theme_JSON and WP_Theme_JSON_Resolver). I wanted to learn if we had changed the places where we use this code (script-loader.php, etc.) and made it so it runs earlier than block registration. I haven't found anything meaningful.

The other reason some blocks may not be registered at that point is that block registration had changed, as Jorge suggested in the trac ticket. A bit out of my expertise at the moment, and given some time-constraints I have this week I'm not sure if I'm going to be able to help investigate much further this path.

@andrewserong
Copy link
Contributor

andrewserong commented Sep 28, 2022

Thanks for writing all that up @oandregal — I think I've found where the caching issue is occuring:

After a git-bisect on wordpress-develop that pointed to this backport commit, I tried looking through the backported server-rendered blocks, and I think I've found the area that's causing the issue in the Template Part block's build_template_part_block_instance_variations function. This function is called when the block is registered, rather than when it's rendered, and uses the get_block_templates function on this line, which appears to eventually further down the line call get_theme_data here, causing the list of registered blocks to be cached.

If I update the following line in register_block_core_template_part to set variations to null, then global styles appears to be working correctly again: https://github.com/WordPress/wordpress-develop/blob/3b63a75108b7f6d5e9112c556285ab1bc80ddbcf/src/wp-includes/blocks/template-part.php#L248

So, the problem appears to be calling a getter function like this at registration time prior to all blocks being registered.

For a bit more background, this caused a few issues in the Gutenberg plugin earlier on. The solution at the time was to adjust the caching in Gutenberg, but it looks like this didn't get to the root of the problem for when the block was backported. Here are some earlier links:

The question now, then, is what's the right fix for this problem? @ramonjd mentioned the idea of potentially calling or adding an action to call clean_cached_data again, similar to the switch theme action: add_action( 'switch_theme', array( 'WP_Theme_JSON_Resolver', 'clean_cached_data' ) );.

I'm wondering if we should either a) avoid calling a getter function for theme data at registration time in the Template Part block, or b) work out a way to reliably clean the cached data at the right time once all blocks are registered. CC: @talldan in case this sparks any ideas, too.

Does anyone have any strong opinions on what the correct caching behaviour should be here?

@talldan
Copy link
Contributor

talldan commented Sep 28, 2022

I'm wondering if we should either a) avoid calling a getter function for theme data at registration time in the Template Part block, or b) work out a way to reliably clean the cached data at the right time once all blocks are registered. CC: @talldan in case this sparks any ideas, too.

A short term fix isn't a bad idea for 6.1. A longer term fix should also be investigated as this code is too fragile. get_block_templates is a public function, and it's one that plugins could very well be interested in calling, so while there's some ability to fix this internally, the deeper problem is exposing brittle APIs for public consumption.

Possibly a quick fix would be to add a way to get templates without filling the theme json cache.

Looking at the deeper problem, to me it seems like there should be some cache invalidation at certain points.

There are also dependency issues. Getting template parts shouldn't require compiling and caching the entire theme json. A lot of the settings in theme json are completely unrelated, so it might be worth breaking the cache into smaller sub-caches. The filling and invalidation of those caches can then be optimized in a way that wouldn't have been possible before.

@andrewserong
Copy link
Contributor

andrewserong commented Sep 28, 2022

Thanks @talldan, you raise some great points! Ideally get_block_templates wouldn't have such potentially risky side-effects 👍

In order to try to find a pragmatic short-term fix, I've opened up a core PR in WordPress/wordpress-develop#3352 to try adding a comparable clean_cached_data static method to the WP_Theme_JSON class and call both it and WP_Theme_JSON_Resolver::clean_cached_data() when the Template Part block has finished gathering its variations. I'm wrapping up for the day now, but happy to try to look into adding tests for it tomorrow if it looks viable (I'm not 100% sure how to reliably test for it, so also very happy for any ideas on that PR if anyone gets the chance 🙂)

I figure a core PR is a better way to test for a fix than doing it in the GB repo. Whichever fix we come up with, we can then port it back over to GB once it's landed.

@ockham
Copy link
Contributor

ockham commented Sep 28, 2022

Thanks @oandregal @andrewserong @talldan for helping track this down!

Upon reading @andrewserong's comment, especially this part:

[...] I think I've found the area that's causing the issue in the Template Part block's build_template_part_block_instance_variations function. This function is called when the block is registered, rather than when it's rendered, and uses the get_block_templates function on this line, which appears to eventually further down the line call get_theme_data here, causing the list of registered blocks to be cached.

...I had one idea off the top of my head: It seems like there's actually a few unneeded function calls, mostly due to the fact that templates and template parts share a lot of logic. Specifically, we're obviously only interested in template parts here; still we end up calling WP_Theme_JSON_Resolver::get_theme_data()->get_custom_templates(). But that's the list of custom page templates that are included in the theme -- which shouldn't have any bearing on template parts.

So maybe we can leverage the fact that we already know (and specify) that we're only interested in template parts upon invoking get_block_templates in order to avoid calling functions that are only relevant to templates (but not parts) 🤔

@ockham
Copy link
Contributor

ockham commented Sep 28, 2022

Uh, nevermind. I think the line we're calling is this (rather than this), so it goes through _add_block_template_part_area_info rather than _add_block_template_info. That's the correct behavior; _add_block_template_part_area_info just also happens to call WP_Theme_JSON_Resolver::get_theme_data() -- albeit to chain it to ->get_template_parts() rather than templates.

So no potential to fix this here 😬

@ockham
Copy link
Contributor

ockham commented Sep 28, 2022

I've filed an alternative fix here, based on y'all's findings: WordPress/wordpress-develop#3359 😊

@andrewserong
Copy link
Contributor

Great work @ockham, that feels much closer to an ideal fix. Just left a comment over on that PR — I think there's still a bit more fussing we need to do with the clearing the resolver cache to get the Button styling in TwentyTwentyTwo working again, but I think we're heading in the right direction!

@andrewserong
Copy link
Contributor

Building on the idea @ockham is exploring, I've opened up another alternative PR in WordPress/wordpress-develop#3373 which tries to be a bit more explicit about when we cache bust in the Theme JSON and Resolver classes — basically, trying to make sure that a change in the number of registered blocks results in the cache being regenerated for just what we need. I think this should fix the issue while still preserving the performance benefits of the current caching.

If anyone gets a chance to take a look, I can work through any feedback on Monday. CC: @talldan @jorgefilipecosta @oandregal

@andrewserong
Copy link
Contributor

andrewserong commented Oct 3, 2022

I've opened (yet another) PR in WordPress/wordpress-develop#3392 to explore a potential solution. @ramonjd and @ajlende rightly pointed out that checking for the number of registered blocks can result in edge cases where the caching isn't busted when we expect it to be. In this alternative PR, I'm proposing adding a registered_block_type action, and to use this to flush the Theme JSON and Resolver caches — that way it's ensured the caches are cleared whenever a block is registered, rather than attempting to infer the current state within the Theme JSON/Resolver classes.

Happy for any feedback / ideas if folks think a different approach would be better!

@oandregal
Copy link
Member

oandregal commented Oct 3, 2022

I've looked at alternatives and WordPress/wordpress-develop#3392 (cleaning data when a new block is registered) sounds theoretically the best approach, though 1) we also need to consider that blocks can be unregistered as well, and 2) there are a few paths 3rd parties can use to register a block AFAIK.

It sounds like WordPress/wordpress-develop#3359 may be a more robust fix for 6.1. We can look at this more holistically after 6.1.

@andrewserong
Copy link
Contributor

Thanks for taking a look @oandregal! I think @ajlende's feedback about moving the "registered" hook to the WP_Block_Type_Registry class would ensure we catch all block registrations (including 3rd party ones), and remove a little duplication. I'll have a play at updating WordPress/wordpress-develop#3392 today.

@andrewserong
Copy link
Contributor

I've updated WordPress/wordpress-develop#3392 to move the hook to the registry class, which now seems to capture all cases where a block might be registered.

@ockham
Copy link
Contributor

ockham commented Oct 4, 2022

Update: We now have a few candidates to fix this issue:

Finally, while not a fix to this issue, there's #44658, which is a related minor performance and code quality improvement.


I'll review these alternatives in a follow-up comment and will try to draw up a strategy that we can take from here.

@ockham
Copy link
Contributor

ockham commented Oct 4, 2022

We have two partial fixes/improvements that I consider very low-risk:

I would then like to land those two.

This leaves us with the caching issue in WP_Theme_JSON_Resolver. I would like to explore this suggestion by @oandregal for a bit, since it seems like it would tackle the caching issue at its core.

If we cannot get that ready in time, we might instead merge a fallback. I'm currently leaning towards WordPress/wordpress-develop#3384, since it doesn't introduce any new APIs.

@ockham
Copy link
Contributor

ockham commented Oct 4, 2022

I think one important thing is to treat the issues separately:

  1. The WP_Theme_JSON PR fixes the manual Global Styles-styling of buttons by the user (this issue).
  2. The theme.json stuff is broken because of WP_Theme_JSON_Resolver caching (#44619).

@c4rl0sbr4v0 is currently looking into @oandregal's suggestion.

@ockham
Copy link
Contributor

ockham commented Oct 4, 2022

Update:

  1. We've now landed the WP_Theme_JSON caching fix; it's part of WP 6.1 Beta 3.
  2. Carlos has filed Try to cache the theme json file reading wordpress-develop#3401 as a tentative fix for [WP 6.1] Block Styles in theme.json do not appear in the Editor or Frontend #44619.

I'm considering closing this issue (since it's arguably fixed), and moving discussion over to #44619 (which AFAICS covers the remaining issue with the theme resolver). Thoughts? 👍 / 👎

@ndiego
Copy link
Member

ndiego commented Oct 4, 2022

I'm considering closing this issue (since it's arguably fixed), and moving discussion over to #44619 (which AFAICS covers the remaining issue with the theme resolver). Thoughts?

Yeah, in my testing, this issue appears to be fixed in Beta 3

@andrewserong
Copy link
Contributor

Thanks for weighing up all the options @ockham, and landing the incremental fixes! 🙇

I'm considering closing this issue (since it's arguably fixed), and moving discussion over to #44619 (which AFAICS covers the remaining issue with the theme resolver). Thoughts? 👍 / 👎

That sounds good to me. Should we keep a trac issue open for #44619, though, as it's still a blocker for 6.1?

@c4rl0sbr4v0 is currently looking into @oandregal's WordPress/wordpress-develop#3359 (comment).

To keep efforts streamlined, I've closed out my alternate PRs now, but do ping me if you need a review on anything. Thanks for digging in to find the right solution!

@ockham
Copy link
Contributor

ockham commented Oct 5, 2022

That sounds good to me. Should we keep a trac issue open for #44619, though, as it's still a blocker for 6.1?

Yeah, good point. I'll file one 👍

To keep efforts streamlined, I've closed out my alternate PRs now, but do ping me if you need a review on anything. Thanks for digging in to find the right solution!

Thank you for all your work and research! ❤️

@ockham ockham closed this as completed Oct 5, 2022
Repository owner moved this from Research/Discussions in progress to Done in WordPress 6.1 Editor Tasks Oct 5, 2022
@ockham
Copy link
Contributor

ockham commented Oct 5, 2022

That sounds good to me. Should we keep a trac issue open for #44619, though, as it's still a blocker for 6.1?

Yeah, good point. I'll file one 👍

https://core.trac.wordpress.org/ticket/56736

pento pushed a commit to WordPress/wordpress-develop that referenced this issue Oct 11, 2022
A significant performance regression was added late in WP 6.1 beta cycle when some of the existing caching for `theme.json` processing was removed. The rationale for removing the caching was this code was now used before all the blocks are registered (aka get template data, etc.) and resulted in stale cache that created issues (see [WordPress/gutenberg#44434 Gutenberg Issue 44434] and [WordPress/gutenberg#44619 Gutenberg Issue 44619]). The changes were limited to only reads from the file system. However, it introduced a big impact in performance.

This commit adds caching and checks for blocks with different origins. How? It add caching for the calculated data for core, theme, and user based on the blocks that are registered. If the blocks haven't changed since the last time they were calculated for the origin, the cached data is returned. Otherwise, the data is recalculated and cached.

Essentially, this brings back the previous cache, but refreshing it when the blocks change.

It partially adds unit tests for these changes. Additional tests will be added.

References:
* [WordPress/gutenberg#44772 Performance regression in WP 6.1 for theme.json processing]

Follow-up to [54251], [54399].

Props aristath, oandregal, bernhard-reiter, spacedmonkey, hellofromTonya.
See #56467.

git-svn-id: https://develop.svn.wordpress.org/trunk@54493 602fd350-edb4-49c9-b593-d223f7449a82
markjaquith pushed a commit to markjaquith/WordPress that referenced this issue Oct 11, 2022
A significant performance regression was added late in WP 6.1 beta cycle when some of the existing caching for `theme.json` processing was removed. The rationale for removing the caching was this code was now used before all the blocks are registered (aka get template data, etc.) and resulted in stale cache that created issues (see [WordPress/gutenberg#44434 Gutenberg Issue 44434] and [WordPress/gutenberg#44619 Gutenberg Issue 44619]). The changes were limited to only reads from the file system. However, it introduced a big impact in performance.

This commit adds caching and checks for blocks with different origins. How? It add caching for the calculated data for core, theme, and user based on the blocks that are registered. If the blocks haven't changed since the last time they were calculated for the origin, the cached data is returned. Otherwise, the data is recalculated and cached.

Essentially, this brings back the previous cache, but refreshing it when the blocks change.

It partially adds unit tests for these changes. Additional tests will be added.

References:
* [WordPress/gutenberg#44772 Performance regression in WP 6.1 for theme.json processing]

Follow-up to [54251], [54399].

Props aristath, oandregal, bernhard-reiter, spacedmonkey, hellofromTonya.
See #56467.
Built from https://develop.svn.wordpress.org/trunk@54493


git-svn-id: http://core.svn.wordpress.org/trunk@54052 1a063a9b-81f0-0310-95a4-ce76da25c4cd
github-actions bot pushed a commit to platformsh/wordpress-performance that referenced this issue Oct 11, 2022
A significant performance regression was added late in WP 6.1 beta cycle when some of the existing caching for `theme.json` processing was removed. The rationale for removing the caching was this code was now used before all the blocks are registered (aka get template data, etc.) and resulted in stale cache that created issues (see [WordPress/gutenberg#44434 Gutenberg Issue 44434] and [WordPress/gutenberg#44619 Gutenberg Issue 44619]). The changes were limited to only reads from the file system. However, it introduced a big impact in performance.

This commit adds caching and checks for blocks with different origins. How? It add caching for the calculated data for core, theme, and user based on the blocks that are registered. If the blocks haven't changed since the last time they were calculated for the origin, the cached data is returned. Otherwise, the data is recalculated and cached.

Essentially, this brings back the previous cache, but refreshing it when the blocks change.

It partially adds unit tests for these changes. Additional tests will be added.

References:
* [WordPress/gutenberg#44772 Performance regression in WP 6.1 for theme.json processing]

Follow-up to [54251], [54399].

Props aristath, oandregal, bernhard-reiter, spacedmonkey, hellofromTonya.
See #56467.
Built from https://develop.svn.wordpress.org/trunk@54493


git-svn-id: https://core.svn.wordpress.org/trunk@54052 1a063a9b-81f0-0310-95a4-ce76da25c4cd
ootwch pushed a commit to ootwch/wordpress-develop that referenced this issue Nov 4, 2022
A significant performance regression was added late in WP 6.1 beta cycle when some of the existing caching for `theme.json` processing was removed. The rationale for removing the caching was this code was now used before all the blocks are registered (aka get template data, etc.) and resulted in stale cache that created issues (see [WordPress/gutenberg#44434 Gutenberg Issue 44434] and [WordPress/gutenberg#44619 Gutenberg Issue 44619]). The changes were limited to only reads from the file system. However, it introduced a big impact in performance.

This commit adds caching and checks for blocks with different origins. How? It add caching for the calculated data for core, theme, and user based on the blocks that are registered. If the blocks haven't changed since the last time they were calculated for the origin, the cached data is returned. Otherwise, the data is recalculated and cached.

Essentially, this brings back the previous cache, but refreshing it when the blocks change.

It partially adds unit tests for these changes. Additional tests will be added.

References:
* [WordPress/gutenberg#44772 Performance regression in WP 6.1 for theme.json processing]

Follow-up to [54251], [54399].

Props aristath, oandregal, bernhard-reiter, spacedmonkey, hellofromTonya.
See #56467.

git-svn-id: https://develop.svn.wordpress.org/trunk@54493 602fd350-edb4-49c9-b593-d223f7449a82
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json [Priority] High Used to indicate top priority items that need quick attention [Type] Bug An existing feature does not function as intended
Projects
No open projects
Development

No branches or pull requests

7 participants