Background image: add support for absolute theme path URLs #60578
Conversation
lib/block-supports/background.php
Outdated
| /* | ||
| * "theme" source implies relative path to the theme directory | ||
| */ | ||
| if ( ! empty( $background_styles['backgroundImage']['url'] ) && 'theme' === $background_image_source ) { |
There was a problem hiding this comment.
This will take care of rendering block and global styles on the frontend.
| return { | ||
| allowRightClickOverrides: get( | ||
| 'core', | ||
| 'allowRightClickOverrides' | ||
| ), | ||
| blockTypes: getBlockTypes(), | ||
| currentTheme: { |
There was a problem hiding this comment.
Here's where my lack of imagination or sleep failed me. I couldn't think of another way, though there probably is one, to ensure the current theme data is available in the editor context.
| @@ -369,6 +388,12 @@ const elementTypes = [ | |||
| ]; | |||
|
|
|||
| function useBlockProps( { name, style } ) { | |||
| const { themeDirURI } = useSelect( ( select ) => { | |||
There was a problem hiding this comment.
Not sure about this either.
For the record, this PR should just take care of theme.json/global styles but it would be good to know in advance how we'll deal with blocks.
These blocks could come from template or pattern files as well.
|
Size Change: +543 B (+0.03%) Total Size: 1.74 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Thanks for digging in! Hope you don't mind, but looking over the code change sparked a couple of thoughts, so I thought I'd jot them down here in case it's helpful 🧠 📔
Is there another way to get the theme directory to the editor?
Great question. Also, to further complicate matters, in PHP the get_theme_file_uri() function will resolve files from a child theme (i.e. checks if the file exists in the child theme, and if not, moves up to the parent theme to see if the file exists, and then it also supports the theme_file_uri filter). This presents some challenges for the editor as we can't call get_theme_file_uri() with each individual file that might be in a template or theme.json.
A couple of off-the-top-of-my-head thoughts:
- If we don't support resolving between child and parent themes, then if someone goes to create a child theme from a theme that includes background images in
theme.jsonor patterns, then the background images will likely be broken in the child theme. - One potential workaround could be to also output
get_template_directory_uri()in the theme API endpoint? - Or, a more complex but potentially explicit approach could be: what if it were possible to register or declare theme assets that are available within a theme? I.e. either in
theme.jsonand/or via a PHP function? Then we could have an endpoint that returns the URLs for each of the registered assets.
That last point could be a "good" way to handle it explicitly, but even as I write that, I can't imagine it being at all simple to come up with a reliable way to do it that everyone would agree to. So, another potential idea:
What if instead of extending the theme API endpoint, we injected the theme URI (and potentially the template / parent theme URI) into block editor settings via the 'block_editor_settings_all' hook? That way we'd be hacking the URIs into block settings without touching any API endpoints?
|
Thanks for looking at this one @andrewserong! 🙇🏻
Yeah - I used
Definitely more pragmatic, given we're touching settings anyway. I like!
This would be more bullet proof, but I agree that it might be a larger feature that would require wider buy in. Also it introduces another registration layer for folks to think about. An MVP could be |
Sounds worth trying to me! Then, once things are working well enough in both the editor and server-side, we could ping Gutenberg core folks for more visibility to confidence check the approach? |
lib/block-editor-settings.php
Outdated
| @@ -76,6 +76,13 @@ function gutenberg_get_block_editor_settings( $settings ) { | |||
| } | |||
| } | |||
|
|
|||
| // Could house also `__unstableIsBlockBasedTheme` as 'isBlockTheme' => $current_theme->is_block_theme(). | |||
| $current_theme = wp_get_theme(); | |||
There was a problem hiding this comment.
Using wp_get_theme to get full WP_Theme object in case folks want to use other helpful methods.
ramonjd
force-pushed
the
add/background-image-theme-source
branch
from
d19c285
to
a66c5e7
Compare
ramonjd
force-pushed
the
add/background-image-theme-source
branch
from
a66c5e7
to
f7bf075
Compare
| * Only applies to background styles for now. | ||
| */ | ||
| if ( !! blockStyles?.background ) { | ||
| blockStyles = setBackgroundStyleDefaults( blockStyles, { |
ramonjd
force-pushed
the
add/background-image-theme-source
branch
from
de024f1
to
16e830b
Compare
| _backgroundImage?.source === 'file' && | ||
| _backgroundImage?.url | ||
| ) { | ||
| if ( typeof _backgroundImage === 'object' && _backgroundImage?.url ) { |
There was a problem hiding this comment.
Removing _backgroundImage?.source === 'file' check as we only care if background image is an object. There's no special processing for file or theme in the style engine.
| * Only applies to background styles for now. | ||
| */ | ||
| if ( !! style.background ) { | ||
| style = setBackgroundStyleDefaults( style, editorSettings ); |
There was a problem hiding this comment.
The rationale is that some style values need to have sensible defaults that are output as CSS, but not necessarily part of the styles attribute.
setBackgroundStyleDefaults could one day be setStylesDefaults or something.
This could theoretically be done in the style engine if we wanted to make the style engine opinionated.
ramonjd
changed the title
ramonjd
requested review from
tellthemachines,
spacedmonkey and
ellatrix
as code owners
|
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
added
[Feature] Themes
| @@ -1230,6 +1246,7 @@ export function useGlobalStylesOutputWithConfig( mergedConfig = {} ) { | |||
| disableLayoutStyles, | |||
| isTemplate | |||
| ); | |||
| console.log( 'globalStyles', globalStyles ); | |||
There was a problem hiding this comment.
This is a marker to remind me that getThemeFileURI isn't implemented for global styles yet either.
Maybe it needs to be done by injecting a CSS rule into the DOM post-render, something like layout does.
I don't know. This is getting out of hand. 😄
| style: { | ||
| // @TODO this should be backgroundImage. How to do that? | ||
| // Also, maybe consider reinstating https://github.com/WordPress/gutenberg/blob/fc98542a7dbba194bb4096d49cd0bd093b63f43e/packages/block-editor/src/hooks/background.js#L82 | ||
| background: `url( '${ encodeURI( |
There was a problem hiding this comment.
Quick question for @noisysocks or @ellatrix
How can I update the value of a style property via useBlockProps or another hook?
I want to overwrite backgroundImage, but I'm guessing the useBlockProps won't overwrite already-set props, only add new ones.... 🤔
Thank you! 🙇🏻
There was a problem hiding this comment.
Oh never mind 😆
I finally worked it out it's the filter order:
| @@ -896,3 +896,34 @@ export const getRevision = | |||
| dispatch.receiveRevisions( kind, name, recordKey, record, query ); | |||
| } | |||
| }; | |||
|
|
|||
| export const getThemeFileURI = | |||
There was a problem hiding this comment.
This is all highly speculative and experimental. Don't worry folks! 😅
@andrewserong is this closer to what you were thinking?
It kinda works, but it's proving difficult to use for block props, and no idea yet how to implement for global styles.
Considering generating some temp CSS rules and injecting into the DOM 🙃
See comment: https://github.com/WordPress/gutenberg/pull/60578/files#r1584350221
There was a problem hiding this comment.
That was the general idea I had in mind, yes, thanks for exploring it! It's a challenging one, though, as it is a bit of a messy way to handle things, but I do like the idea of us having a JS equivalent to the PHP logic, and I like how you've gone for a generic approach that could be used for things other than just the background image support 👍
I think featured images should be bound to background-url via block bindings, not a concern of the style engine? In the UI of the editor we'll have to have a distinct way to indicate this binding, unlike static files (relative or absolute) which use the normal styling UI.
When would that be the case? I mean if the background is from uploads we should maybe use the full URL? |
|
Thanks for continuing the discussion!
To clarify, the style engine won't be doing any of the block-editor-specifics of resolving background image paths, despite the chaotic nature of this PR's commits (all experimental). Totally agree that it should occur at the block level. Looking at that now for this PR.
It's pretty edge case, but it might be useful to have relative paths to uploads for testing across dev/prod environments where the installations are mirrored but the domain names are different? I dunno. 😄 Though possible, I'd wager it'd be detrimental to portability for a theme to add absolute paths to uploaded files in template/pattern/theme.json files. Back to what this PR is trying to do, if we assume relative paths are relative to the theme, I still think that makes sense for theme assets (template/pattern/theme.json files). It doesn't have to mean we can't introduce the "source" prop later. |
What would that look like in practice? I was imagining that for the background image support, it'd feel a bit more "first party" and be a canonical part of how the block support works, so it'd be exposed in a similar way as it is in the Cover block, but as part of the 
My wording was likely off, but my point was more about the value of a
I think that's a pragmatic path forward. We still need |
- get the theme directory URI, and - ensure background styles are parsed for theme.json and pattern/template files
…heme` both parent and child dir Update tests to check for image paths with leading slash Revert rogue tab
…ll url-like paths. Revert rogue space
Added/updated unit tests Bad horsie
…mage on the frontend.
…_file_uri (would be extracted into a new PR) Haven't worked out global styles yet. CSS rules might need to be injected into the DOM???
Feature complete, but hacky
ramonjd
force-pushed
the
add/background-image-theme-source
branch
from
30f4fed
to
1aa8f6a
Compare
|
Thanks for sticking with me on this one, folks. StatusThis PR, though hacky, is feature complete:
Exceptions
Path forward for WordPress 6.6 🤞🏻
Beyond WordPress 6.6
|
|
Closing this PR as it'll be used as a reference for future PRs See: #60578 (comment) |
|
Forced push and didn't realised this PR had been closed 🙃 So moving experiment to |
Moved to #61401
Note
This PR is experimental and will be a reference for ongoing work. See #60578 (comment)
What? And How??!!!
Part of:
This PR proposes a way to add support for theme relative url paths for theme.json/templates/patterns by:
"source": "theme"on the background image object in background supports, and appending the current theme path. It usesget_stylesheet_directory_uriinstead ofget_template_directory_uriso that the path will reflect any active child theme.currentTheme, that contains the current theme directory path. It also usesget_stylesheet_directory_uriinstead ofget_template_directory_uriso that the path will reflect any active child theme.Why?
So that themes don't have to use global styles and rather, insert theme-relative paths into templates, patterns and theme.json.
This enhances portability of themes as Gutenberg will use the theme's current directory, regardless of install.
Testing Instructions
Assuming that
img/Untitled.jpgexists in the theme dir, here's some test theme.json. Note only top-level background images are possible until #60100 gets in.{ "$schema": "https://schemas.wp.org/trunk/theme.json", "version": 2, "settings": { "appearanceTools": true }, "styles": { "background": { "backgroundImage": { "source": "theme", "url": "img/Untitled.jpg" }, "backgroundPosition": "center center", "backgroundRepeat": "no-repeat", "backgroundSize": "cover" } } }Here's a test pattern (could also be HTML). Add it under
/patternsin the theme directory:It should also work with templates. Add it under
/templatesin the theme directory:background-imageCSS value for both the frontend and editor.Testing Instructions for Keyboard