Separator Block: user colors styles are not merged with base theme.json correctly, style book CSS overrides user margin values

[creativecoder]

Description

Where a theme has defined a border color for the core/separator block, changing the colors in the Separator Block global style settings does not appear to update the block.

Themes:

• Twenty Twenty Four
• Twenty Twenty Five

I think this is because if a theme defines a value for styles.blocks.core/separator.border.color, updating the global styles for the block will only change the background color.

It appears to be a bug with updateConfigWithSeparator() introduced in #44943

In the style book, changing the margins won't take effect for the separator block because the style book has rules that overrides them:

https://github.com/WordPress/gutenberg/blob/trunk/packages/edit-site/src/components/style-book/constants.ts#L258-L263

Step-by-step reproduction instructions

1. In TT4 or TT5, open the Site editor and insert a Seperator block
2. Change background color
3. See that the block preview does not update correctly
4. Open the style book and view the separator block
5. Change the margins for the separator block in global styles
6. The margin styles are overridden by the style book's CSS

Screenshots, screen recording, code snippet

Environment info

• WP: 6.5-alpha-56966-src
• Gutenberg: 17.3.0

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

[richtabor]

Post editor applies color as well as background-color. The style book implementation only applies background-color:

[Takshil-Kunadia]

This is happenning due to the ExperimentalBlockEditorProvider used in Style Book

gutenberg/packages/edit-site/src/components/style-book/index.js

Lines 463 to 469 in 77122c5

	<ExperimentalBlockEditorProvider
	value={ renderedBlocks }
	settings={ settings }
	>
	<EditorStyles />
	<BlockList renderAppender={ false } />
	</ExperimentalBlockEditorProvider>

[ramonjd]

This is not a style book issue, but a global styles one. Here's a separator block in TT4 in the editor canvas:

From what I can tell, the colors are not being merged for themes that define separate block colors in theme.json (TT4 for example)

TT3 does not specify colors for the separator block, so it works.

I suspect the bespoke merge of separator block base and user styles configs isn't overwriting the base.

[ramonjd]

This is not a style book issue, but a global styles one.

Correction, the color issue is a global styles bug.

The margin issue is a style book bug.

[ramonjd]

Style book margin PR:

• Style book: reduce margin selector specificity so that it doesn't override global block styles #66895

[ramonjd]

I'm not confident on the solution for the color however.

Just going by the editor, the separator block has a single background color control.

As a user, I'd expect changing the value using this control would be reflected in the editor.

However some themes (TT4, TT5) specify a border color for the separator block in theme.json, and therefore the needsSeparatorStyleUpdate will fail causing the border color to take precedence.

I'm thinking that, if a background color exists, it should always overwrite both text and border color. I think that still satisfies the bug reported in #43660

Example code:

