Background block supports: move block support defaults to gutenberg_render_background_support and revert gutenberg_get_background_support_styles #59889
Conversation
|
Size Change: -114 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
|
Thanks for opening the PR! In #59454 (comment), I was imagining that defaulting to So, what I was wondering is if instead of altering the default output of the block support, if we can achieve it (possibly in #59454) via what the UI controls set when we add a background image, rather than altering the implicit value in the block support's output. I.e. if What do you think? |
Thanks for the thoughts 🙇🏻 I'll have a play around. We'd also need to consider how the backend operates. I'm thinking root theme.json styles should behave similarly to global styles in the editor, i.e., defaulting to "auto". However, we'd default to "cover" when "styles.blocks['core/group']` backgrounds are (eventually enabled and) set. Given that, I'm thinking maybe we could do a blanket default of "auto" and then pass a param to dictate alt behaviour, e.g., "cover". It might be useful use "blockName" as a flag, or some generic flag, or something. I'll experiment 👍🏻
Good idea. This might work. I'll also try using |
|
Thanks for digging in! I suppose one of the questions in terms of default values might be: "If I were writing a 3rd party block and wanted to opt in to the support, what would I expect the default size to be?"
Not sure if it helps or is a good idea, but the layout support allows a default value in |
Excellent! I like this. |
|
Actually, testing this out again today I'm not convinced that the style engine needs to be at all opinionated about background size, or any, values. If we coerce, it probably should be in the block supports themselves. |
ramonjd
changed the title
ramonjd
added
[Type] Enhancement
ramonjd
force-pushed
the
update/default-background-size-auto
branch
from
49208b0 to
f2bf6be
Compare
ramonjd
requested review from
spacedmonkey,
tellthemachines,
ajitbohra,
ryanwelcher,
juanmaguitar,
fabiankaegy and
ndiego
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. |
There was a problem hiding this comment.
I might have gone a bit far 😄
The intention was to:
- maintain existing defaults and behaviour for the group block (both editor and frontend)
- remove defaults for global styles, and decide what to do with them in Global styles: background UI controls #59454
Whenever you get free time to validate the approach, I'd be very grateful!
There was a problem hiding this comment.
Thanks for the ping and for digging in deep here, Ramon! This PR does a bit of a refactor at the same time as updating behaviour, so if it gets tricky to wrangle, we could potentially split it further into smaller PRs? I've left a few comments about the refactor parts, but my main thoughts were regarding the switch of the default to auto.
One potential blocker to switching out the default for blocks to auto is that the background image/size block supports have been out in the wild for a while now, and I see there are at least a couple of plugins that use backgroundSize: true and so might be depending on cover being the default 🤔
Here's a search in the plugin directory with Autogrid and Good Slider opting in to backgroundImage and backgroundSize — while it doesn't look like they have installs, it could be a signal of potentially wider use of the block support.
So, in terms of backwards compatibility, would it be worth exploring the idea that backgroundSize being set to true defaults to cover, but that we can also set the default to auto? If so, how might this look for when we get to site wide background images in global styles?
I'm imagining that once this PR lands, we want to be in a situation that's something like the following:
- If a user adds an image to a Group block in post content, it'll get
coversize by default - If a user adds an image to all Group blocks in global styles, they'll also get
coversize by default - If a user adds a site-wide image in global styles, then it'll get
autosize by default
If so, it sounds like auto might be the exception rather than the default used in most cases? What do you think?
|
Thanks a lot for testing this PR!
Oh yeah. I was enchanted by the seductive temptations of progress. In this case, I think it's okay to leave the "cover" value as a default for blocks and remove the block.json-related shizzle. It will hopefully make this all easier to review as well. Would that be reasonable?
This is the way I understand it too. Block background size styles (individual and global) default to "cover", and theme.json top-level backgrounds default to "auto". In fact, we probably needn't set "auto" at all - it's the default whenever one sets a background-image anyway if I'm not mistaken. 😄 So really, the question we're trying to answer is "should html backgrounds (site-wide) have a default other than 'cover'", at least when it comes to theme.json. Probably "yes" given your findings in #59454 (review)? |
|
Nice one, thanks for distilling the discussion.
Yes, I think that sounds ideal to me 👍
That's my feeling, too. Most blocks are in a limited area visually where Let me know when you're ready for another review, and I'll give this another spin! |
|
I think I have this working so I'm going to split this PR tomorrow:
👍🏻 Thanks again @andrewserong for the helpful reviews. |
ramonjd
changed the title
lib/block-supports/background.php
Outdated
| $background_styles['backgroundPosition'] = 'center'; | ||
| } | ||
| } | ||
|
|
||
| return gutenberg_style_engine_get_styles( array( 'background' => $background_styles ) ); |
There was a problem hiding this comment.
It looks like all this function does now is call gutenberg_style_engine_get_styles. Is it still worth keeping this function in? If we're not planning to add any other features to this function, then I think we could likely safely remove it since it hasn't been in Gutenberg very long. If we're thinking we might add more features to it as part of the background image UI in global styles, then please ignore this comment 😄
There was a problem hiding this comment.
then please ignore this comment
I just remembered that we'll likely still want a shared function for future functionality like retrieving an image based on a theme url, or potentially a featured image, so there's at least a couple of examples of potential uses for the function in the future 😄
There was a problem hiding this comment.
Very good point - I left it in because we might want some hackery for global styles as well.
I was hoping to know for sure when we flip the switch on block global styles in theme.json.
One other thing: the style engine handles both "backgroundImage": "url(...)" and "backgroundImage": { "url": "http://..." }
But you're right. Apart from that it's not doing anything.
I'm happy to cull it here, or keep it until we're sure we don't need it. 🤔
There was a problem hiding this comment.
Whichever way you want to go with it! I think it all depends on what abstract makes sense to a consumer. Calling "in" to a block support function from global styles, even if it only does one other thing isn't necessarily bad, as it helps abstract things a little, so conceptually I don't mind it.
There was a problem hiding this comment.
One other thing: the style engine handles both "backgroundImage": "url(...)" and "backgroundImage": { "url": "http://..." }
This is the only advantage as far as I can see.
Removing the function just means that theme.json users will have to use "backgroundImage": "url(...)" for now, and the block support/theme.json models won't be the same for the backgroundImage property.
I don't think that's so bad, and we can always reinstate when things are more concrete.
There was a problem hiding this comment.
So I reverted it.
Now we're nearly back to the state before:
😄
ramonjd
force-pushed
the
update/default-background-size-auto
branch
from
662b3ec to
7ab18c8
Compare
ramonjd
changed the title
Also defaulting to the values that the style engine set previously.
…evel theme.json backgroundImage can only be a string that is a valid CSS value for `background-image` and not an object. It might revert back, but for now we don't need this extra code.
…because background image can be a string or an object.
ramonjd
force-pushed
the
update/default-background-size-auto
branch
from
5350ab5 to
e17df6e
Compare
…ender_background_support and revert gutenberg_get_background_support_styles (WordPress#59889) * Move block support defaults to gutenberg_render_background_support and revert gutenberg_get_background_support_styles * Revert JS changes, moving them to a new PR WordPress#60008 * background-size auto is the same as no background-size * Using style engine in theme json class to generate background styles because background image can be a string or an object. Co-authored-by: ramonjd <ramonopoly@git.wordpress.org> Co-authored-by: andrewserong <andrewserong@git.wordpress.org>
What and how?
This PR:
gutenberg_render_background_supporton the PHP side, and to the background hook (from style engine).gutenberg_get_background_support_stylesas it's only calling the style engine, and moving the style engine call to the theme json class.The consequences are:
Context:
Important
Changes in this PR only affect the frontend. Harmonizing the site editor with these changes is happening over in #60008
Why?
To isolate specific property mutations to block supports.
When adding backgrounds to blocks in the editor, e.g., Group block, having defaults allows users to see the scaled image immediately as the containing element might be too small or large for the source image.
For site backgrounds in global styles, these defaults may not make sense, especially if theme devs deliberately leave them out in theme.json.
As for reverting
gutenberg_get_background_support_styles, it's code that's not longer required. It hasn't been in Gutenberg for long and is not in WordPress Core. Let's re-add it as we need it, which might be soon.Testing Instructions
Test the changes on a published post in the frontend.
supports.background.backgroundSizeis set to another defaults, e.g.,"contain", in which case it'll be "contain"Example theme.json
{ "$schema": "../../schemas/json/theme.json", "version": 2, "settings": { "appearanceTools": true, "background": { "backgroundImage": true, "backgroundSize": true } }, "styles": { "color": { "background": "tomato" }, "background": { "backgroundImage": "url('https://i0.wp.com/wordpress.org/files/2023/12/sotw-dotorg-drawer.png?w=1807&ssl=1')" } } }Screenshots or screencast
2024-03-20.11.05.10.mp4