Duotone: Only output Duotone filters that are used #48291

scruffian · 2023-02-21T17:20:11Z

What?

This is an attempt at fixing #38299. The idea is that by outputting only the SVGs that are used, rather than all of the presets, we can remove a lot of unused SVGs on the frontend.

Why?

Less code will make the page load faster.

How?

Create a new WP_Gutenberg class.
Add an action to the WP_Theme_JSON class to make is possible to hook onto declarations.
Save a list of the CSS variables that duotone uses to reference SVGs, in a static array on the new class.
Add a filter to the WP_Theme_JSON class to make is possible to output SVGs from another class.
Output the SVGs that are saved in the static array created in step 1.

Testing Instructions

Switch to a theme that has a default duotone applied to all blocks (you can use the Sherbet variation of TT3)
Add an image to a post
Confirm that the image has a duotone applied
Check the page source and check that none of the default duotone SVGs are being output.

Testing Instructions for Keyboard

For step 3 above check that the source contains an SVG with an ID which is the same as the id that the CSS on an image references in its filter property.
For step 4 check that there are no other SVGs with filters in this format on the page:
<filter id="wp-duotone-*">

lib/class-wp-theme-json-gutenberg.php

lib/compat/wordpress-6.2/script-loader.php

lib/class-wp-theme-json-gutenberg.php

scruffian · 2023-02-21T17:24:55Z

lib/class-wp-theme-json-gutenberg.php

