[WIP] Background image: add support for relative theme path URLs in top-level theme.json styles #61271
Conversation
ramonjd
added
[Status] In Progress
|
Size Change: +723 B (+0.04%) Total Size: 1.74 MB
ℹ️ View Unchanged
|
ramonjd
force-pushed
the
add/background-images-top-level-resolved-theme-paths
branch
from
8840474
to
5edf884
Compare
ramonjd
requested review from
draganescu,
scruffian,
noisysocks,
justintadlock,
tellthemachines and
andrewserong
|
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. |
There was a problem hiding this comment.
Testing shows this working well! I'm able to reference a theme image from theme.json, and replace it with an uploaded on through the UI.
One small bit of weirdness I noticed is that, having set the image through theme.json, in the UI I can see the "reset" option, which does nothing when clicked:
Oh yeah, nice spotting. It won't "reset" to nothing as it it's theme.json defined. But if you set a user-defined image it should "reset" back to the theme.json-defined on. But the fact you can click reset at all doesn't sound right, I'll look into it. Thanks!! |
Handling this in a bug fix PR. Thanks again for reporting it. |
|
Flaky tests detected in 2133e80. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/9013064066
|
ramonjd
force-pushed
the
add/background-images-top-level-resolved-theme-paths
branch
from
b641060
to
9285a43
Compare
ramonjd
changed the title
| * Styles backgrounds. | ||
| * Where a URL is not absolute (has no host fragment), it is assumed to be relative to the theme directory. | ||
| * Blocks, elements, and block variations are not yet supported. | ||
| */ | ||
| if ( | ||
| isset( $this->theme_json['styles']['background']['backgroundImage']['url'] ) && | ||
| is_string( $this->theme_json['styles']['background']['backgroundImage']['url'] ) && | ||
| ! isset( wp_parse_url( $this->theme_json['styles']['background']['backgroundImage']['url'] )['host'] ) ) { | ||
| _wp_array_set( | ||
| $this->theme_json, | ||
| array( 'styles', 'background', 'backgroundImage', 'url' ), | ||
| esc_url( get_theme_file_uri( $this->theme_json['styles']['background']['backgroundImage']['url'] ) ) | ||
| ); | ||
| } |
There was a problem hiding this comment.
Just wondering, if we're adding this logic to the theme JSON class, if and when we go to support referencing theme assets (e.g. background images) at the block level within patterns, will we need to duplicate this logic in background.php (or have similar logic over there)?
I guess to put the question another way — is it worth abstracting into a function in the background image support at this stage, or leave it inline if we know we're going to be using the resolve_theme_file_uris() function anyway, and the innards can be moved elsewhere further down the track?
From my perspective, looking at the scope of this PR, if we know we're happy with the resolve_theme_file_uris() function and signature, then it sounds reasonable to move things around in follow-ups to me.
There was a problem hiding this comment.
if and when we go to support referencing theme assets (e.g. background images) at the block level within patterns, will we need to duplicate this logic in background.php (or have similar logic over there)?
Yeah we'll have to do that, so abstracting should be 👍🏻
What do you think? We'd move the logic into a background.php function and call it from theme.json? We do it with typography and duotone already so 🤷🏻 For global styles, ideally the transformation would occur in WP_Theme_JSON_Resolver::get_merged_data as it is in this PR, so it's "resolved" everywhere.
I can reinstate this function (again) 😄
There was a problem hiding this comment.
We'd move the logic into a background.php function and call it from theme.json?
Sorry, typing too fast. I wanted to also write, "or, call the block supports function directly in P_Theme_JSON_Resolver::get_merged_data
Actually, scratch that. We need to work with the current theme_json so it's probably best suited to be called from theme_json, e.g.,
// in class-wp-theme-json-gutenberg.php
function public resolve_theme_file_uris() {
$this->theme_json['styles'] = gutenberg_get_background_support_styles( $this->theme_json['styles'] );
}Or whatever
There was a problem hiding this comment.
What do you think? We'd move the logic into a background.php function and call it from theme.json?
Yeah, the example in your comment above looks pretty good to me — keeps the theme.json class quite light, and parallels the approach the typography support uses as you mention. IMO I like the structure where the block supports have functions for their own logic and the theme json calls out to them when needed, but that could just be my own preference!
There was a problem hiding this comment.
This is testing pretty well for me at the root theme.json level. One subtle thing I noticed in the UI is that it doesn't expose the size controls by default when the theme.json sets an image. Should it, do you think?
I'm also wondering (not for this PR) if now that we're supporting a theme setting a background image, if we should have none as an available option, perhaps in the dropdown where Reset and Open Media Library live, so that folks could explicitly remove a background image if a theme sets one?
In terms of code, resolving the theme uris before the site editor loads is a clever way to ensure we don't need to worry about requesting the URL via the site editor. A couple of questions:
- I wasn't able to get a theme style variation to work by giving it its own different background image. Should that be possible at this stage? I could very possibly be making a mistake in my local testing environment.
- What happens in the database if we go to save changes, or apply a variation? Does the resolved version of the URL get saved instead of the theme relative one? If so, I wonder if there's a way to preserve the reference to the theme URL instead of "baking" in the URL in the user global styles data 🤔
|
Thanks for testing and the great questions @andrewserong
Very doable, and it should, I agree. It should be a one liner. I'll add it in another PR 👍🏻
Oh, yeah good idea. That sounds like a useful thing to have. I'll add a follow up task to the tracking issue
Variations aren't supported currently in this PR only the
I'll check, but I'm going to guess "yes", the resolved path is saved. Just thinking out loud... the path resolution in that case should happen when building styles, not merging theme.json data. It's therefore likely we'd need need some of the core data selectors created in #60578 because styles are built in the block editor via javascript from theme.json tree, and we'd need a way to tell if the file belongs to the child/parent theme. As for the frontend, we could just build the path resolution into |
No, no, it's not you! It's such a nuanced and tricky thing to discuss and my focus has been split, so I'm still playing catch-up a little with this feature 🙂
Perfect, I agree! Let's leave those nested / block-level-in-global-styles situations with theme relative urls to follow-ups (i.e. potentially 6.7) or nice-to-haves 👍 |
ramonjd
force-pushed
the
add/background-images-top-level-resolved-theme-paths
branch
from
97c7d2b
to
444bbc6
Compare
Firstly, apologies for the long read. Trying to satisfy present and future requirements here has been my main preoccupation this week. I've concluded that, in order to ensure that relative paths are preserved in theme json (for portability, mainly), relative paths must be dynamically resolved when generating stylesheets. Why all the fuss? Preserving relative theme paths is desirable when exporting a theme from the site editor. The paths should not be absolute in the export files. Furthermore, but not so important in my opinion, it will prevent 404s from absolute paths in already-saved user config (the post content in the database) should a website's theme directory or host ever change (even though uploaded images in post content suffer the same risk). Weighing the benefits and other competing requirements, I haven't been able to find a performant way to support clientside theme file resolution for global styles. #61401 showed some promise in this regard - it works, kinda, but I'm not confident it will scale well with theme.json files that contain multiple relative file paths in various locations in the tree. More so because the large However, resolving these URIs is relatively trivial on the backend thanks to get_theme_file_uri. That's what I've started in 32906b9, whose approach is to send resolved URIs in the All the frontend requires is a transformation on The usual caveats apply given the recency of this proposal. I'll ping more widely soon to discuss the immediate need and alternative approaches to this whole enterprise. Thank you! |
ramonjd
force-pushed
the
add/background-images-top-level-resolved-theme-paths
branch
from
32906b9
to
2133e80
Compare
ramonjd
changed the title
ramonjd
force-pushed
the
add/background-images-top-level-resolved-theme-paths
branch
2 times, most recently
from
b5be41f
to
a3bda72
Compare
| 'self' => array( | ||
| 'href' => rest_url( sprintf( '%s/%s/themes/%s', $this->namespace, $this->rest_base, $request['stylesheet'] ) ), | ||
| ), | ||
| ); | ||
| $resolved_theme_uris = WP_Theme_JSON_Resolver_Gutenberg::get_resolved_theme_uris( $theme ); | ||
| if ( ! empty( $resolved_theme_uris ) ) { | ||
| $links['theme_file_uris'] = $resolved_theme_uris; |
There was a problem hiding this comment.
Why _links?
Well, I don't think resolved URIs belong in the theme.json schema because they're related to the environment rather than the theme specifically.
Hypertext Application Language (HAL) is quite flexible in its structure, allowing us to represent links to multiple resources or related resources using arrays.
|
Size Change: -1.06 kB (-0.06%) Total Size: 1.75 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Functionality-wise, this is working really well! Very promising, and it's generally testing well for me with style variations, too, where one of the variations sets a different background image.
Edit: when I first tested this, I thought I found a bug with restoring revisions. It turns out I had forgotten the very necessary step of hitting "save" after applying a revision! 😄🤦
To test with variations, I used TwentyTwentyFour since it already includes some images in its assets directory, so it was pretty quick to add the following:
In styles/ember.json I added the following:
"styles": {
"background": {
"backgroundImage": {
"url": "assets/images/museum.webp"
}
},
And in the root theme.json file I added the following:
"styles": {
"background": {
"backgroundImage": {
"url": "assets/images/abstract-geometric-art.webp"
}
},
In terms of the API design here, I really like the approach of injecting into _links, I think it's a good pragmatic path forward, and resolving on the server with the URLs available and matched to their relative paths, feels quite clever to me. I'm not very familiar with the API standards used in the REST API and what's expected in _links beyond what's mentioned in this page in the docs: https://developer.wordpress.org/rest-api/using-the-rest-api/linking-and-embedding/ — would it be worth pinging in the WP Slack core-restapi channel for feedback, or if this is the preferred way forward, opening a trac ticket for the REST API component, to gather more input there?
In practice it's been testing nicely for me. E.g. logging out responses from the variations, I see the relative path used in the styles:
And the resolved paths in the _links: 👍
|
I appreciate the test @andrewserong Apologies it's so huge - it's turning into another proof of concept PR, so it's ripe to be broke up once I get feedback from REST API folks.
Yeah, that was the next step. I wanted to make sure it would work at all and integrate into Gutenberg's existing CSS-generation hooks. I think it does, so I'll give those folks a ping.
Good find. I'll test this today and get back to you. If it's what I think it is, it should be relatively easy to fix. The honey in the sandwich here is ensuring that
I'll open the debate with REST folks to get some input. Here are some of my notes for reference when the discussion comes up: From what I've learned so far, the proposed spec is pretty flexible and allows for embedding related links to other resources. We're embedding URIs, which is perfectly valid. I see resolved image data as "child resources" of the theme JSON rest API. Now whether we utilize I wouldn't expect much dogmatism, aside from using Also, WordPress has a bunch of custom link relations, e.g., There are some reserved properties that could describe the resource better, so if we wanted to follow the proposed spec a little more closely, then it could be something like: "_links": {
"wp:theme-file-uris": [
{
"href": "http://resolve.uri/path/image.png",
"name": "path/image.png",
},
]
}The fact that they're reserved implies that they're not to be used exclusively. I'd also think about adding a new prop "_links": {
"wp:theme-file-uris": [
{
"href": "http://resolve.uri/path/image.png",
"name": "path/image.png",
"path": [ "style", "blocks", "core/group", "background", "backgroundImage" ]
},
]
}The least I'd do right now is use Edit: actually, I might need to rethink this as some endpoints return collections of global styles objects for example from the Thanks again! |
- removes requirement for a `source` property in theme.json as the assumption is that, for now, all paths are paths to image files, whether absolute or relative - checks for existence of "host" in URL and then tries to resolve background image url using get_theme_file_uri - Adds a new public method WP_Theme_JSON_Gutenberg::resolve_theme_file_uris to allow theme devs to optionally resolve relative paths in theme.json to a theme.
…nal? That is, done in the global themes controller and wherever a stylesheet is generated?
Backend resolution of theme file URIs for global styles revisions Ensuring links are preserved when updating global styles.
Always adding path to the link object so that it can dynamically resolved.
ramonjd
force-pushed
the
add/background-images-top-level-resolved-theme-paths
branch
from
668d8ac
to
2eb8219
Compare
Pinged in Core REST API Slack and also created new trac ticket: https://core.trac.wordpress.org/ticket/61205 👍🏻 |
Updated tests
|
This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress. If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged. If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack. Thank you! ❤️ View changed files❔ lib/compat/wordpress-6.6/class-gutenberg-rest-global-styles-revisions-controller-6-6.php ❔ lib/class-wp-rest-global-styles-controller-gutenberg.php ❔ lib/class-wp-theme-json-resolver-gutenberg.php ❔ lib/compat/wordpress-6.5/rest-api.php ❔ lib/compat/wordpress-6.6/rest-api.php ❔ lib/global-styles-and-settings.php ❔ lib/load.php ❔ phpunit/class-wp-theme-json-resolver-test.php |
| * returns a collection ([{}]) and not a response object ({}) it's not possible. | ||
| */ | ||
| if ( ! empty( $resolved_theme_uris ) ) { | ||
| $variation['_links']['wp:theme-file-uris'] = $resolved_theme_uris; |
There was a problem hiding this comment.
Looks like we should add this stuff to the header:
https://developer.wordpress.org/rest-api/using-the-rest-api/linking-and-embedding/
|
|
||
| // Add resolved URIs to the response. | ||
| $links = array(); | ||
| $resolved_theme_uris = WP_Theme_JSON_Resolver_Gutenberg::get_resolved_theme_uris( $theme_json ); |
There was a problem hiding this comment.
Looks like we should add this stuff to the header:
https://developer.wordpress.org/rest-api/using-the-rest-api/linking-and-embedding/
|
This is a flyby comment, so forgive me if I'm missing context. We already have the convention of using
|
🚧 🚧 🚧 🚧 🚧 🚧 🚧 🚧 🚧 🚧 🚧 🚧 🚧 🚧 🚧 🚧 🚧 🚧 🚧
🚧 Needs refinement, getting things to work first. 🚧
🚧 🚧 🚧 🚧 🚧 🚧 🚧 🚧 🚧 🚧 🚧 🚧 🚧 🚧 🚧 🚧 🚧 🚧 🚧
What?
Part of:
Based on explorations in:
Related trac ticket:
This PR resolves relative paths in top-level
background.backgroundImage.urlvalues in theme.json using get_theme_file_uri.The current approach
The current approach is to preserve relative paths when saving global styles to the database.
Relative paths needs to be resolved (using
get_theme_file_uri) when fetching theme.json for the purposes of building styles. Since global styles are built using JS in the block editor, Gutenberg needs to prepare them in the backend and pass them to the browser, e.g., in the REST response GET requests.Resolving URIs has to happen for the following assets:
"Do we have to return resolved URLs in the REST request? Can't the block editor do all clientside?" Well, yes. #61401 went most of the way in that direction, but to do so it was modifying global styles on the fly in an async hook that tried to emulate
get_file_theme_uriviawindow.fetchand cache the results. Given the dynamic nature of the global styles state, it's difficult to balance performance.Note
This PR doesn't enable theme relative path resolution for blocks in templates/patterns. That'll be a follow up.
Why?
This is the first step to ensure that themes with background image values in theme.json are portable, that is, a theme developer can set a relative path to the image asset (meaning a path to a file in a theme) and it will resolve correctly when the theme is installed (and the image is there).
How?
Removes requirement for aDone in Background images: remove required "file" prop #61387sourceproperty in theme.json as the assumption is that, for now, all paths are paths to image files, whether absolute or relativeget_theme_file_uriso that child themes can refer to parent-theme assets.WP_Theme_JSON_Resolver_Gutenberg::resolve_theme_file_uristo allow theme devs to optionally resolve relative paths in theme.json to a theme.Testing Instructions
Note
Background images won't appear in the theme.json when exporting themes from the site editor until this PR is merged #61561
Test by adding an image to your theme directory. Here I'm using
img/untitled.png.Also add a variation of this theme.json as a style variation (under
/stylesin the theme directory) with a different image.{ "$schema": "https://schemas.wp.org/trunk/theme.json", "version": 2, "settings": { "appearanceTools": true }, "styles": { "background": { "backgroundImage": { "url": "img/untitled.png" } } } }Ensure absolute paths work as expected as well:
{ "$schema": "https://schemas.wp.org/trunk/theme.json", "version": 2, "settings": { "appearanceTools": true }, "styles": { "background": { "backgroundImage": { "url": "https://images.pexels.com/photos/22484288/pexels-photo-22484288/free-photo-of-the-circular-stone-terraces-of-the-inca-ruins.jpeg" } } } }Run tests:
Screenshots
Do you love budgies?!