Global Styles: Fix Separator block color use #67269
Conversation
aaronrobertshaw
added
[Type] Bug
|
Size Change: +1 B (0%) Total Size: 1.83 MB
ℹ️ View Unchanged
|
| * selection of a background color should be applied to the other paths | ||
| * so it can be honoured. | ||
| */ | ||
| $separator_color = $config['styles']['blocks']['core/separator']['color']['background'] ?? null; |
There was a problem hiding this comment.
This reminds me to brush off the theme json style-related methods abstraction issue
|
In e71fd97 I've taken a run at removing the fudging of style declarations for the separator block in favour of a theme.json data based approach. There's a slight change in logic when it comes to merging the theme origin data (i.e. the separator styles when defined in theme.json). Previously, the The theme origin tweaking of separator color only needs to happen on the PHP class side in The user origin data tweaks still need to occur on the JS side too. This is due to the editor needing to handle unsaved selections made by the user in the Global Styles UI. I've removed the old @ramonjd, this isn't a high priority but if you get the chance, let me know what you think of the general approach now. If you think it has legs, I'll take this out of draft status and cast the net wider for alternative ideas. |
|
Flaky tests detected in ba610d7. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/12114764110
|
I like that the code is now in existing internals. I know it's not always easy in practice, but the fewer bespoke private functions we have in theme.json the better.
These are the changes in I think that's sound reasoning. From looking at the changes, it's doing its best to honor the theme.json values and safe-guard against unexpected separator colors. Overall, I think it's a good direction and you've balanced the competing priorities. Off the top of my head, no backwards compat issues occur to me, but hopefully wider testing will wash that out. Thank a lot for persisting with this. What do you think? Should we try to get eyes on it early in 6.8 cycle? |
|
Forgot to add that I tested the PR too in TT4 and TT5 and it works as expected 😄 |
That's correct. I should have been clearer on that point. There was previously no consideration of which origin the data was coming from. Which in part, was the cause for global styles selections not being honoured.
💯 Agreed. I'll give the PR, its description, and test instructions a bit of a polish and flag for review later this afternoon. Thanks for all the sanity checking around the approach 🙇 |
aaronrobertshaw
changed the title
aaronrobertshaw
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. |
|
I've added a more detailed breakdown of all the changes to the PR description and flagged as ready for review. I'll sort out the backport shortly and then ping for a few reviews. |
|
Backport is available (WordPress/wordpress-develop#7927) and changelog added here. |
aaronrobertshaw
requested review from
talldan and
tellthemachines
and removed request for
tellthemachines
| * 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; |
There was a problem hiding this comment.
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 🤔
There was a problem hiding this comment.
I'll dig into this one tomorrow and see if there's anything we can smooth out.
There was a problem hiding this comment.
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
backgroundCSS property - The Separator block's
hrelement will inheritcurrentColorfor itscolorCSS 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
colorproperty effectively masking any gradient applied here - If the
backgroundproperty has alinear-gradientvalue at all, copying that toborder-colororcolorresults in an invalid CSS rule and falls back tocurrentColor - 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
gradientisn't retrieved and processed like thebackgroundColorvalue - when
customColoris determined it only takes into accountstyle.color.backgroundforgetting aboutstyle.color.gradient
- There's one default theme (TwentyTwenty) that appears to leverage
linear-gradientwhile 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
hrelement 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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
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: It seems like the color and background color are being overridden, but not the border color: |
|
Thanks for testing @talldan 👍
This was another oversight in the original implementation in #38428 in addition to mistakenly (in my opinion) adopting gradient support. Further details from some recent digging can be found in #67269 (comment). If my understanding is correct, the noted behaviour is consistent with trunk. My focus for this PR was fixing Global Styles. If we're changing the application of background to border-color on the block instance, it will need a deprecation as well. Do you think this is ok as a follow-up? |
Yep, sounds good if you think it's best to split it across multiple PRs. |
|
Given it's taken me a while to get back to this, I've pushed what I think could be a fix to this PR for the block instance styles not appearing to take effect correctly in 4074641.
I went with a Let me know what you think, whether you see any pitfalls to this approach etc 🙏 |
| // 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; | ||
| } |
There was a problem hiding this comment.
Just so I understand:
- if the separator block has a background color set, the
colorproperty 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, throughcurrentColor
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:
- In TT4, change the background value of Wide Line variation in global styles
- At the block level, apply Wide Line variation, and overwrite the background color
Do block style variation global styles need the same treatment? 🤔
- In TT4, change the background value of Wide Line variation in global styles
- 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);
}
Related:
What?
This PR removes problematic fudging of Separator block style declarations in favour of filtering the actual theme.json data at both the theme and user origin levels within the theme.json resolver. This approach improves color consistency and flexibility across both origins while also making the Separator block styles more filterable and standardized.
Why?
Global Styles currently only sets a single property in the user origin theme.json data,
background. This means if a theme sets theborder.colororcolor.textproperty, thebackgroundcolor isn't applied consistently.How?
Detailed Breakdown
lib/class-wp-theme-json-gutenberg.phpWP_Theme_JSON_Resolver::get_theme_data()lib/class-wp-theme-json-resolver-gutenberg.phpget_user_data) and theme (get_theme_data) level.color.background,color.text, orborder.color)color.backgroundvalue has been defined it is used as a fallback forcolor.textandborder.colorif they haven't been defined.packages/block-editor/src/components/global-styles/use-global-styles-output.jspackages/editor/src/components/global-styles-provider/index.jsphpunit/class-wp-theme-json-resolver-test.phpphpunit/class-wp-theme-json-test.phpInitial context from early explorations into a fix
Approach
The initial idea was to enforce the user selected color from Global Styles, i.e.
backgroundin the user origin. That evolved to perhaps do so in a way that wouldn't mean that it was also saved to the CPT. This didn't really seem achievable as it would mean having to instantiate additionalWP_Theme_JSON_Gutenberginstances which come with not insignificant overhead.To simply have something for testing purposes, I tweaked the retrieval of user origin data in the theme.json resolver class and where the user and theme origin data is merged on the JS side. If we pursue an approach of enforcing the background color across multiple properties, the points at which it is done in JS and PHP should be the same e.g. during merge, or retrieval of user data etc.
The same consistency should also be applied to the existing fudging of separator data in
useGlobalStylesOutputand the style declarations inWP_Theme_JSON_Gutenberg.Alternatives
wp_theme_json_data_userfilter and set things there. The equivalent in JS could be done inuseGlobalStylesContextoruseGlobalStylesUserConfig.Testing Instructions
background,border-color, andcolor.Replicating original issue on trunk
Screenshots or screencast
Excuse the bright yellow color in these screenshots, it was the easiest option to highlight the pre-existing theme.json color impacting the Global Styles selection.
Before
After