Block Supports: Add background image support to Group block #53934
Conversation
andrewserong
added
[Type] Enhancement
|
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/block-supports/background.php ❔ lib/class-wp-theme-json-gutenberg.php ❔ lib/load.php ❔ phpunit/style-engine/style-engine-test.php |
|
Size Change: +9.74 kB (+1%) Total Size: 1.53 MB
ℹ️ View Unchanged
|
lib/block-supports/media.php
Outdated
| return $block_content; | ||
| } | ||
|
|
||
| // TODO: should we get the style engine to handle `backgroundImage`? |
There was a problem hiding this comment.
That would make sense to me
There was a problem hiding this comment.
Great! I made a start on looking at adding it to the style engine but ran into a couple of edge cases. I might leave things here for now and split out a separate PR for looking at adding in backgroundImage / backgroundSize to the style engine.
There was a problem hiding this comment.
Out of interest, what were the edge cases?
There was a problem hiding this comment.
Out of interest, what were the edge cases?
It turns out the edge cases were me doing things wrong 😄
I think I've sorted it out now, I've pushed a commit in b836572 that seems to work for now. Since this PR is getting pretty big, though, if the approach in this PR winds up being viable, I can split out those changes in a separate PR to make it easier for review.
|
Once I have uploaded an image, if I try to change it I can open the media library and select a video, which doesn't work.... |
|
|
||
| .block-editor-hooks__media__inspector-upload-container, | ||
| .block-editor-hooks__media__inspector-media-replace-container { | ||
| button.components-button { |
There was a problem hiding this comment.
I feel like we should make this a variation of the button component rather than having a custom rule for it here. That should be a different PR though.
|
|
I can't seem to make both "color background" and "image background" work at the same time? what about with gradients? |
|
Excited to see this under way! I know you mentioned it wont be covered in this PR, but in terms of design it would be good to understand the broader vision for this. Is the intention to replicate the Cover experience, which is quite prescriptive? Or should we aim for something more flexible with options like background-size/position/repeat (as suggested), stacked backgrounds (images and colors), and so on? cc @WordPress/theme-team for thoughts. |
|
I am definitely missing a lot of context and knowledge but as I have explored a supports icon idea in the Icon/Vector registration API #53510 discussion and the test plugin - Demo of WordPress Icon API - I think a Why is support media = background media? The block should decide what to do with the media. The support opts in into a standard way of picking media. |
|
It would actually be great if we could create a standarized Media settings panel. Having a focal picker would also be nice. Having a Media settings panel for any block that can use media. I do see the Media settings evolve which can be used for various blocks. |
|
Flaky tests detected in d501bd6. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6105917677
|
|
Thanks for the early reviewing and testing, everyone! There were lots of comments, so I'll reply here, a little out of sequence. Apologies if I missed anything!
I've fixed this up — styles should only be output once, and they shouldn't be serialized to post content. This allows us to look at more dynamic behaviour in follow-ups such as setting background to Featured Image.
Global styles is an interesting case. I imagine blocks that could use it could include Post Content. But personally I'd be very interested in a root-global styles control for media/background image, particularly to set site-wide patterned backgrounds (like those in https://www.transparenttextures.com/). Since there would be lots of decisions to be made about how we'd implement it, I'm leaning toward us starting with just the block level for now, and build it out in global styles in follow-ups. This is another reason to keep all styles injection dynamic, so that we can change things in the future.
In contrast to Cover block, this block support doesn't do layering / overlays, so background color will only work with transparent images, but it does appear to work. There is a styling conflict with gradients, though, as gradients use the 2023-08-28.12.23.39.mp4
My intention with this PR is for us to start out as simple as possible, and then add more advanced controls in follow-ups (potentially hidden by default in the ToolsPanel). I'd love to see control over background size within repeat, so that folks can scale up/down their repeating images as desired. But again, for follow-ups — would be happy to see some design explorations for how the current PR might scale, though!
Thanks for the feedback! The intention wasn't for
Thanks! Yes, my hope is that if we settle on Media as a panel, it could help with consistency across blocks, too 👍
This should be fixed now, and I've added a notice for if the media type is incorrect on upload. Let me know if you're still encountering it, though:
Thanks again for all the feedback, folks! Let me know if there's other changes anyone would like to see. My objective for now is to try to focus on a decent MVP, and hopefully defer further features to follow-ups. I still have a little cleaning up to do with the PHP code, which I ran out of time to do today. Once that's done, and pending any design feedback / ideas, I'll switch this over to ready for review. |
This is I think not the first place where we favor server rendering over post content serialization for styles. The downside of this approach is that it breaks these blocks for third-party block editors. These customization can in theory be easily supported for server less block editors but we don't have the functions that render the right HTML for that (including all these dynamic styles that are static in reality). It would be good to keep that in mind and maybe keep track of all these static/dynamic styles to implement the |
Thanks for raising this! I've added a comment over on the Gutenberg as Framework issue, too (#53874 (comment)). Up until this point, I'd assumed that it was desirable for future block supports to be dynamically output via the style engine in both JS and PHP for the reasons discussed back in #37495, so I'm keen to better understand if there's been a shift in direction there (or if I've misunderstood, of course 🙂). In the case of this PR, the main reasons for dynamic output (even of static styles) are:
Yes, it'd be good to ensure all features can be rendered in the same way in JS or PHP contexts. For this one, since we're adding the logic to the JS version of the style engine, depending on how we might want dynamic JS injection to work in the future (possibly at block render time?), I think overall we're heading in the right direction for that? Please do let me know if I'm missing something though, as I might have missed some of the context behind the thinking in #53874. |
| @@ -17,4 +18,5 @@ export const styleDefinitions = [ | |||
| ...spacing, | |||
| ...typography, | |||
| ...shadow, | |||
| ...media, | |||
There was a problem hiding this comment.
At first I questioned the media top-level grouping, wondering if we could use background, given it has a lot of related properties.
I think it's a good decision however since most, probably all, of those properties manipulate a background element most likely to be an image, and then we'd also have a neat home for stuff like object-fit and anything related to replaced elements (videos, canvas, etc)
lib/block-supports/media.php
Outdated
| return $block_content; | ||
| } | ||
|
|
||
| // TODO: should we get the style engine to handle `backgroundImage`? |
There was a problem hiding this comment.
Out of interest, what were the edge cases?
| @@ -300,6 +300,10 @@ const BlockInspectorSingleBlock = ( { clientId, blockName } ) => { | |||
| label={ __( 'Border' ) } | |||
| /> | |||
| <InspectorControls.Slot group="styles" /> | |||
| <InspectorControls.Slot | |||
| group="media" | |||
| label={ __( 'Media' ) } | |||
There was a problem hiding this comment.
This is most likely overthinking it, but I was wondering if this might be confused with the other "Media", the library? Since it's specific to backgrounds for now, what about "Background media"?
There was a problem hiding this comment.
Good question! For the panel title, I was thinking of trying to keep it generic to Media for consistency with the Site Logo block that already has a Media panel, and leave the specifics to the individual block support within it. The idea being that we then have a consistent panel across any block that has media, irrespective of where that media winds up being output.
That was just my initial thinking though, happy to change things around! For now, I might leave it as-is while we await further design feedback 🙂
andrewserong
requested review from
spacedmonkey,
ellatrix,
ajitbohra and
ryanwelcher
as code owners
|
Thank you for all the collaboration here, everyone! 🙇 |
|
I've opened a backport PR for the PHP parts of this feature over in WordPress/wordpress-develop#5209 |
|
Related to the backport PR, I've opened up a separate Gutenberg PR to add in some more PHP tests over in #54489 |
|
Re: #53934 (comment)
@ajlende Another benefit of an Example comparing background images in Group block versus Cover block, where only latter gets |
|
Draft dev note: Background image block supportIn WordPress 6.4, a new background image block support has been added, with the Group block opted-in by default. For themes using the The feature allows users to set a background image for a Group block. The selected image will be output at render-time for the block via an inline style applied to the block's wrapper. When a background image is set, the rule How to add backgroundImage support to a themeThere are two ways to add support for For themes that wish to have more granular control over which UI tools are enabled, the Note that as of WP 6.4, the |
I'm looking at trying to prevent Gutenberg has backed itself into a pretty tight corner by using Because we're skipping serialization for background block supports however, to achieve the above means that some
Just flagging that changing output is not as flexible or convenient as it one might assume.
Relative to background-image, is this such a bad thing? If an image has been reset in the editor, Gutenberg can remove the style properties at that point. If background-size is set elsewhere, e.g., in theme.json and there's not image, I don't see why Gutenberg should care. |
What?
Fixes #14744, part of #16479. Previous attempt: #39243
This PR explores adding a Background block support, with a single
backgroundImageproperty to begin with. The Group block is opted-in to this support.Note: this PR originally proposed a Media block support as the container for this sub-feature. The PR currently reflects a Background block support, and the final naming is not (yet) decided — this is currently in discussion in the comments.
Why?
As discussed in #14744, it is useful for more container blocks to have background image support, rather than just the Cover block. The approach in this PR adds in background image support (within Background) as a generalised block support so that more blocks could use the feature in the future, too. Of note: the approach in this PR dynamically injects the background image and sizing at render time, which gives the block support flexibility, and means we don't need to worry about block deprecations.
How?
cover(with the ability for us to customise this value in follow-ups)Not covered
Testing Instructions
appearanceTools(e.g. TT3) add a Group block to a post or page, with a few blocks within it, and maybe some padding.To-do
Fix skipSerialization (currently the styles are being serialized when they shouldn't be)Screenshots or screencast
The following screengrab demos adding a background image with a transparent background, and then selecting a background image that has no transparency:
2023-08-30.15.04.23.mp4