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

Global Styles: Fix Separator block color use #67269

Open
wants to merge 5 commits into
base: trunk
Choose a base branch
from
+345 −180
Open

Global Styles: Fix Separator block color use #67269

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3aae7d5
Try enforcing global styles separator color
aaronrobertshaw Nov 25, 2024
27ece0c
Fix whitespace and US spelling
aaronrobertshaw Nov 27, 2024
e71fd97
Make separator hacks consistent work on theme.json data
aaronrobertshaw Nov 27, 2024
ba610d7
Add backport changelog
aaronrobertshaw Dec 2, 2024
4074641
Try allowing block instance styles to override global styles border c…
aaronrobertshaw Dec 6, 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.8/7927.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/7927

* https://github.com/WordPress/gutenberg/pull/67269
45 changes: 0 additions & 45 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 @@ -2676,46 +2676,6 @@ public function get_styles_block_nodes() {
return static::get_block_nodes( $this->theme_json );
}

/**
* Returns a filtered declarations array if there is a separator block with only a background
* style defined in theme.json by adding a color attribute to reflect the changes in the front.
*
* @since 6.1.1
*
* @param array $declarations List of declarations.
* @return array $declarations List of declarations filtered.
*/
private static function update_separator_declarations( $declarations ) {
// Gutenberg and core implementation differed.
// https://github.com/WordPress/gutenberg/pull/44943.
$background_color = '';
$border_color_matches = false;
$text_color_matches = false;

foreach ( $declarations as $declaration ) {
if ( 'background-color' === $declaration['name'] && ! $background_color && isset( $declaration['value'] ) ) {
$background_color = $declaration['value'];
} elseif ( 'border-color' === $declaration['name'] ) {
$border_color_matches = true;
} elseif ( 'color' === $declaration['name'] ) {
$text_color_matches = true;
}

if ( $background_color && $border_color_matches && $text_color_matches ) {
break;
}
}

if ( $background_color && ! $border_color_matches && ! $text_color_matches ) {
$declarations[] = array(
'name' => 'color',
'value' => $background_color,
);
}

return $declarations;
}

/**
* An internal method to get the block nodes from a theme.json file.
*
Expand Down Expand Up @@ -3003,11 +2963,6 @@ static function ( $pseudo_selector ) use ( $selector ) {
);
}

// Update declarations if there are separators with only background color defined.
if ( '.wp-block-separator' === $selector ) {
$declarations = static::update_separator_declarations( $declarations );
}

/*
* Root selector (body) styles should not be wrapped in `:root where()` to keep
* specificity at (0,0,1) and maintain backwards compatibility.
Expand Down
36 changes: 36 additions & 0 deletions lib/class-wp-theme-json-resolver-gutenberg.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -271,6 +271,27 @@ public static function get_theme_data( $deprecated = array(), $options = array()
$theme_json_data = static::inject_variations_from_block_style_variation_files( $theme_json_data, $variations );
$theme_json_data = static::inject_variations_from_block_styles_registry( $theme_json_data );

/*
* The core Separator block has Block Styles that rely on different CSS
* properties. For example, the "Dots" style renders using `::before` and
* requires styling the `color` property instead of `border-color`.
* TwentyTwenty's default style renders using a linear gradient that can't
* be applied to `color` so uses `background` instead.
*
* End users are only given a single color control in the Global Styles UI
* so we need to ensure all the required values are set if a theme only adds
* the `background` property, matching the UI.
*/
$separator_color = $theme_json_data['styles']['blocks']['core/separator']['color']['background'] ?? null;
if ( $separator_color ) {
if ( ! isset( $theme_json_data['styles']['blocks']['core/separator']['color']['text'] ) ) {
_wp_array_set( $theme_json_data, array( 'styles', 'blocks', 'core/separator', 'color', 'text' ), $separator_color );
}
if ( ! isset( $theme_json_data['styles']['blocks']['core/separator']['border']['color'] ) ) {
_wp_array_set( $theme_json_data, array( 'styles', 'blocks', 'core/separator', 'border', 'color' ), $separator_color );
}
}

/**
* Filters the data provided by the theme for global styles and settings.
*
Expand Down Expand Up @@ -563,6 +584,21 @@ public static function get_user_data() {
}
}

/*
* The Separator block uses different CSS properties for its color depending
* on how it is being rendered e.g. as "content" for the Dots style, or
* as a border etc.
*
* Uses are only presented with a single color control for background. Any
* selection of a background color should be applied to the other paths
* so it can be honored.
*/
$separator_color = $config['styles']['blocks']['core/separator']['color']['background'] ?? null;
Copy link
Member

Choose a reason for hiding this comment

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

Noticed over here: #57297 (comment)

What do you think we should do about gradients? If anything at all?

