Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Background images: ensure appropriate default values #64192

Merged
merged 4 commits into from
Aug 7, 2024
Merged

Background images: ensure appropriate default values #64192

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
74c92c4
First commit:
ramonjd Aug 2, 2024
5fe858e
First commit:
ramonjd Aug 2, 2024
861ecb8
Linterino
ramonjd Aug 5, 2024
bde5671
Add Core changelog
ramonjd Aug 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions backport-changelog/6.7/7137.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
https://github.com/WordPress/wordpress-develop/pull/7137

* https://github.com/WordPress/gutenberg/pull/64192
11 changes: 11 additions & 0 deletions lib/class-wp-theme-json-gutenberg.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2383,6 +2383,17 @@ protected static function compute_style_properties( $styles, $settings = array()

// Processes background styles.
if ( 'background' === $value_path[0] && isset( $styles['background'] ) ) {
/*
* For user-uploaded images at the block level, assign defaults.
* Matches defaults applied in the editor and in block supports: background.php.
*/
if ( static::ROOT_BLOCK_SELECTOR !== $selector && ! empty( $styles['background']['backgroundImage']['id'] ) ) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the same as the assignments performed in background.php

Thinking this could be abstracted to a function either in background.php, or the style engine?

What do you reckon @andrewserong ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or the style engine?

Well, I think the style engine shouldn't provide defaults willy-nilly, but maybe it could "register" defaults or accepts them in the options argument.

Maybe a separate function is better here, e.g.,

gutenberg_style_engine_get_styles( array( 'background' => gutenberg_apply_style_defaults( $styles['background'] ) ) )

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be honest, I think I'd go with the duplication for now or until we have another block support that needs default values in this way, as what we're doing with background image seems fairly unique right now. Are there other use cases for gutenberg_apply_style_defaults we know of in the near-term?

If not, I'd lean a little toward going with the duplication, especially since it's only ~5 lines or so, and revisit if and when we have other defaults to apply. What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there other use cases for gutenberg_apply_style_defaults we know of in the near-term?

Not that I know.

Thanks for the feedback. I agree with your reasoning. I'll leave it for now then. 🙇🏻

$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';
}
}
$background_styles = gutenberg_style_engine_get_styles( array( 'background' => $styles['background'] ) );
$value = $background_styles['declarations'][ $css_property ] ?? $value;
}
Expand Down
30 changes: 17 additions & 13 deletions packages/block-editor/src/components/global-styles/background-panel.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -318,8 +318,12 @@ function BackgroundImageControls( {
return;
}

const sizeValue = style?.background?.backgroundSize;
const positionValue = style?.background?.backgroundPosition;
const sizeValue =
style?.background?.backgroundSize ||
inheritedValue?.background?.backgroundSize;
const positionValue =
style?.background?.backgroundPosition ||
inheritedValue?.background?.backgroundPosition;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This ensures inherited values (from global styles) are taken into account.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops, I think I introduced a bug here. We don't want the inherited styles to creep into the block supports styles - see the onChange below.

Note to self for tomorrow:

To test, insert a group block that has global styles. Replace the image with an uploaded one.

The styles from global styles should apply, but not be saved to the inline CSS.

The easiest thing to do would be to reset the styles once an image is uploaded, but that would ignore any theme styles. 🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WIP #64328


onChange(
setImmutably( style, [ 'background' ], {
Expand All @@ -334,6 +338,7 @@ function BackgroundImageControls( {
! positionValue && ( 'auto' === sizeValue || ! sizeValue )
? '50% 0'
: positionValue,
backgroundSize: sizeValue,
} )
);
};
Expand Down Expand Up @@ -448,6 +453,9 @@ function BackgroundSizeControls( {
const imageValue =
style?.background?.backgroundImage?.url ||
inheritedValue?.background?.backgroundImage?.url;
const isUploadedImage =
style?.background?.backgroundImage?.id ||
inheritedValue?.background?.backgroundImage?.id;
const positionValue =
style?.background?.backgroundPosition ||
inheritedValue?.background?.backgroundPosition;
Expand All @@ -456,19 +464,15 @@ function BackgroundSizeControls( {
inheritedValue?.background?.backgroundAttachment;

/*
* An `undefined` value is replaced with any supplied
* default control value for the toggle group control.
* An empty string is treated as `auto` - this allows a user
* to select "Size" and then enter a custom value, with an
* empty value being treated as `auto`.
* Set default values for uploaded images.
* The default values are passed by the consumer.
* Block-level controls may have different defaults to root-level controls.
* A falsy value is treated by default as `auto` (Tile).
*/
const currentValueForToggle =
( sizeValue !== undefined &&
sizeValue !== 'cover' &&
sizeValue !== 'contain' ) ||
sizeValue === ''
? 'auto'
: sizeValue || defaultValues?.backgroundSize;
! sizeValue && isUploadedImage
? defaultValues?.backgroundSize
: sizeValue || 'auto';

/*
* If the current value is `cover` and the repeat value is `undefined`, then
Expand Down
37 changes: 37 additions & 0 deletions packages/block-editor/src/components/global-styles/test/use-global-styles-output.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1028,6 +1028,43 @@ describe( 'global styles renderer', () => {
'letter-spacing: 2px',
] );
} );
it( 'should set default values for block background styles', () => {
const backgroundStyles = {
background: {
backgroundImage: {
url: 'https://wordpress.org/assets/image.jpg',
id: 123,
},
},
};
expect(
getStylesDeclarations( backgroundStyles, '.wp-block-group' )
).toEqual( [
"background-image: url( 'https://wordpress.org/assets/image.jpg' )",
'background-size: cover',
] );
// Test with root-level styles.
expect(
getStylesDeclarations( backgroundStyles, ROOT_BLOCK_SELECTOR )
).toEqual( [
"background-image: url( 'https://wordpress.org/assets/image.jpg' )",
] );
expect(
getStylesDeclarations(
{
background: {
...backgroundStyles.background,
backgroundSize: 'contain',
},
},
'.wp-block-group'
)
).toEqual( [
"background-image: url( 'https://wordpress.org/assets/image.jpg' )",
'background-position: center',
'background-size: contain',
] );
} );
} );

describe( 'processCSSNesting', () => {
Expand Down
2 changes: 1 addition & 1 deletion packages/block-editor/src/components/global-styles/use-global-styles-output.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -404,7 +404,7 @@ export function getStylesDeclarations(
* Set default values for block background styles.
* Top-level styles are an exception as they are applied to the body.
*/
if ( ! isRoot ) {
if ( ! isRoot && !! blockStyles.background?.backgroundImage?.id ) {
blockStyles = {
...blockStyles,
background: {
Expand Down
49 changes: 33 additions & 16 deletions phpunit/class-wp-theme-json-test.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4808,7 +4808,6 @@ public function test_get_top_level_background_image_styles() {
'backgroundImage' => array(
'url' => 'http://example.org/image.png',
),
'backgroundSize' => 'contain',
'backgroundRepeat' => 'no-repeat',
'backgroundPosition' => 'center center',
'backgroundAttachment' => 'fixed',
Expand All @@ -4822,7 +4821,7 @@ public function test_get_top_level_background_image_styles() {
'selector' => 'body',
);

$expected_styles = "html{min-height: calc(100% - var(--wp-admin--admin-bar--height, 0px));}body{background-image: url('http://example.org/image.png');background-position: center center;background-repeat: no-repeat;background-size: contain;background-attachment: fixed;}";
$expected_styles = "html{min-height: calc(100% - var(--wp-admin--admin-bar--height, 0px));}body{background-image: url('http://example.org/image.png');background-position: center center;background-repeat: no-repeat;background-attachment: fixed;}";
$this->assertSameCSS( $expected_styles, $theme_json->get_styles_for_block( $body_node ), 'Styles returned from "::get_styles_for_block()" with top-level background styles do not match expectations' );

$theme_json = new WP_Theme_JSON_Gutenberg(
Expand Down Expand Up @@ -4853,27 +4852,45 @@ public function test_get_block_background_image_styles() {
'core/group' => array(
'background' => array(
'backgroundImage' => "url('http://example.org/group.png')",
'backgroundSize' => 'cover',
'backgroundRepeat' => 'no-repeat',
'backgroundPosition' => 'center center',
'backgroundAttachment' => 'fixed',
),
),
'core/quote' => array(
'background' => array(
'backgroundImage' => array(
'backgroundImage' => array(
'url' => 'http://example.org/quote.png',
'id' => 321,
),
'backgroundSize' => 'contain',
),
),
'core/verse' => array(
'background' => array(
'backgroundImage' => array(
'url' => 'http://example.org/verse.png',
'id' => 123,
),
'backgroundSize' => 'cover',
'backgroundRepeat' => 'no-repeat',
'backgroundPosition' => 'center center',
),
),
),
),
)
);

$group_node = array(
'name' => 'core/group',
'path' => array( 'styles', 'blocks', 'core/group' ),
'selector' => '.wp-block-group',
'selectors' => array(
'root' => '.wp-block-group',
),
);

$group_styles = ":root :where(.wp-block-group){background-image: url('http://example.org/group.png');background-position: center center;background-repeat: no-repeat;background-attachment: fixed;}";
$this->assertSameCSS( $group_styles, $theme_json->get_styles_for_block( $group_node ), 'Styles returned from "::get_styles_for_block()" with core/group background styles as string type do not match expectations' );

$quote_node = array(
'name' => 'core/quote',
'path' => array( 'styles', 'blocks', 'core/quote' ),
Expand All @@ -4883,20 +4900,20 @@ public function test_get_block_background_image_styles() {
),
);

$quote_styles = ":root :where(.wp-block-quote){background-image: url('http://example.org/quote.png');background-position: center center;background-repeat: no-repeat;background-size: cover;}";
$this->assertSameCSS( $quote_styles, $theme_json->get_styles_for_block( $quote_node ), 'Styles returned from "::get_styles_for_block()" with block-level background styles do not match expectations' );
$quote_styles = ":root :where(.wp-block-quote){background-image: url('http://example.org/quote.png');background-position: center;background-size: contain;}";
$this->assertSameCSS( $quote_styles, $theme_json->get_styles_for_block( $quote_node ), 'Styles returned from "::get_styles_for_block()" with core/quote default background styles do not match expectations' );

$group_node = array(
'name' => 'core/group',
'path' => array( 'styles', 'blocks', 'core/group' ),
'selector' => '.wp-block-group',
$verse_node = array(
'name' => 'core/verse',
'path' => array( 'styles', 'blocks', 'core/verse' ),
'selector' => '.wp-block-verse',
'selectors' => array(
'root' => '.wp-block-group',
'root' => '.wp-block-verse',
),
);

$group_styles = ":root :where(.wp-block-group){background-image: url('http://example.org/group.png');background-position: center center;background-repeat: no-repeat;background-size: cover;background-attachment: fixed;}";
$this->assertSameCSS( $group_styles, $theme_json->get_styles_for_block( $group_node ), 'Styles returned from "::get_styles_for_block()" with block-level background styles as string type do not match expectations' );
$verse_styles = ":root :where(.wp-block-verse){background-image: url('http://example.org/verse.png');background-size: cover;}";
$this->assertSameCSS( $verse_styles, $theme_json->get_styles_for_block( $verse_node ), 'Styles returned from "::get_styles_for_block()" with default core/verse background styles as string type do not match expectations' );
}

/**
Expand Down
Loading