Background images: add support for theme.json ref value resolution #64128
Conversation
ramonjd
added
[Type] Enhancement
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
ramonjd
force-pushed
the
update/background-image-ref-support
branch
from
1f3fe7c
to
aa6bb1d
Compare
| $ref_value = _wp_array_get( $theme_json, $value_path ); | ||
| $ref_value = _wp_array_get( $theme_json, $value_path, null ); | ||
| // Background Image refs can refer to a string or an array containing a URL string. | ||
| $ref_value_url = $ref_value['url'] ?? null; |
There was a problem hiding this comment.
This is specifically for background images.
It's part of the reason why I think this could be a style engine util, used both internally, and externally by WP_Theme_JSON.
The style engine could subsume many style-related utils and potentially reduce the site of WP_Theme_JSON.
There was a problem hiding this comment.
Anything that can chip away at the size of WP_Theme_JSON sounds like a win to me.
packages/block-editor/src/components/global-styles/use-global-styles-output.js
Outdated
Show resolved
Hide resolved
|
Size Change: -62 B (0%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Nice work! I've only taken it for a quick spin so far, but can give this some more testing tomorrow. It's mostly testing well for me 👍
I noticed that it looks like the default of cover background size, and the thumbnail for the image in the UI control don't appear to be included when using the ref-based value. Are you able to replicate?
Tiny bit of theme.json snippet:
"core/quote": {
"background": {
"backgroundImage": {
"ref": "styles.background.backgroundImage"
}
},
Overall I like your idea of abstracting things away in the style engine in the future. For now, though, it doesn't look like this has added too much code so far, so seems a reasonable approach to me! But curious to hear what other folks think, too 🙂
packages/block-editor/src/components/global-styles/use-global-styles-output.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/global-styles/use-global-styles-output.js
Outdated
Show resolved
Hide resolved
|
I think I just noticed another issue: I wasn't able to update a block in global styles with a user set image. The 
|
|
Thanks for testing @andrewserong I'll give it belt tomorrow and straighten things out. |
Very interesting. So I think we never output defaults at the block level in theme.json - I'll have to fix that in general. As for the thumbnail, we're passing All fixable! Thanks again for testing 🙇🏻 |
Oh, good point! I can't remember the discussion now... maybe we decided that we'd only set the default when a user uploads versus when someone is using |
I think that was the intention, looking at the commit at the time. I can't find any specific conversation though. Adding defaults only to user-uploaded backgrounds makes sense to me - I'll test it through. |
ramonjd
force-pushed
the
update/background-image-ref-support
branch
from
2f17492
to
6f2a83e
Compare
| @@ -334,6 +340,7 @@ function BackgroundImageControls( { | |||
| ! positionValue && ( 'auto' === sizeValue || ! sizeValue ) | |||
| ? '50% 0' | |||
| : positionValue, | |||
| backgroundSize: sizeValue, | |||
There was a problem hiding this comment.
Uploaded images will receive any incoming default values.
For example, hooks/background.js defines "cover" - this will affect blocks only.
| } | ||
| ); | ||
| return resolvedValues; | ||
| }, [ globalStyles, inheritedValue ] ); |
There was a problem hiding this comment.
Here I'm consolidating all the data and resolution logic.
That now includes themeURIs for relative URIs, which really only affects background images.
It's a first step towards linking globalStyles with all styles controls, at least for one panel.
Where themeURIs have been passed around as props before, I've removed it.
packages/block-editor/src/components/global-styles/use-global-styles-output.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/global-styles/use-global-styles-output.js
Outdated
Show resolved
Hide resolved
|
This PR does too much:
I'll split it up (in order of merge priority):
|
packages/block-editor/src/components/global-styles/use-global-styles-output.js
Outdated
Show resolved
Hide resolved
Sounds great, I like the direction of the refactors in this PR so far!
Has that been addressed? When re-testing, it looks like I'm still seeing an "Add background image" button at the block level in global styles, and in the block inspector when selecting a block instance. Just wanted to double-check in case I've messed something up in my testing locally 🙂 
|
|
Update: in my screenshot above, my root 
|
Ah, that would be it 👍🏻 That was a concession we made a few cycles ago, that is, not to try to prioritize parsing out the image URI from such string values because those values could be anything, not just I suppose we could do a simple regex and URI sanitization in a follow up. I'll add it to the tracking issue as an enhancement. |
Yeah, not a blocker at all! I wouldn't worry about it for now, just glad we got to the bottom of it 😄 |
ramonjd
force-pushed
the
update/background-image-ref-support
branch
from
008b2f4
to
ec668b8
Compare
| $ref_value = _wp_array_get( $theme_json, $value_path ); | ||
| $ref_value = _wp_array_get( $theme_json, $value_path, null ); | ||
| // Background Image refs can refer to a string or an array containing a URL string. | ||
| $ref_value_url = $ref_value['url'] ?? null; |
There was a problem hiding this comment.
Anything that can chip away at the size of WP_Theme_JSON sounds like a win to me.
There was a problem hiding this comment.
Thanks for the explanatory comments and detailed PR description @ramonjd 👍
I've followed the test instructions, including those in #64128 (comment), and it is working pretty well for me.
I think I'm missing some context around then removal of the _links data and theme file uri stuff. I think it makes sense. Was this part of what you were planning to split into separate PRs?
On the code side of things, I noticed a few minor issues that I'll push commits for shortly. Feel free to revert anything you disagree with 🙂
Once the dust settles, I'll give it another proper test tomorrow.
|
Huge thanks for testing this one and pushing those changes @aaronrobertshaw LGTM
I was pushing the _links data around from component to component, when I really only needed it in the background-panel component. The _links data is used to resolve any relative paths. Most of the ground work for this was already done in #64182, so this PR is mainly putting all the resolution operations required for background panels in one place.
It's kinda related. My idea was to abstract what the background panel is doing in terms of resolution so, eventually other styles could take advantage. It was a bit of forward work for the following issue: So, not really required right now, and I think it makes more sense to do afterwards. I do see a couple of follow ups to this PR for the very immediate or for the longer term scope. In opinionated order of least complex to most complex:
|
ramonjd
force-pushed
the
update/background-image-ref-support
branch
from
aaf0cc9
to
f6f7f7a
Compare
There was a problem hiding this comment.
Nice work wrangling all this @ramonjd ✨
Special thanks for the detailed PR description and extra explanations to help get me up to speed here.
This tests well for me across the board.
✅ Theme.json set background styles apply correctly in both editors and frontend
✅ Background ref values resolve correctly
✅ Updating Global Styles overrides background image values, including ref values
✅ Updating Global Styles background image that is the source for another block's ref value does get reflected on the block referencing it
✅ Global Styles changes apply consistently across editors and frontend
✅ Block instance styles override Global Styles correctly
LGTM 🚢
There was a problem hiding this comment.
Thanks for the PR!
I tested it but noticed something I can't explain. I'm not sure if there's anything I'm doing wrong or this is not even related to this PR?
When I set the global styles background for core/quote to a custom image like shown below, the quote block is still referencing the background image.
However, the frontend is correctly showing the custom image I uploaded.
The block itself doesn't have per-block style.
You can also find the inconsistency on the group block above 🤔. I'm not too familiar with the feature and the code base so I could use some sanity check here 🙇.
|
Thanks for testing @kevin940726 and @aaronrobertshaw 🙇🏻
I can't seem to reproduce 🤔 I checked in Firefox, Chrome and Safari. Kapture.2024-08-15.at.10.30.35.mp4I recognize the behavior, as it was the reason behind this change. When uploading an image there's a slight delay I see, but that's because we're waiting on the upload callback to fire. Which makes me wonder if we should have some loading UI there to indicate that an image is being uploaded, like the image block has 🤔 I'll add it as a follow up 👍🏻 |
- add tests - adds tests Check for string types in background image so it will apply defaults for blocks. Update comments
- remove useGlobalStyleLinks completely - delete theme URI utils Remove string condition LInto
… exists. This will occur when base and user configs are merged. Update tests and perform check for background size before use. Update tests and perform check for background size before use.
… merged, but replaced when merged theme.json configs. So, for example, an incoming config such as a saved user config should overwrite the background styles of the base theme.json, where it exists.
…und styles as general styles are tested above in test_merge_incoming_data
ramonjd
force-pushed
the
update/background-image-ref-support
branch
from
f6f7f7a
to
f21a3e9
Compare
|
I'm also having trouble replicating this issue (#64128 (review)). The global styles background image for the quote block gets applied correctly for me. I tried a few approaches and all worked well:
A loading UI as a follow-up sounds like a nice touch to me 👍 
|
There was a problem hiding this comment.
Well, I certainly can't reproduce #64128 (review) either 🤦. I'm guessing an outdated or branch locally on my end. Sorry for the false alarm!
In other words, this works well in my testing! Thanks!
What?
Part of:
So, you know how you can "reference" another value in theme.json using
"ref"?Well, this PR adds support for ref resolution to
"background"style properties.Why?
The
"background"style properties haven't supported this feature, until now!The theme.json schema has already been updated to reflect this change.
How?
By passing the resolved background image value to the style engine.
In the client (editor JS), I'm doing it long-hand, as you can see. It's a bit fugly. In both the frontend (PHP) and editor, the style engine does the work of escaping
background-imagevalues and also ensuring they're wrapped inurl().The
WP_Theme_JSONclass resolves values before passing to the style engine, however the editor does it the other way around.However, the style engine does not perform any "ref" resolution. And since the work of escaping and wrapping in
url()needs to happen after ref resolution, we need to make an exception for background image.Alternatives
We add logic to
WP_Theme_JSONanduse-global-styles-output.jsto sanitize/buildurl()values and bypass the style engine forbackground-imagevalues. Involves duplicating code.The style engine could handle ref resolution (and possibly defaults) via a public API, used both internally and externally. Theme.json tree and defaults could be passed as options.
Example (not prescriptive):
Testing Instructions
Example block HTML
Example theme.json
{ "$schema": "../../schemas/json/theme.json", "version": 3, "settings": { "appearanceTools": true, "layout": { "contentSize": "840px", "wideSize": "1100px" } }, "styles": { "background": { "backgroundImage": { "url": "https://images.pexels.com/photos/105819/pexels-photo-105819.jpeg?auto=compress&cs=tinysrgb&w=1260&h=750&dpr=" }, "backgroundSize": "cover", "backgroundAttachment": "fixed", "backgroundPosition": "top left" }, "blocks": { "core/verse": { "background": { "backgroundImage": { "url": "file:./img/1024x768_emptytheme_test_image.jpg" }, "backgroundAttachment": "fixed", "backgroundSize": "61px" }, "color": { "text": "white" }, "dimensions": { "minHeight": "100px" } }, "core/quote": { "background": { "backgroundImage": { "ref": "styles.background.backgroundImage" } }, "dimensions": { "minHeight": "100px" } }, "core/group": { "background": { "backgroundImage": { "ref": "styles.blocks.core/verse.background.backgroundImage" }, "backgroundAttachment": { "ref": "styles.blocks.core/verse.background.backgroundAttachment" }, "backgroundSize": { "ref": "styles.blocks.core/verse.background.backgroundSize" }, "backgroundPosition": { "ref": "styles.background.backgroundPosition" } }, "color": { "text": { "ref": "styles.blocks.core/verse.color.text" } }, "dimensions": { "minHeight": "111px" } } } } }TODO
Split this PR up into (in order of merge priority):
getCSSVarFromStyleValuefrom style engine in place ofcompileStyleValue(new PR) Style engine: export util to compile CSS custom var from preset string #64490