They are applied to the background prop, but don't really have any effect. And linear-gradient is invalid for color and border-color anyway as far as I see 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll dig into this one tomorrow and see if there's anything we can smooth out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've spent some time on an archaeological dig today around the addition of gradient support to the Separator block.

The TL;DR is that I believe it was simply mistakenly added. It wasn't referred to at all on the original PR and doesn't appear to have been tested in any way either. From what I can tell gradients have been broken since their adoption.

So...here's the long version of my current understanding 😅

  • Global Styles will generate styles targeting the background CSS property
  • The Separator block's hr element will inherit currentColor for its color CSS value
  • In the editor, the Separator block has a tiny amount of padding added which exposes some background
  • On the frontend, the Separator has no padding normally so no background is visible
  • Even in the editor where there is some background visible it's overlaid by the color property effectively masking any gradient applied here
  • If the background property has a linear-gradient value at all, copying that to border-color or color results in an invalid CSS rule and falls back to currentColor
  • The Separator block was updated in Separator block: refactor to use color block supports #38428 which skipped color block support serialization but it didn't take gradients into account at all
  • The approach in #38428 means
    • gradient classes aren't considered and remain on the block wrapper
    • any preset gradient isn't retrieved and processed like the backgroundColor value
    • when customColor is determined it only takes into account style.color.background forgetting about style.color.gradient
  • There's one default theme (TwentyTwenty) that appears to leverage linear-gradient while it sort of works the // characters don't get the gradient and hovering the block in the editor breaks the // onto separate lines
  • A theme could style the Separator block's hr element to not use border or color, give it a height or padding and then apply any background style including gradients.

At the end of all of that, I'm not sure if this is fixable to a degree where the Separator block could reliably have gradients applied to it. If it was "fixed" it might only be for use cases where the Separator block has had its default styles overridden or has a block style that makes color transparent, removes border, and gives it a visible height.

I'd like to see gradient support removed from the block. Given its current broken state and that it has been that way since its inception, I don't think there's much of a backwards compatibility argument to be made. Removing the gradients support at least lowers complexity and maintenance burden moving forward.

What do you think @ramonjd?

Copy link
Member

Choose a reason for hiding this comment

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

Amazing digging, thank you!

I'd like to see gradient support removed from the block. Given its current broken state and that it has been that way since its inception, I don't think there's much of a backwards compatibility argument to be made. Removing the gradients support at least lowers complexity and maintenance burden moving forward.

+1

In any case, since it's a long-standing issue, and doesn't affect the intention of this PR it's a "tomorrow" thing.

I appreciate the time you spent researching. It'll be useful to whoever picks it up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we do go with removing support it should only be deleting a single line from the block.json. Despite that, still worth a separate PR to debate and document the decision. I'll spin that up after I work through the possible follow-up for the issue Dan flagged below.

if ( $separator_color ) {
_wp_array_set( $config, array( 'styles', 'blocks', 'core/separator', 'color', 'text' ), $separator_color );
_wp_array_set( $config, array( 'styles', 'blocks', 'core/separator', 'border', 'color' ), $separator_color );
}
aaronrobertshaw marked this conversation as resolved.
Show resolved Hide resolved

/** This filter is documented in wp-includes/class-wp-theme-json-resolver.php */
$theme_json = apply_filters( 'wp_theme_json_data_user', new WP_Theme_JSON_Data_Gutenberg( $config, 'custom' ) );
static::$user = $theme_json->get_theme_json();
Expand Down
50 changes: 7 additions & 43 deletions 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 @@ -1282,41 +1282,6 @@ export const getBlockSelectors = (
return result;
};

/**
* If there is a separator block whose color is defined in theme.json via background,
* update the separator color to the same value by using border color.
*
* @param {Object} config Theme.json configuration file object.
* @return {Object} configTheme.json configuration file object updated.
*/
function updateConfigWithSeparator( config ) {
const needsSeparatorStyleUpdate =
config.styles?.blocks?.[ 'core/separator' ] &&
config.styles?.blocks?.[ 'core/separator' ].color?.background &&
! config.styles?.blocks?.[ 'core/separator' ].color?.text &&
! config.styles?.blocks?.[ 'core/separator' ].border?.color;
if ( needsSeparatorStyleUpdate ) {
return {
...config,
styles: {
...config.styles,
blocks: {
...config.styles.blocks,
'core/separator': {
...config.styles.blocks[ 'core/separator' ],
color: {
...config.styles.blocks[ 'core/separator' ].color,
text: config.styles?.blocks[ 'core/separator' ]
.color.background,
},
},
},
},
};
}
return config;
}

