Background image: add support for relative theme path URLs in top-level theme.json styles #61271
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
commented
May 1, 2024
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. |
55 tasks
ramonjd
commented
May 1, 2024
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 92a5fe8. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/9143693322
|
ramonjd
force-pushed
the
add/background-images-top-level-resolved-theme-paths
branch
from
b641060
to
9285a43
Compare
ramonjd
changed the title
andrewserong
reviewed
May 6, 2024
andrewserong
reviewed
May 6, 2024
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 |
Updating comments
Rename utils file
ramonjd
force-pushed
the
add/background-images-top-level-resolved-theme-paths
branch
from
92a5fe8
to
5d3ba0b
Compare
Added an issue for this: |
|
ramonjd
added a commit
to ramonjd/wordpress-develop
that referenced
this pull request
May 22, 2024
ramonjd
added a commit
to ramonjd/wordpress-develop
that referenced
this pull request
May 28, 2024
ramonjd
added a commit
to ramonjd/wordpress-develop
that referenced
this pull request
May 28, 2024
ramonjd
added a commit
to ramonjd/wordpress-develop
that referenced
this pull request
May 29, 2024
ramonjd
added
the
Needs Dev Note
ramonjd
added
has dev note
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Dev Note 📓
#59354 (comment)
What?
This PR resolves relative paths in top-level
background.backgroundImage.urlvalues in theme.json and theme variation JSON files using get_theme_file_uri.Part of:
Based on explorations in:
Related trac ticket:
Note
This PR doesn't enable theme relative path resolution for blocks in templates/patterns. That'll be a follow up.
Why?
To ensure that themes with background image values in theme.json are portable — a theme developer can set a relative path to an image asset as a background image value, and that path will resolve correctly to absolute path of the file in the theme directory no matter where the theme is installed.
Contrast this with absolute image paths: every installation of the theme would point to single image files sitting somewhere on the internet.
How?
Paths to images residing in the theme directory can be set in theme.son using the
file:syntax, a convention already used in Core for web fonts.The path is relative to the theme root. For example:
For the frontend:
Relative paths will be resolved when building the global styles sheet in
gutenberg_get_global_stylesheet().A new public method
WP_Theme_JSON_Resolver_Gutenberg::resolve_theme_file_urisaccepts aWP_Theme_JSON_Gutenbergobject and will resolves anyfile:paths it finds using get_theme_file_uri.For the editor:
Global styles are built using merged theme, core and user theme.json configs in the block editor. All this is done using Javascript.
So that the editor has access to the resolved paths before building editor global styles, Gutenberg resolves the file paths in the backend and passes them to the browser via the
_linksobjects in global styles controller REST responses (Global styles and Global styles revisions controllers).Global styles are built using the
useGlobalStylesOutputWithConfighook. This PR adds a mutating function to that hook that replaces the relative paths with their absolute equivalents using the data in the_linksobject.This approach ensures that the values are used only when building and outputting styles, and not when saving or manipulating the global styles config, thereby preventing any resolved, absolute paths from being stored in the database.
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 or two to your theme directory.
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": "file:./img/untitled.png" } } } }Check for regressions: absolute paths should still 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?!