Background images: resolve ref and ensure appropriate default values

[ramonopoly]

Background image theme.json styling for blocks is slated for 6.7. See #61588

This ticket tracks the following enhancements to background image block styles supports:

Add support for "ref" values in theme.son

Theme.json values support "referencing" another value in theme.json using the "ref" keyword.

These values are resolved by the WP_Theme_JSON class.

"background" style properties do not yet support this feature.

Default values are not applied consistently

Blocks with user-applied background images, i.e., background images added from a site's media library, receive certain default background size and position styles.

These default styles increase the likelihood that an image sits well in a block without the user doing anything else.

An enhancement here is to extend these defaults for uploaded images in block global styles.

Why set defaults only for uploaded images and not those defined in theme.json?

Setting defaults changes the way the background-image appears.

Images added by the user in the editor receive defaults to increase the chances that they look "okay" in the editor immediately.

We should respect, as much as possible, the values in theme.json, that includes values that are not set. The assumption is theme developers explicitly add and omit values. If the editor were to add default to theme.json styles, it would undermine that assumption.

A PR to sync the changes in:

• ​https://github.com/WordPress/gutenberg/pull/64192
  • ​https://github.com/WordPress/gutenberg/pull/64328 (follow up to the above)
• ​https://github.com/WordPress/gutenberg/pull/64128

## Default values

Block background images have long had "default" values to optimize their appearance.

For example, block styles receive a default background size of "cover" in the editor and the frontend. Or, where the background size is "contain" the background position is "center".

Defaults have always applied to images uploaded by the user in the editor.

The PR brings a bit of consistency to background image styles.

Block styles "inherit" values from global styles/theme.json and display the current value (whether the set, inherited or default value) in the editor controls.

In relation to default values:

• Site wide background images (uploaded or otherwise) do not receive any default values.
• Block background images defined in theme.json do not receive any default values.
• Block background images that have been uploaded (images with ids) receive background default values.

## Resolving theme.json "ref" values

Add support for ref resolution to "background" style properties.

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

While comparing the code changes here with their Gutenberg counterparts, it appears this backport doesn't include all the latest changes that landed in GB.

Thanks for double checking! I'll fix this first thing this morning 🙇🏻

I'd left the previous "unset" ref logic (and related test) in there. 🤦🏻

I think it should be pretty close now, but I'm a bit bug-eyed .

There's one inconsistency between this PR and Gutenberg trunk, where Gutenberg trunk is wrong.

I have a sync PR here:

​https://github.com/WordPress/gutenberg/pull/64564

This was due to a bit of flip-flopping on features and I forgot to remove it.

🙇🏻

For some reason I'm having trouble testing this. I've added "id": 123 to the verse block's background.backgroundImage object in my local theme.json file, but on the site frontend I'm not seeing the default cover background-size rule. Instead I'm only seeing the background image rule:

My bad @andrewserong

I forgot to specify that the image cannot be a relative image - is that what you were testing with?

The reason being, relative images won't ever have ids because they're not uploaded to the media library and therefore not saved to the database with an id.

In that way "id" is reserved attribute for uploaded images.

Any "id"s added to a background image object with a relative path will be stripped out here: ​https://github.com/ramonjd/wordpress-develop/blob/03bf3461c853eaed09fe383404829fd2f74d923e/src/wp-includes/class-wp-theme-json-resolver.php#L946-L946

Now, that's not intentional, it's just the practical reality 😄

The other side of the coin is that if we support more properties in background.backgroundImage, they'll have to be merged correctly for relative path images.

This should work:

"core/verse": {
                                "background": {
                                        "backgroundImage": {
                                                "url": "http://localhost:8889/wp-content/themes/twentytwentyfour/assets/images/building-exterior.webp",
                                                "id": 123
                                        }
                                },
                                "color": {
                                        "text": "white"
                                },
                                "dimensions": {
                                        "minHeight": "100px"
                                }
                        },

[ramonopoly]

In 58936:

Background images: resolve theme.json dynamic ref values and ensure appropriate style default values

The commit syncs the following changes from Gutenberg:

• Background images: add support for theme.json ref value resolution gutenberg#64128
• Background images: ensure appropriate default values gutenberg#64192
• Background image: ensure consistency with defaults and fix reset/remove functionality gutenberg#64328

These changes brings consistency to the default background image styles WordPress applies to user uploaded images, and adds support for ref resolution to "background" style properties.

