Duotone: Action causes premature caching of theme.json block nodes

[aaronrobertshaw]

Description

In #49103, SVG filter output was limited to only used filters. Part of this approach involved scraping block names using filters via the wp_loaded hook. It appears this is causing the theme.json block node processing to occur before the get_current_screen global function is available which prevents editor-only selectors from being used in theme.json.

Expected Behaviour

Duotone filter processing would occur after we might alter selectors for the editor.

Actual behaviour

Duotone actions cause caching of theme.json block node data prematurely.

Not being able to generate editor-only selectors via theme.json means that #46496 causes a regression with the Image block and the border not being applied to the inline cropping area.

Potential Solutions

1. Using a hook that executes later. (Blindly updating the hook used by the duotone filter to wp_enqueue_scripts fixes the editor-only selectors although I don't know the implications for duotone)
2. Somehow invalidate theme.json block metadata cache when retrieving styles for the editor

Step-by-step reproduction instructions

1. Add border styles to styles.blocks['core/image'].border in theme.json

   // theme.json
   {
   	"styles": {
   	 	"blocks": {
   	 		"core/image": {
   	 			"border": {
   	 				"radius": "24px",
   	 				"style": "solid",
   	 				"color": "red",
   	 				"width": "4px"
   	 			}
   	 	 	}
   		}
   	}
   }

2. In the post editor and an image block and add a border to it
3. Select to crop the image and notice the border is lost.
4. Confirm there are no styles output matching the editor-only selector from image/block.json

Screenshots, screen recording, code snippet

No response

Environment info

• WP 6.1 / 6.2
• Gutenberg trunk

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

[aaronrobertshaw]

cc/ @ajlende @jeryj would you mind taking a look at this one? In particular, if it is possible for the duotone action to use a hook later in the cycle?

[aaronrobertshaw]

While updating Duotone to use the selectors API in #49325. I found that moving the set_global_style_block_names action from wp_loaded to parse_request was sufficient to both, keep duotone working on the frontend (using #49325), and delay the theme.json block node retrieval long enough for the editor-only selectors to work via get_current_screen.

This seems too brittle though so any ideas on perhaps invalidating the theme.json block metadata cache when accessed for the first time in the editor would be welcome 🙏

Quick hack in `WP_Theme_JSON_Gutenberg::get_blocks_metadata`

diff --git a/lib/class-wp-theme-json-gutenberg.php b/lib/class-wp-theme-json-gutenberg.php
index 8f4054bf46..a25ecada08 100644
--- a/lib/class-wp-theme-json-gutenberg.php
+++ b/lib/class-wp-theme-json-gutenberg.php
@@ -37,6 +37,17 @@ class WP_Theme_JSON_Gutenberg {
 	 */
 	protected static $blocks_metadata = array();
 
+	/**
+	 * Holds the same data as $blocks_metadata except that it is only
+	 * populated once blocks metadata has been cached in the editor. This is
+	 * useful when there should be editor-only selectors and we need to ensure
+	 * their inclusion despite theme.json potential already generating metadata
+	 * during the earlier execution of hook actions and filters.
+	 *
+	 * @var array
+	 */
+	protected static $editor_blocks_metadata = array();
+
 	/**
 	 * The CSS selector for the top-level styles.
 	 *
@@ -846,10 +857,19 @@ class WP_Theme_JSON_Gutenberg {
 		$registry = WP_Block_Type_Registry::get_instance();
 		$blocks   = $registry->get_all_registered();
 
+		$in_editor = false;
+		if ( function_exists( 'get_current_screen' ) ) {
+			$in_editor = get_current_screen()->is_block_editor;
+		}
+
 		// Is there metadata for all currently registered blocks?
-		$blocks = array_diff_key( $blocks, static::$blocks_metadata );
+		$blocks = array_diff_key(
+			$blocks,
+			$in_editor ? static::$editor_blocks_metadata : static::$blocks_metadata
+		);
+
 		if ( empty( $blocks ) ) {
-			return static::$blocks_metadata;
+			return $in_editor ? static::$editor_blocks_metadata : static::$blocks_metadata;
 		}
 
 		foreach ( $blocks as $block_name => $block_type ) {
@@ -880,6 +900,10 @@ class WP_Theme_JSON_Gutenberg {
 			}
 		}
 
+		if ( $in_editor ) {
+			static::$editor_blocks_metadata = static::$blocks_metadata;
+		}
+
 		return static::$blocks_metadata;
 	}
 

@oandregal do you have any wisdom to share?

[oandregal]

This seems too brittle though so any ideas on perhaps invalidating the theme.json block metadata cache when accessed for the first time in the editor would be welcome pray

I would recommend against this.

It seems to me that the core issue is that WP_Theme_JSON_Gutenberg uses the get_current_screen when it shouldn't, as it makes this class coupled to a certain WordPress flow. Ideally, this class should be "pure": unaware of the context, the output should only depend on the input.

I don't know enough of the new selectors API to be more concrete, sorry, but I can start by offering the following thoughts:

• Can we make "the context" a parameter of some method? For example: the get_stylesheet method takes a $types array because it needs different output depending on the context (editor, front, etc.).
• Why the new selector API needs editor selectors and front-end selectors when the old API didn't?

[jeryj]

In particular, if it is possible for the duotone action to use a hook later in the cycle?

Honestly, I looked at where some other blocks had commonly hooked into for their actions. If delaying the action to parse_request works to resolve the issue and keeps duotone output working correctly on the frontend, then that's fine with me! I'll test it out and open a PR.

cc @ajlende

[jeryj]

@aaronrobertshaw I put in a PR at #49343 to update the action to parse_request. I'm having trouble reproducing this issue though. I've tried in two different environments, thinking maybe it was related to the dev environment not using the caching or something similar. It's very possible I'm misunderstanding the testing instructions.

Edit: @scruffian helped me understand how to reproduce it. I was applying the global duotone style to the image in the editor, rather than via the global styles theme editor.

[aaronrobertshaw]

Glad you were able to sort it out @jeryj 👍

I was applying the global duotone style to the image in the editor, rather than via the global styles theme editor.

That's the catch. Global styles are global styles and aren't applied to specific individual blocks in the block editor. The application of a duotone filter to a specific block is a "block support". There's so much overlap between the two it gets tricky to maintain the difference and nuance in conversation.

It seems to me that the core issue is that WP_Theme_JSON_Gutenberg uses the get_current_screen when it shouldn't

🙇 Thanks @oandregal for the idea. Just the sort of feedback I was hoping to get on the Selectors API PR as I wasn't sure about this approach and it was part of why I was chasing a final review of the API before merging. Unfortunately, I didn't communicate that desire clearly in my PR comments, so it merged prematurely.

We have a little time before the next Gutenberg release, so If I can't move the detection of being in the editor, outside the class, the switch to the parse_request action is a quick fix to buy some time.

Can we make "the context" a parameter of some method? For example: the get_stylesheet method takes a $types array because it needs different output depending on the context (editor, front, etc.).

I'll try and put a PR together for this. Honestly, though at this point, I don't exactly know where I should move the detection of being in the editor, to be able to pass it as context.

Why the new selector API needs editor selectors and front-end selectors when the old API didn't?

You could argue the old API did need editor selectors but fell short in not offering them. By not offering editor-only selectors, we were outputting useless CSS on the frontend just so we could get global styles to be applied correctly in the editor.

The counterpoint to that was the old API didn't need editor-only selectors because it didn't care about the extra unused CSS on the frontend.