export function processCSSNesting( css, blockSelector ) {
let processedCSS = '';

Expand Down Expand Up @@ -1404,27 +1369,26 @@ export function useGlobalStylesOutputWithConfig(
if ( ! mergedConfig?.styles || ! mergedConfig?.settings ) {
return [];
}
const updatedConfig = updateConfigWithSeparator( mergedConfig );

const blockSelectors = getBlockSelectors(
getBlockTypes(),
getBlockStyles
);

const customProperties = toCustomProperties(
updatedConfig,
mergedConfig,
blockSelectors
);

const globalStyles = toStyles(
updatedConfig,
mergedConfig,
blockSelectors,
hasBlockGapSupport,
hasFallbackGapSupport,
disableLayoutStyles,
disableRootPadding
);
const svgs = toSvgFilters( updatedConfig, blockSelectors );
const svgs = toSvgFilters( mergedConfig, blockSelectors );

const styles = [
{
Expand All @@ -1437,7 +1401,7 @@ export function useGlobalStylesOutputWithConfig(
},
// Load custom CSS in own stylesheet so that any invalid CSS entered in the input won't break all the global styles in the editor.
{
css: updatedConfig.styles.css ?? '',
css: mergedConfig.styles.css ?? '',
isGlobalStyles: true,
},
{
Expand All @@ -1451,19 +1415,19 @@ export function useGlobalStylesOutputWithConfig(
// If there are, get the block selector and push the selector together with
// the CSS value to the 'stylesheets' array.
getBlockTypes().forEach( ( blockType ) => {
if ( updatedConfig.styles.blocks[ blockType.name ]?.css ) {
if ( mergedConfig.styles.blocks[ blockType.name ]?.css ) {
const selector = blockSelectors[ blockType.name ].selector;
styles.push( {
css: processCSSNesting(
updatedConfig.styles.blocks[ blockType.name ]?.css,
mergedConfig.styles.blocks[ blockType.name ]?.css,
selector
),
isGlobalStyles: true,
} );
}
} );

return [ styles, updatedConfig.settings ];
return [ styles, mergedConfig.settings ];
}, [
hasBlockGapSupport,
hasFallbackGapSupport,
Expand Down
7 changes: 7 additions & 0 deletions packages/block-library/src/separator/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,13 @@
border-bottom: none;
}

// 0-2-0 specificity required to enforce block instance
// color selections over global styles.
.wp-block-separator.has-background,
.wp-block-separator.has-text-color {
border-color: currentColor;
}
Comment on lines +11 to +16
Copy link
Member

Choose a reason for hiding this comment

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

Just so I understand:

  • if the separator block has a background color set, the color property will have the same value (thanks to the changes in this PR),
  • this rule therefore ensures the border color has the same value as color, through currentColor

Gave this a test. In TT4, I'm seeing that the block's instance color settings aren't overriding global style color for the 'Default' or 'Wide Line' block styles variations:

The CSS rule above addresses this issue for me in the editor and frontend:

  1. In TT4, change the background value of Wide Line variation in global styles
  2. At the block level, apply Wide Line variation, and overwrite the background color

Do block style variation global styles need the same treatment? 🤔

  1. In TT4, change the background value of Wide Line variation in global styles
  2. At the block level, apply Wide Line variation

The background color is set, but the theme.json border color still affects the appearance. Here's what I'm seeing in in the frontend:

:root :where(.wp-block-separator.is-style-wide--2) {
    background-color: #00ff40;
}

:root :where(.wp-block-separator) {
}
:root :where(.wp-block-separator) {
    border-color: currentColor;
    border-width: 0 0 1px 0;
    border-style: solid;
    color: var(--wp--preset--color--contrast);
}


// Dots block style variation
:root :where(.wp-block-separator.is-style-dots) {
text-align: center;
Expand Down
24 changes: 23 additions & 1 deletion packages/editor/src/components/global-styles-provider/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,9 +216,31 @@ export function useGlobalStylesContext() {
}, [ userConfig, baseConfig ] );

const context = useMemo( () => {
/*
* The Separator block uses different CSS properties for its color depending
* on how it is being rendered e.g. as "content" for the Dots style, or
* as a border etc.
*
* Uses are only presented with a single color control for background. Any
* selection of a background color should be applied to the other paths
* so it can be honored.
*/
const updatedUserConfig = { ...userConfig };
const separatorColor =
userConfig?.styles?.blocks?.[ 'core/separator' ]?.color?.background;

if ( separatorColor ) {
updatedUserConfig.styles.blocks[ 'core/separator' ].color.text =
separatorColor;
updatedUserConfig.styles.blocks[ 'core/separator' ].border = {
...userConfig.styles.blocks[ 'core/separator' ].border,
color: separatorColor,
};
}

return {
isReady: isUserConfigReady && isBaseConfigReady,
user: userConfig,
user: updatedUserConfig,
base: baseConfig,
merged: mergedConfig,
setUserConfig,
Expand Down
Loading
Loading