Props andrewserong, aaronrobertshaw.

Fixes #61858

Committed in:

• r58936
• ​https://github.com/WordPress/wordpress-develop/commit/830d66c55cdb5afcaa8c0137d0c0f991258565ee

[mukesh27]

Reopening as we found ​https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-includes/class-wp-theme-json.php#L3239-L3251 is too expensive see the profiling result. I removed the code and did the comparison.

See ​https://github.com/WordPress/performance/issues/1572#issuecomment-2387861711 for more information

### Server-timing for TT4 home/blog page

Mertic	Beta 1	Beta 1 with remove code	Diff %	Diff abs.
Response Time	252.84	247.12	-2.26%	-5.72
wp-before-template	76.26	76.28	0.03%	0.02
wp-before-template-db-queries	3.3	3.35	1.52%	0.05
wp-template	173.42	167.66	-3.32%	-5.76
wp-total	249.87	244.23	-2.26%	-5.64
wp-template-db-queries	2.6	2.59	-0.38%	-0.01

🔴 It clear shows that the code introduce ~3% regression

[ramonopoly]

Thanks for the info @mukesh27

I'm AFK for a week, but looking at the code and the ​report I think we can avoid calling ​WP_Theme_JSON::get_blocks_metadata, which seems to be the source of the regression.

Instead of iterating over the output of WP_Theme_JSON::get_styles_block_nodes(), I think it's enough to iterate over $incoming_data['styles']['blocks'] to check block styles for object values.

I can look when I get back if there's no movement in between.

cc @andrewserong for visibility.

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

See ​https://github.com/WordPress/performance/issues/1572#issuecomment-2387861711 for more details about the performance regression.

### Server-timing After the PR

Mertic	Beta 1	PR	Diff %	Diff abs.
Response Time	110.43	106.22	-3.81%	-4.21
wp-before-template	68.19	70.02	2.68%	1.83
wp-before-template-db-queries	2.71	2.63	-2.95%	-0.08
wp-template	41.12	34.52	-16.05%	-6.60
wp-total	108.73	104.24	-4.13%	-4.49
wp-template-db-queries	2.09	1.97	-5.74%	-0.12

The branchmark shows that it improve the performance.

Someone needs to verify the approach and the performance numbers.

### Server-timing After the PR impelemtation.

Mertic	Trunk	PR	Diff %	Diff abs.
Response Time	71.64	68.96	-3.74%	-2.68
wp-before-template	30.99	30.74	-0.81%	-0.25
wp-template	37.56	35.82	-4.63%	-1.74
wp-total	69.78	66.58	-4.59%	-3.20

[joemcgill]

Adding the performance focus to this ticket as we investigate a way to address the regression found after Beta 1.

Not sure if it helps, but my understanding is that background images aren't supported for element styles yet (i.e. they're only supported for blocks themselves). Would it make it simpler to not include support for elements at this stage, then we could go with something like the first commit in this PR? Then we could revisit further down the track if or when we wished to add support for background images on elements.

Just an idea, though, in case it helps wrangle the complexity. I don't want to derail the discussion if the current path is a better fix!

In ​https://github.com/WordPress/wordpress-develop/pull/7486/commits/76e0140f145a8053ece8fb2e31f822d71f2fe662 i introduce two option.

• include_node_paths_only only return path for the nodes, Default false
• include_block_elements per @andrewserong comment if we want to remove it then we have to pass false in option array.

@felixarntz @andrewserong Could you please take look when you have moment. Thanks

### Server-timing After the PR implementation.

Mertic	Trunk	PR	Diff %	Diff abs.
Response Time	68.94	66.46	-3.60%	-2.48
wp-before-template	30.17	29.74	-1.43%	-0.43
wp-template	36.26	34.57	-4.66%	-1.69
wp-total	67.03	64.52	-3.74%	-2.51

Thanks for the updates here. Apologies if I missed some context, but it looks like this adds a fair bit of additional complexity, where the original commit took a simpler approach. Was there a reason we need to make the additional changes? Since we're only using this flag in one place, I wondered if it's worth it to try to reduce the changeset. Or were there other instances where you think we might re-use this flag?

@andrewserong In my quick search I don't find any place where we can use these option, happy to update other places as it return lightweight version of array values.

Per ​https://github.com/WordPress/wordpress-develop/pull/7486#discussion_r1786330683, to add support for elements we updated the approach.

Ping @ramonjd as he added this feature in core and have more context.