/**
 * If there is a separator block whose color is defined in theme.json via background,
 * override the text and border colors to use the single color.
 * In the editor, the separator block has a single background color control.
 *
 * @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;

	if ( needsSeparatorStyleUpdate ) {
		return {
			...config,
			styles: {
				...config.styles,
				blocks: {
					...config.styles.blocks,
					'core/separator': {
						...config.styles.blocks[ 'core/separator' ],
						border: {
							...config.styles.blocks[ 'core/separator' ].border,
							color: config.styles?.blocks[ 'core/separator' ]
								.color.background,
						},
						color: {
							...config.styles.blocks[ 'core/separator' ].color,
							text: config.styles?.blocks[ 'core/separator' ]
								.color.background,
						},
					},
				},
			},
		};
	}
	return config;
}

What do folks think?

cc @cbravobernal and @michalczaplinski who worked on #44943

[aaronrobertshaw]

Appreciate the sleuthing here @ramonjd 👍

Just going by the editor, the separator block has a single background color control.

The history of the Separator block makes it a tricky one.

There are the original styles for the block which simply use border to apply color. Then the "Dots" block style was added. To achieve those dots, the content property was required, to which border would no longer apply. For these, the color property was used.

Further down the line default themes like TwentyTwenty did funky things like rendering // in the middle of the separator. The styles to achieve that apparently needed linear-gradient() which couldn't be applied to the color property, so we got yet another CSS property used, background.

This all means the theme.json/global styles value that would be needed depends on the block style or theme (looking at you 2020).

The end user doesn't care about any of this history or nuance, they just want a single color control to change the Separator. So, I can understand why background was used. Border makes a lot of sense but once the separator is more like a shape than a line, background suits more. None of the separator styles fit well with color and how we map that in theme.json i.e. color.text, as there's not a lot of text going on in the separator block 🙂

I'm thinking that, if a background color exists, it should always overwrite both text and border color. I think that still satisfies the bug reported in #43660

Agreed.

In the end, when a user sets a color for the Separator block in Global Styles, we need that to be used. As different block styles and themes might need that value in different paths within the global styles data, it looks like we need to set it for all the possible paths at once. That could happen similar to now, within the theme.json class and useGlobalStylesOutput hook, or within the Global Styles color panel. Either way we have block specific code somewhere we'd rather it not be.

To be able to assign the color value to all the paths that might be needed, color.background, color.text, and border.color, we'd need to ensure that the "unused" paths and the styles they generate are overridden by existing styles in the block library's stylesheet. In other words, we don't want borders around Separators that should have them etc.

So other than superfluous CSS being sent to the frontend, the other potential downside is that fixing this issue might also be breaking for some themes.

I'd be happy to get a PR up around wrangling Separator colors. We can then at least cast the net wide for testing across lots of themes of various persuasions.

[aaronrobertshaw]

I have a draft PR available in #67269 that is exploring a possible angle on fixing this issue by consistently merging the separator color values at the theme.json data layer rather than across both the style data and style declarations.

While looking promising, it might need a little more time to cook.

[cbravobernal]

I'm thinking that, if a background color exists, it should always overwrite both text and border color. I think that still satisfies the bug reported in #43660

Would it override the user choice? Using pre-built colors was modifying the color CSS attribute, not the background one. That was the issue we faced that time.

Would background color work on dotted separators?

[ramonjd]

Thanks for chiming in @cbravobernal, and especially for the context

Would it override the user choice?

From my testing of #67269 user choice takes precedence at every level.

When a user wants to change the background color of the separator, the other properties are overwritten too so that the user's choice is honored.

Using pre-built colors was modifying the color CSS attribute, not the background one. That was the issue we faced that time.
Would background color work on dotted separators?

It looks like #67269 also handles both of these situations.

One thing I noticed was that gradients don't really have any effect. I'm wondering if should disable the control 🤔

Kapture.2024-12-03.at.11.05.08.mp4

Anyway, if you have time to test that PR, it would be very helpful!

[aaronrobertshaw]

I think @ramonjd covers it all.

Using pre-built colors was modifying the color CSS attribute, not the background one. That was the issue we faced that time.

I'm not sure I follow this comment. The original hack copied the background color value and fudged style declarations for color etc. From what I can tell, nothing was modifying the color CSS attribute except for the solution to the issue you were facing.

Would it override the user choice?

No. To the contrary this would enforce the user choice whereas the original solution effectively blocked the application of the user's color choice.

One thing I noticed was that gradients don't really have any effect. I'm wondering if should disable the control 🤔

Good question!

I'll try and dig deeper into gradients tomorrow. We'd need to make sure removing that support wouldn't break some existing uses of blocks. The fact that not all color CSS properties support gradients might further complicate things.

[ramonjd]

Using pre-built colors

Oh, I took that to mean presets? I probably jumped to a conclusion there.

[cbravobernal]

I'm not sure I follow this comment.

When the user chose a pre-defined color (like vivid-red), the setting was applied only as a CSS color attribute. The issue was, if I remember correctly, that dotted separators were not working, as they needed a background color to work.

It seems that fix interfered with the theme.json merge as you commented, and also it seems that you got it covered in the PR :-)

Sorry for not being too helpful 😅 , I think we covered that issue because we were release leads, but you are the style engine experts :-)

[aaronrobertshaw]

When the user chose a pre-defined color (like vivid-red), the setting was applied only as a CSS color attribute.

This seems to be where the confusion has come in. It's the other way around. When users choose a color the background color is set whereas the Dots style needs the color attribute set and it wasn't.

For anyone else coming to this in the future, the Dots block style sets color here in the default styles. It explicitly disables background and border here.

Appreciate the continued context, I think we're on the same page now 👍

[mikemcalister]

I just came across this bug as well in global styles in TT5. I can confirm the initial issue details and also the PR seems to fix the issue. I haven't tested extensively, but looking through my pattern collection the fix works well.