@@ -2425,8 +2461,34 @@ function( $pseudo_selector ) use ( $selector ) {
 		$declarations_duotone = array();
 		foreach ( $declarations as $index => $declaration ) {
 			if ( 'filter' === $declaration['name'] ) {
-				unset( $declarations[ $index ] );
+				$filter_preset = $this->gutenberg_get_preset_from_declaration( $declarations[ $index ] );


This is the one area that needs a lot more attention. What we have is a filter preset of the form var(--wp--preset--duotone--(.*)). What we need is a preset object from theme.json to get the settings so we can output the correct SVG. I think there must be a better way of getting the preset than using the slug like this.

lib/class-wp-theme-json-gutenberg.php

ajlende · 2023-02-22T02:25:42Z

I feel a bit weird about adding a side-effect to get_styles_for_block. It's the sort of function that could end up getting called elsewhere outside of rendering which could cause issues.

I wonder if we could do something similar to this function that just returns the string of SVGs and then call it in gutenberg_enqueue_global_styles or keep the wp_body_open action?

scruffian · 2023-02-22T10:01:07Z

lib/compat/wordpress-6.2/script-loader.php

+ *
+ * @since 5.9.1
+ */
+function gutenberg_global_styles_render_svg_filters() {


This is just a copy of wp_global_styles_render_svg_filters

scruffian · 2023-02-22T10:01:44Z

lib/compat/wordpress-6.2/get-global-styles-and-settings.php

+ *
+ * @return string
+ */
+function gutenberg_get_global_styles_svg_filters() {


This is just a copy of gutenberg_get_global_styles_svg_filters with the reference to the WP_Theme_JSON_Resolver class updated to WP_Theme_JSON_Resolver_Gutenberg.

getdave · 2023-02-22T15:03:50Z

As I see it the problem people have is that we're dumping a tonne of SVG markup on the page when those SVGs may not be used. So what we need is a way to retain the ability to select from all of the presets but only load the SVGs for the presets that are in use.

This PR partly addresses this but might leave us in a situation where the available presets are not presented/available to the user because they were not currently in use on the given page.

Perhaps a different approach could be to

allow all presets to be defined and availanle as CSS custom props
only inline SVGs that are currently in use
lazy load any SVGs that are needed as a result of users selecting a new preset

scruffian · 2023-02-22T15:41:56Z

This PR partly addresses this but might leave us in #48318 because they were not currently in use on the given page.

I don't think this is true. The presets will continue to be presented to the user, they just might not be output in the frontend.

scruffian · 2023-02-22T17:57:08Z

I updated the approach here to use actions and filters. I know that this comes with a maintenance burden, but the benefit in this case is that it will allow other plugins to provide their own filters. cc @oandregal

jeryj · 2023-02-22T18:31:26Z

I updated the approach here to use actions and filters. I know that this comes with a maintenance burden,

What is the extra maintenance burden with using actions and filters here?

scruffian · 2023-02-22T18:55:09Z

What is the extra maintenance burden with using actions and filters here?

Once an action / filter is in core we have to support it indefinitely.

jeryj · 2023-02-22T20:10:05Z

lib/block-supports/duotone.php

+		$duotone_preset_css_var = WP_Theme_JSON_Gutenberg::get_preset_css_var( array( 'color', 'duotone' ), $duotone_preset['slug'] );
+
+		// Only output the preset if it's used by a block.
+		if ( in_array( $duotone_preset_css_var, WP_Duotone::$duotone_presets, true ) ) {


I'm on Theme Twenty Twenty Two and getting a fatal error at this line due to WP_Duotone::$duotone_presets being null. I haven't figured out why it's getting set to null yet, but adding an is_array( WP_Duotone::$duotone_presets ) && check does fix the error.

Thanks. It's null because TT2 doesn't set a duotone by default.

The in_array check on WP_Duotone::$duotone_presets causes a fatal error if there are no filter declarations present in the theme json.

…s/gutenberg into update/output-only-used-filters

github-actions · 2023-02-22T21:44:50Z

Flaky tests detected in 4e1876d.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4247183882
📝 Reported issues:

[Flaky Test] should always expand single line selection #39787 in specs/editor/various/multi-block-selection.test.js

scruffian · 2023-02-22T21:48:25Z

One problem with the approach here is that it still outputs the CSS variables for the filters that aren't being used.

ajlende · 2023-02-22T23:33:29Z

lib/block-supports/duotone.php

+			// I think we should do this differently, but I'm not sure how.
+			$filters .= '<style>body{' . $duotone_preset_css_var . ':'. gutenberg_get_duotone_filter_property( $duotone_preset ) . ';}</style>';


It would be nice if it could still be handled by value_func in WP_Theme_JSON_Gutenberg instead of moving it here.

Maybe we have to think a little more about where the filters/actions belong in that code? Could it be more useful in gutenberg_add_global_styles_for_blocks—modifying the $metadata passed into get_styles_for_block?

I think @ajlende has a point. Somehow appending that preset CSS code doesn't feel it belongs in get_filter_svg and since it's a preset and WP Theme json gives us a way to add callback for presets then we should use that.

getdave · 2023-02-23T10:46:09Z

One problem with the approach here is that it still outputs the CSS variables for the filters that aren't being used.

I don't see that as a major problem and not one you need to tackle here. Getting rid of the SVG markup is the main thing.

oandregal · 2023-02-28T12:28:11Z

I updated the approach here to use actions and filters. I know that this comes with a maintenance burden, but the benefit in this case is that it will allow other plugins to provide their own filters. cc @oandregal

Thanks for the ping. Not super fan of adding a filter to a private method/class, as it prevents us from modifying that private unit going forward (it makes it effectively public and part of a flow we should not change). What other options do we have?

Is allowing plugins to modify the theme.json schema something we need? I'd decouple that decision from what this PR is trying to achieve, as it comes with a different set of trade-offs.

scruffian · 2023-03-08T11:55:54Z

Closing in favour of #48374

Duotone: Only output Duotone filters that are used

afb2547

scruffian commented Feb 21, 2023

View reviewed changes

lib/class-wp-theme-json-gutenberg.php Outdated Show resolved Hide resolved

scruffian commented Feb 21, 2023

View reviewed changes

lib/compat/wordpress-6.2/script-loader.php Outdated Show resolved Hide resolved

scruffian commented Feb 21, 2023

View reviewed changes

lib/class-wp-theme-json-gutenberg.php Outdated Show resolved Hide resolved

scruffian commented Feb 21, 2023

View reviewed changes

scruffian self-assigned this Feb 21, 2023

scruffian requested a review from ajlende February 21, 2023 17:29

ajlende reviewed Feb 22, 2023

View reviewed changes

lib/class-wp-theme-json-gutenberg.php Outdated Show resolved Hide resolved

simplify

0335718

scruffian commented Feb 22, 2023

View reviewed changes

scruffian added 4 commits February 22, 2023 11:15

just compare the CSS variables

cb94566

remove unused code

c8e659a

remove unconnected change

f6b610a

fix linter

7e03c5d

scruffian marked this pull request as ready for review February 22, 2023 11:24

scruffian requested a review from spacedmonkey as a code owner February 22, 2023 11:24

Fix linter

117cdf3

scruffian mentioned this pull request Feb 22, 2023

Use Duotone presets in block duotone attributes #48318

Merged

use actions and filters to make the approach more generic

1acc298

remove unused code

3b06685

jeryj reviewed Feb 22, 2023

View reviewed changes

jeryj and others added 2 commits February 22, 2023 14:21

Default static $duotone_preset to empty array

7623952

The in_array check on WP_Duotone::$duotone_presets causes a fatal error if there are no filter declarations present in the theme json.

Merge branch 'update/output-only-used-filters' of github.com:WordPres…

4e1876d

…s/gutenberg into update/output-only-used-filters

move the place where we output the css for the preset

74ae720

ajlende reviewed Feb 22, 2023

View reviewed changes

scruffian mentioned this pull request Feb 23, 2023

Duotone: Output duotone presets per block #48374

Closed

2 tasks

scruffian closed this Mar 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Duotone: Only output Duotone filters that are used #48291

Duotone: Only output Duotone filters that are used #48291

scruffian commented Feb 21, 2023 •

edited by draganescu

Loading

scruffian Feb 21, 2023

ajlende commented Feb 22, 2023

scruffian Feb 22, 2023

scruffian Feb 22, 2023

getdave commented Feb 22, 2023 •

edited

Loading

scruffian commented Feb 22, 2023

scruffian commented Feb 22, 2023

jeryj commented Feb 22, 2023

scruffian commented Feb 22, 2023

jeryj Feb 22, 2023

scruffian Feb 22, 2023

github-actions bot commented Feb 22, 2023 •

edited

Loading

scruffian commented Feb 22, 2023

ajlende Feb 22, 2023

draganescu Feb 23, 2023

getdave commented Feb 23, 2023

oandregal commented Feb 28, 2023

scruffian commented Mar 8, 2023

		// I think we should do this differently, but I'm not sure how.
		$filters .= '<style>body{' . $duotone_preset_css_var . ':'. gutenberg_get_duotone_filter_property( $duotone_preset ) . ';}</style>';

Duotone: Only output Duotone filters that are used #48291

Duotone: Only output Duotone filters that are used #48291

Conversation

scruffian commented Feb 21, 2023 • edited by draganescu Loading

What?

Why?

How?

Testing Instructions

Testing Instructions for Keyboard

scruffian Feb 21, 2023

Choose a reason for hiding this comment

ajlende commented Feb 22, 2023

scruffian Feb 22, 2023

Choose a reason for hiding this comment

scruffian Feb 22, 2023

Choose a reason for hiding this comment

getdave commented Feb 22, 2023 • edited Loading

scruffian commented Feb 22, 2023

scruffian commented Feb 22, 2023

jeryj commented Feb 22, 2023

scruffian commented Feb 22, 2023

jeryj Feb 22, 2023

Choose a reason for hiding this comment

scruffian Feb 22, 2023

Choose a reason for hiding this comment

github-actions bot commented Feb 22, 2023 • edited Loading

scruffian commented Feb 22, 2023

ajlende Feb 22, 2023

Choose a reason for hiding this comment

draganescu Feb 23, 2023

Choose a reason for hiding this comment

getdave commented Feb 23, 2023

oandregal commented Feb 28, 2023

scruffian commented Mar 8, 2023

scruffian commented Feb 21, 2023 •

edited by draganescu

Loading

getdave commented Feb 22, 2023 •

edited

Loading

github-actions bot commented Feb 22, 2023 •

edited

Loading