@andrewserong

Thanks for the updates here. Apologies if I missed some context, but it looks like this adds a fair bit of additional complexity, where the original commit took a simpler approach. Was there a reason we need to make the additional changes? Since we're only using this flag in one place, I wondered if it's worth it to try to reduce the changeset. Or were there other instances where you think we might re-use this flag?

Can you clarify what you consider additional complexity in relation to what other version of the code? I'm not sure I understand what you're referring to.

Can you clarify what you consider additional complexity in relation to what other version of the code? I'm not sure I understand what you're referring to.

The original commit in ​https://github.com/WordPress/wordpress-develop/pull/7486/commits/c13a3989c338568cfd08047f20bb32a4e7481569 looked a little simpler to me. Not a blocker at all, but I was just trying to wrap my head around whether that approach would have been viable instead of adding the $include_node_paths_only option. I don't have any objections to the approach here, it overall sounds reasonable to me — this was really just a comment because we now have three checks for if ( $include_node_paths_only ) and if the condition ($include_node_paths_only) was only used in one place, I wasn't too sure how necessary it was.

But these questions were only asked out of curiosity / to confidence check the approach. Please don't let my comments here hold up this performance improvement!

The original commit in ​c13a398 looked a little simpler to me. Not a blocker at all, but I was just trying to wrap my head around whether that approach would have been viable instead of adding the $include_node_paths_only option. I don't have any objections to the approach here, it overall sounds reasonable to me — this was really just a comment because we now have three checks for if ( $include_node_paths_only ) and if the condition ($include_node_paths_only) was only used in one place, I wasn't too sure how necessary it was.

Thank you for clarifying. I think the three checks make sense, because $include_node_paths_only should still mean only that - you want only the paths for every node, but you still want _every_ node. There was originally only one check because it was excluding element nodes, but that would be unexpected behavior. In that case, the parameter would be more like $include_node_paths_only_and_only_for_block_nodes.

If we wanted to only get this data for block nodes, we could consider another parameter like you had originally proposed, but that would not really simplify the PR code, and it would introduce another parameter which we don't really know whether we need it. Hope that makes sense.

Thanks for explaining!

If we wanted to only get this data for block nodes, we could consider another parameter like you had originally proposed

Just to clarify, I wasn't meaning to advocate for another parameter — the original commit didn't include any as it skipped calling get_styles_block_nodes altogether and was iterating over $incoming_data. From reading the comments here, I was idly wondering if we could get away with not adding any parameters. But it was just a thought, and nothing worth blocking the good work here. The added parameter is clear and the conditions read well to me 🙂

Thank you for working on this, folks.

Not sure if it helps, but my understanding is that background images aren't supported for element styles yet (i.e. they're only supported for blocks themselves).

This is correct. I don't see any argument to support them until we support them 😄

This is a new feature in 6.7 so I assume backwards compatibility is not on the table.

the original commit didn't include any as it skipped calling get_styles_block_nodes altogether and was iterating over $incoming_data.

I think iterating over $incoming_data is a perfectly fine approach - it's small, targeted and doesn't introduce extra changes to get_block_nodes().

Having said that, I don't want to be a blocker either. The parameter addition is not a huge footprint. I just think this PR needs unit tests to support it.

Will there be unit tests for the changes in this method?

If it helps, I've added one to cover the new option in the Gutenberg backport PR, if you want to port it over:

• ​https://github.com/WordPress/gutenberg/pull/66002

Now that I've had time to reflect, I think the new option has merit as it will future-proof background image support for elements et. al.

Thanks again to all!

@felixarntz @ramonjd The feedback is addressed and PR is ready for commit.

Looking good to me. If no-one gets to it first, I can retest and commit tomorrow (I'm end of day).

@joemcgill There's already a Gutenberg backport PR, see ​https://github.com/WordPress/gutenberg/pull/66002.

[flixos90]

In 59213:

Themes: Improve performance of applying background image styles in theme.json.

The cost of using WP_Theme_JSON::get_block_nodes() for this in its original shape was high enough to lead to a performance regression. Therefore this changeset introduces a new option on the method that allows to bypass all logic except for retrieving the node paths, which is much faster and everything that this functionality needs.

Follow up to [58936].

Props mukesh27, flixos90, ramonopoly, joemcgill, andrewserong, swissspidy.
Fixes #61858.

Committed in https://core.trac.wordpress.org/changeset/59213