Make WordPress Core

Opened 20 months ago

Closed 20 months ago

Last modified 20 months ago

#56736 closed defect (bug) (fixed)

Block Styles in theme.json do not appear in the Editor or Frontend

Reported by: bernhard-reiter's profile Bernhard Reiter Owned by: davidbaumwald's profile davidbaumwald
Milestone: 6.1 Priority: normal
Severity: major Version: 6.1
Component: Editor Keywords: has-patch commit
Focuses: Cc:

Description

Originally reported by @ndiego in https://github.com/WordPress/gutenberg/issues/44619.
Simplified testing instructions based on this comment.

Description

When Gutenberg is not active, block styles in theme.json are not getting rendered in the Editor or Frontend in WordPress 6.1 Beta 3. Activating Gutenberg solves this.

Step-by-step reproduction instructions

To reproduce, insert some button blocks and view on the frontend. They should be rectangular, with a dark green background (rather than round and black).

Actual (broken):

https://user-images.githubusercontent.com/14988353/192704978-e43957d7-8868-490c-814d-83c6fe24fae1.png

Expected (correct):

https://user-images.githubusercontent.com/14988353/192704825-6cc455bc-dde5-45ba-b94d-b4e584e92e9e.png

Change History (20)

#1 @Bernhard Reiter
20 months ago

While working on https://github.com/WordPress/gutenberg/issues/44434 (which is somewhat related), we've identified WP_Theme_JSON_Resolver's caching of theme data as the problem. That caching happens early and sanitizes block-specific styling found in theme.json against the list of registered blocks. However, this can happen before all blocks are registered. As a consequence, some styling is scrapped.

@cbravobernal has started working on a fix in https://github.com/WordPress/wordpress-develop/pull/3401.

This ticket was mentioned in PR #3384 on WordPress/wordpress-develop by ajlende.


20 months ago
#3

This is another (partial) alternative to #3352, #3359, and #3373.

This adjusts the priorities of block registration so that the core/template-part block is registered last. This means that all blocks (except core/template-part) will be registered before the metadata form get_blocks_metadata is called for the first time.

It currently has the problem that the block metadata for the core/template-part block itself won't be included, but that may be fixed with the combination of cache invalidation from #3373 if we can guarantee that register_block_core_template_part never unregisters any blocks.

I upped the WP_Block_Supports::init priority value to make room for blocks like `core/post-comments` to be registered after the rest of the core blocks, but before the core/template-part registration.

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

#4 @Bernhard Reiter
20 months ago

I've linked another potential fix: https://github.com/WordPress/wordpress-develop/pull/3384
Note that this works based on changing the priority of actions.

ockham commented on PR #3401:


20 months ago
#5

Rebased.

ockham commented on PR #3401:


20 months ago
#6

I think this is working now 🎉

I was about to implement file caching for the parent theme (see https://github.com/WordPress/wordpress-develop/pull/3401#discussion_r986868064), but then it occurred to me that we might solve this slightly differently:

We can add a single $theme_json_file_cache static var, and use it from within static::read_json_file().

#7 @SergeyBiryukov
20 months ago

  • Milestone changed from Awaiting Review to 6.1

This ticket was mentioned in PR #3408 on WordPress/wordpress-develop by ockham.


20 months ago
#8

Based on @c4rl0sbr4v0's https://github.com/WordPress/wordpress-develop/pull/3401, and @oandregal's suggestion.

### Description

Currently, individual block styling defined in a theme's theme.json isn't being applied on the frontend (see https://core.trac.wordpress.org/ticket/56736 or the testing instructions below).

While working on https://github.com/WordPress/gutenberg/issues/44434 (which is somewhat related), we've identified WP_Theme_JSON_Resolver's caching of theme data as the problem. That caching happens early and sanitizes block-specific styling found in theme.json against the list of registered blocks. However, this can happen before all blocks are registered. As a consequence, some styling is scrapped.

This can be fixed by removing the caching from WP_Theme_JSON_Resolver::get_theme_data(), and instead freshly computing theme data each time that method is called, which will take into account all blocks registered at that point.

In order to prevent performance from being impacted all too much, we add caching to the JSON file reading and processing in WP_Theme_JSON_Resolver::read_json_file() instead (per this suggestion).

### Testing instructions

To test, activate the TT2 theme. In a new post, insert some button blocks and view on the frontend. They should be rectangular, with a dark green background (rather than round and black).

| Before (broken) | After (fixed) |
| --- | --- |
| https://i0.wp.com/user-images.githubusercontent.com/14988353/192704978-e43957d7-8868-490c-814d-83c6fe24fae1.png | https://i0.wp.com/user-images.githubusercontent.com/14988353/192704825-6cc455bc-dde5-45ba-b94d-b4e584e92e9e.png |

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

ockham commented on PR #3408:


20 months ago
#9

As a follow-up, we could consider caching WP_Theme_JSON_Resolver::translate() for some more performance gains. (We might wanna use a checksum generated from its $theme_json arg as cache key.)

Alternatively, we could explore introducing a new, cached method that calls both read_json_file and translate after each other, since those seem to almost always occur in sequence.

ockham commented on PR #3408:


20 months ago
#10

cc/ @andrewserong @oandregal @ndiego @c4rl0sbr4v0 for review

oandregal commented on PR #3408:


20 months ago
#11

We also need to remove the caching in get_core_data and get_user_data. I'm looking at pushing changes to this PR. I have them locally, have to coordinate with Bernie for permissions.

#12 @oandregal
20 months ago

The PR https://github.com/WordPress/wordpress-develop/pull/3408 is ready to be committed. Any core committer around that can merge it?

#13 @davidbaumwald
20 months ago

  • Keywords commit assigned-for-commit added
  • Owner set to davidbaumwald
  • Status changed from new to reviewing

#14 @davidbaumwald
20 months ago

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

In 54399:

Editor: Ensure block styles in theme.json are rendered.

This change removes the caching of theme data in WP_Theme_JSON_Resolver::get_theme_data(), instead freshly compiling theme data on each call.

Also, to prevent any performance degradation by the removal, the file contents of theme.json files are now cached in WP_Theme_JSON_Resolver::read_json_file(), preventing multiple filesystem reads.

Follow-up to [54385].

Props ndiego, bph, mikachan, andrewserong, oandregal, cbravobernal, bernhard-reiter, aristath.
Fixes #56736.

#16 @davidbaumwald
20 months ago

  • Keywords assigned-for-commit removed

ockham commented on PR #3408:


20 months ago
#17

Unfortunately, it seems like this introduced a major performance issue after all: https://github.com/WordPress/gutenberg/issues/44772#issuecomment-1271448734 😕

oandregal commented on PR #3408:


20 months ago
#18

As I mentioned https://github.com/WordPress/gutenberg/issues/44772#issuecomment-1271447927 it'd be good to have some before/after numbers. Ari mentioned he's into that (my setup is malfunctioning).

c4rl0sbr4v0 commented on PR #3401:


20 months ago
#19

https://github.com/WordPress/wordpress-develop/pull/3408 does the work, so we can close this one.

ajlende commented on PR #3384:


20 months ago
#20

Closing this out. Both related trac tickets have been solved.

Note: See TracTickets for help on using tickets.