Background image: ensure consistency with defaults and fix reset/remove functionality #64328
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
…ontrols. - checks global styles for uploaded images and applies defaults - changes the default position from "center" to "50% 50%" so it displays in the controls - ensures that reset/remove functionality closes the panel and removes image - do not save already-inherited styles to the block, and don't apply defaults where an inherited style already exists.
|
Size Change: +3.28 kB (+0.19%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
…nel. Add defaults for position '50% 50%' for blocks so it can be displayed in the background panel controls Pass inherited values to setBackgroundStyleDefaults so as not apply/overwrite inherited styles. Tighten up logic in setBackgroundStyleDefaults so that background position isn't set when an inherited value is contain and the block style is something else.
andrewserong
reviewed
Aug 8, 2024
lib/block-supports/background.php
Outdated
Comment on lines
61
to
67
| if ( isset( $background_styles['backgroundImage']['id'] ) ) { | ||
| $inherited_styles = gutenberg_get_global_styles(); | ||
| $inherited_block_background_styles = $inherited_styles['blocks'][ $block['blockName'] ]['background'] ?? null; | ||
| $inherited_background_size = $inherited_block_background_styles['backgroundSize'] ?? null; | ||
| if ( ! isset( $background_styles['backgroundSize'] ) && ! $inherited_background_size ) { | ||
| $background_styles['backgroundSize'] = 'cover'; | ||
| } |
There was a problem hiding this comment.
Apologies if I'm missing something, but just thinking about this one a little more. Why do we need to check the inherited styles in the server-rendered output of the block instance level styles?
In this case, the image will always be one that the user set (or that's bundled in a pattern), so in that case, won't we want to go with the defaults rather than styles inherited from global styles? I.e. if a user sets an image at the block instance level, we assume that they're setting things in the UI to their liking rather than inheriting from global styles at that point 🤔
Mostly I think my question boils down to: do we need to complexity of this look up? In the UI at least we kind of bundle setting an image with all the other properties now, so I wasn't sure how necessary it is to allow this inheritance.
Edit: on the other hand I do see the value in attempting to preserve what's set in global styles so that it overwrites our defaults logic! I suppose I'm torn on this one 😄
There was a problem hiding this comment.
Why do we need to check the inherited styles in the server-rendered output of the block instance level styles?
Good question. It's all very complex, you're right.
My assumption on this one is that, where the theme or global styles provide values for a block, the block should use them if no local styles exist to overwrite them.
So if a theme defines "contain" for a block but nothing else, and a user uploads a new image to that block, whether in global styles or at the block level, the default will be the same as if they chose "contain" themselves.
The other, and probably easier option, would be to just set "cover" every time a user adds an image and forget about inherited values?
There was a problem hiding this comment.
I very much appreciate the attention to detail here! I think the way you've done is the right way to go about it while handling a complex situation. But my gut feeling is that it's probably more complex than we need it to be now that we have all the controls bundled together in the popover where folks can make adjustments easily.
The other, and probably easier option, would be to just set "cover" every time a user adds an image and forget about inherited values?
That sounds like a good option to try to me!
There was a problem hiding this comment.
@andrewserong I've simplified this PR and focussed on bug/inconsistency fixing.
Thank you for helping me move it forward! 🙇🏻
… and block supports, always apply the defaults.
ramonjd
changed the title
ramonjd
added
[Type] Bug
69 tasks
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
commented
Aug 8, 2024
| @@ -2391,7 +2391,7 @@ protected static function compute_style_properties( $styles, $settings = array() | |||
| $styles['background']['backgroundSize'] = $styles['background']['backgroundSize'] ?? 'cover'; | |||
| // If the background size is set to `contain` and no position is set, set the position to `center`. | |||
| if ( 'contain' === $styles['background']['backgroundSize'] && empty( $styles['background']['backgroundPosition'] ) ) { | |||
| $styles['background']['backgroundPosition'] = 'center'; | |||
| $styles['background']['backgroundPosition'] = '50% 50%'; | |||
There was a problem hiding this comment.
Changing from "center" to "50% 50%" because it's the same value, but we can display this in the controls.
| return ( | ||
| <VStack spacing={ 4 } className="single-column"> | ||
| <VStack spacing={ 3 } className="single-column"> |
There was a problem hiding this comment.
These changes are about reducing space, can be in a separate PR
| onChange={ updateBackgroundPosition } | ||
| /> | ||
| <ToggleControl | ||
| __nextHasNoMarginBottom | ||
| label={ __( 'Fixed background' ) } | ||
| checked={ attachmentValue === 'fixed' } | ||
| onChange={ toggleScrollWithPage } | ||
| help={ __( |
There was a problem hiding this comment.
Another change about reducing space, can be in a separate PR
| const shouldShowBackgroundImageControls = | ||
| hasImageValue && | ||
| 'none' !== imageValue && |
There was a problem hiding this comment.
Ensure the panel closes where an image is removed
lib/block-supports/background.php
Outdated
Comment on lines
61
to
67
| if ( isset( $background_styles['backgroundImage']['id'] ) ) { | ||
| $inherited_styles = gutenberg_get_global_styles(); | ||
| $inherited_block_background_styles = $inherited_styles['blocks'][ $block['blockName'] ]['background'] ?? null; | ||
| $inherited_background_size = $inherited_block_background_styles['backgroundSize'] ?? null; | ||
| if ( ! isset( $background_styles['backgroundSize'] ) && ! $inherited_background_size ) { | ||
| $background_styles['backgroundSize'] = 'cover'; | ||
| } |
There was a problem hiding this comment.
@andrewserong I've simplified this PR and focussed on bug/inconsistency fixing.
Thank you for helping me move it forward! 🙇🏻
| * The incoming value could be a value + unit, e.g. '20px'. | ||
| * In this case set the value to 'tile'. | ||
| */ | ||
| currentValueForToggle = ! [ 'cover', 'contain', 'auto' ].includes( |
There was a problem hiding this comment.
This is a new check to activate the "Tile" toggle for non-word values, e.g., value + unit.
This ensure consistency with toggling sizes, where clicking on tile and updating the width will not deactivate tile - the "tile" effect is still there as far as the user is concerned.
| @@ -510,6 +523,7 @@ function BackgroundSizeControls( { | |||
| * receives a default background position of '50% 0', | |||
| * when the toggle switches to "Tile". This is to increase the chance that | |||
| * the image's focus point is visible. | |||
| * This is in-editor only to assist with the user experience. | |||
There was a problem hiding this comment.
More comments just to explain all this madness.
| @@ -562,24 +576,27 @@ function BackgroundSizeControls( { | |||
| ) | |||
| ); | |||
|
|
|||
| // Set a default background position for non-site-wide, uploaded images with a size of 'contain'. | |||
| const backgroundPositionValue = | |||
There was a problem hiding this comment.
Display the defaults if any are set.
| @@ -725,7 +747,7 @@ export default function BackgroundPanel( { | |||
| ) } | |||
| > | |||
| <ToolsPanelItem | |||
| hasValue={ () => hasImageValue } | |||
| hasValue={ () => !! value?.background } | |||
There was a problem hiding this comment.
Because we can change size, repeat etc values of inherited background images, we should check for all properties, not just the image to indicate whether the controls can be reset.
| @@ -55,31 +56,28 @@ export function hasBackgroundSupport( blockName, feature = 'any' ) { | |||
| } | |||
|
|
|||
| export function setBackgroundStyleDefaults( backgroundStyle ) { | |||
| if ( ! backgroundStyle ) { | |||
| if ( ! backgroundStyle || ! backgroundStyle?.backgroundImage?.url ) { | |||
There was a problem hiding this comment.
Just tightening things up here.
andrewserong
approved these changes
Aug 9, 2024
There was a problem hiding this comment.
Just walked through this code change and demo with @ramonjd on a call, and this is looking terrific to me!
✅ Works nicely at the block level in post content
✅ Block level in global styles is working correctly
✅ As-is site wide background images
LGTM! 🚀
ramonjd
added
the
Backport to Gutenberg RC
|
I just cherry-picked this PR to the release/19.0 branch to get it included in the next release: 1521946 |
jorgefilipecosta
pushed a commit
that referenced
this pull request
Aug 13, 2024
…ve functionality (#64328) * Fixes miscellaneous bugs in the background image default values and controls. - checks global styles for uploaded images and applies defaults - changes the default position from "center" to "50% 50%" so it displays in the controls - ensures that reset/remove functionality closes the panel and removes image - do not save already-inherited styles to the block, and don't apply defaults where an inherited style already exists. * Reduce vertical height a smidge * Ensure defaults are accurately displayed in the background control panel. Add defaults for position '50% 50%' for blocks so it can be displayed in the background panel controls Pass inherited values to setBackgroundStyleDefaults so as not apply/overwrite inherited styles. Tighten up logic in setBackgroundStyleDefaults so that background position isn't set when an inherited value is contain and the block style is something else. * Simplify default logic for blocks with uploaded images * Update tests. * Adds tests for setBackgroundStyleDefaults * Simplify the inheritence checks. For uploaded images in global styles and block supports, always apply the defaults. * Simplify the inheritence checks. For uploaded images, always apply the defaults. * Changelog * Revert background.php change and comment in background-panel.js Co-authored-by: ramonjd <[email protected]> Co-authored-by: andrewserong <[email protected]>
jorgefilipecosta
removed
the
Backport to Gutenberg RC
getdave
pushed a commit
that referenced
this pull request
Aug 14, 2024
…ve functionality (#64328) * Fixes miscellaneous bugs in the background image default values and controls. - checks global styles for uploaded images and applies defaults - changes the default position from "center" to "50% 50%" so it displays in the controls - ensures that reset/remove functionality closes the panel and removes image - do not save already-inherited styles to the block, and don't apply defaults where an inherited style already exists. * Reduce vertical height a smidge * Ensure defaults are accurately displayed in the background control panel. Add defaults for position '50% 50%' for blocks so it can be displayed in the background panel controls Pass inherited values to setBackgroundStyleDefaults so as not apply/overwrite inherited styles. Tighten up logic in setBackgroundStyleDefaults so that background position isn't set when an inherited value is contain and the block style is something else. * Simplify default logic for blocks with uploaded images * Update tests. * Adds tests for setBackgroundStyleDefaults * Simplify the inheritence checks. For uploaded images in global styles and block supports, always apply the defaults. * Simplify the inheritence checks. For uploaded images, always apply the defaults. * Changelog * Revert background.php change and comment in background-panel.js Co-authored-by: ramonjd <[email protected]> Co-authored-by: andrewserong <[email protected]>
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.
Follow up to:
What and how?
Block background styles will be part of WordPress 6.7.
This PR fixes miscellaneous bugs in the background image default values and controls. It:
blocks should honour any inherited styles. That means, defaults should not overwrite styles defined in theme.json or global stylesWhy?
To address a lot of inconsistencies in the way defaults are applied to block background styles.
Testing Instructions
This might take some time, sorry!
Using a theme with background images set on blocks (see example theme.json), add some blocks to the editor (see example blocks).
Ensure that you can see inherited styles in the controls and that no background default values are applied.
Now upload/add your own images to these blocks.
Ensure that the frontend matches what you're seeing in the editor.
Site-wide background images should operate as before and be unaffected by any defaults aside from the existing position of
50% 0position when switching to tiled.Example block HTML
```htmlGroup
example theme.json
{ "$schema": "../../schemas/json/theme.json", "version": 3, "settings": { "appearanceTools": true, "layout": { "contentSize": "840px", "wideSize": "1100px" } }, "styles": { "background": { "backgroundImage": { "url": "https://images.pexels.com/photos/105819/pexels-photo-105819.jpeg?auto=compress&cs=tinysrgb&w=1260&h=750&dpr=" } }, "blocks": { "core/verse": { "background": { "backgroundImage": { "url": "https://images.pexels.com/photos/27332202/pexels-photo-27332202/free-photo-of-surfer.jpeg?auto=compress&cs=tinysrgb&w=1260&h=750&dpr=2" }, "backgroundSize": "contain" }, "dimensions": { "minHeight": "100px" } }, "core/quote": { "background": { "backgroundImage": { "url": "https://images.pexels.com/photos/22484288/pexels-photo-22484288/free-photo-of-the-circular-stone-terraces-of-the-inca-ruins.jpeg?auto=compress&cs=tinysrgb&w=1260&h=750&dpr=2" } }, "dimensions": { "minHeight": "100px" } }, "core/group": { "background": { "backgroundImage": { "url": "https://images.pexels.com/photos/27101553/pexels-photo-27101553/free-photo-of-a-cocktail-with-mint-and-lime-in-it.jpeg?auto=compress&cs=tinysrgb&w=1260&h=750&dpr=2" } }, "dimensions": { "minHeight": "111px" } } } } }Testing Instructions for Keyboard
Screenshots or screencast
Kapture.2024-08-08.at.21.06.07.mp4
TODO