Global Styles: Fix push-to-global-styles clearing of attributes, border fallbacks, link hover colors, and behaviors #51621
Conversation
aaronrobertshaw
added
[Type] Bug
| } | ||
|
|
||
| return changes; | ||
| }, [ supports, attributes ] ); |
There was a problem hiding this comment.
Unused dependency removed to satisfy linter. I couldn't see any regressions but worth flagging in case I missed something.
| }, [ | ||
| changes, | ||
| attributes, | ||
| userConfig, | ||
| name, | ||
| createSuccessNotice, | ||
| setAttributes, | ||
| setUserConfig, | ||
| __unstableMarkNextChangeAsNotPersistent, | ||
| ] ); |
There was a problem hiding this comment.
Missing dependencies added to keep linter happy.
|
Size Change: +420 B (0%) Total Size: 1.5 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 1fcce00. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5826903699
|
|
Thanks for the PR, @aaronrobertshaw! I have tried applying various styles globally. Perhaps some of them are not included in the scope of this PR, but they are listed below. The "Apply globally" button is not enabled when I change the Block Spacing of the block. If I change other styles, that button is enabled, but it is not reflected in the global style. This problem also occurs with trunk. Recorded Videoblockgap_disabled.mp4If you apply only a border color to a block, its border will be rendered. However, if you push that style globally, the border will disappear. In trunk, if only the border color is changed, the "Apply globally" button cannot be pressed. Recorded Videoborder_disappear.mp4If you specify a custom value for the border color and push it globally, an error is logged in the browser console. In trunk, if you change only the border color, the "Apply globally" button cannot be pushed, but if you change other styles (such as margin) and push globally, the same error is logged. Recorded Videobrowser_error.mp4 |
|
I was trying the reverse and think I found a bug: setting global border styles to a group block, then applying local block border changes globally doesn't work. The bug is also in trunk, so I'm not sure what the scope of this branch is.
Screencast
|
|
Thanks for the testing and reviews. I've also just been working on a fix for pushing split borders i.e. borders with different configurations per side. Block gap isn't included in the For this PR, I think it would be best to now limit the scope to just the border styles, and address each other style/support in a follow up. |
I can replicate the application of a block instance's styles not overriding pre-existing global styles but I can't get the error at all. I'll keep digging but it might be a few days before I can devote too much time to this. Given this is bug related we should still be able to get it merged within the next GB beta period. |
There's a catch with global styles borders and theme.json set borders. If the theme json sets per side borders, the global styles also has to apply per side borders even if the user's border is "flat" and can be a simple shorthand Currently, if the border is a flat border on the instance it's still being pushed as that and so won't clear or override the per side config set by Global Styles. We could clear the Global Styles per side config but then might expose the issue if a theme.json sets per side borders. Long story, short, applying block styles globally will require |
|
I've pushed some changes that I believe fix the issues reported on earlier reviews (although I still can't replicate the error). Given there are so many different ways borders can be configured, I'd really appreciate some further thorough testing 🙏 Screen.Recording.2023-06-19.at.6.06.57.pm.mp4 |
| gradient: undefined, | ||
| fontSize: undefined, | ||
| fontFamily: undefined, | ||
| style: cleanEmptyObject( newBlockStyles ), |
There was a problem hiding this comment.
Only clearing the individual border side properties would still leave empty objects in the block style attribute without this cleanEmptyObject.
| // The shorthand border styles can't be mapped directly as global | ||
| // styles requires longhand config. | ||
| if ( flatBorderProperties.includes( key ) && value ) { | ||
| // The shorthand config path is included to clear the block attribute. | ||
| const borderChanges = [ { path, value } ]; | ||
| sides.forEach( ( side ) => { | ||
| const currentPath = [ ...path ]; | ||
| currentPath.splice( -1, 0, side ); | ||
| borderChanges.push( { path: currentPath, value } ); | ||
| } ); | ||
| return borderChanges; | ||
| } |
There was a problem hiding this comment.
There's an issue where if a theme.json configures longhand border properties and the user's global styles only configured shorthand border styles, the theme.json styles would incorrectly take precedence. As a result, Global Styles applies even shorthand borders to the longhand border properties.
This block of code maps shorthand block instance border styles to the longhand paths required by global styles.
| const { user: userConfig, setUserConfig } = | ||
| useContext( GlobalStylesContext ); | ||
|
|
||
| const changes = useChangesToPush( name, attributes, userConfig ); |
There was a problem hiding this comment.
We now pass the current user global styles to assist with determining if we need to provide a fallback border style to maintain a visible border.
There was a problem hiding this comment.
If I'm not wrong, I think we've discussed a potential alternative to actually add the fallback when generating the styles (I don't remember entirely why we discarded this option). It feels to me, like this PR is kind of a hack because we're not applying the fallback properly.
I know that there are some nuances there that might make the above not easy to achieve, so not entirely a blocker but if we want to remove these hacks, I think we should probably apply the conceptual fix at some point.
I'm not sure of the alternative you are referring to. Was it setting defaults in controls from global styles? I think a blocker for that was flagged in this comment. For some history and context. The discussion began after the extraction of the border panel. Which required some follow-up fixes in #49603. That lead to the proposal to remove the "smart" border style behaviour. That PR has more in the way of explanations. Ultimately, from those discussions, we went with two fixes for the extracted panel that continued to handle the edge cases with borders:
I'd be 100% on board with a holistic fix to covering these edge cases. Unfortunately, I don't know what that fix is, whether it is getting global styles defaults in the block editor for all controls, updating the style engine or its use to cover these edge cases etc. I'm happy to close this PR if you have better ideas. I created it to try and get this feature fixed as it has had bugs since it landed (not clearing preset attributes etc, missing some styles) and is accumulating more as additional styles have been added to the style config arrays in the block editor package and not here. |
|
Thanks for updating the PR @aaronrobertshaw I've retested the changes using Featured Image and Group blocks in the site editor. Applying a block's styles globally, I can:
It's working well for me. Haven't run into any issues so far. ^ "styles": {
"blocks": {
"core/group": {
"border": {
"color": "#000000",
"style": "solid",
"width": "10px"
}
}
}
},
|
|
Just noting that I'm not suggesting that we change anything in this PR. I think if you're all satisfied with it please continue and merge. Mainly trying to understand the problem better to see why it's so complex. Here's my mental model,, just sharing how I think about this problem and what might be an alternative solution. We have two places where we generate styles based on "user input": 1- Global Styles (theme.json) root level When do we need the fallback? (this is where I'm not sure about my answer, so correct me here because the logic depends on it)
Or maybe the correct fallback by taking into consideration the edge cases is:
So we could move the fallback logic to the style generation if we have access to global styles from the block supports it seems, right? |
|
Thank you for expanding on things @youknowriad 👍
We need to potentially apply a fallback on all three places users can set styles.
Use Case 1 - Global Styles - root level - set by userWe still need a fallback here if nothing is set in the theme.json. The UI control handles this as it should get the theme.json value as a default on the control. Use Case 2 - Global Styles - block level - set by userAlmost the same as Use Case 1. We need a fallback if nothing has been set yet, either by theme.json or the user's global root styles Use Case 3 - Block Styles (block supports): individual block levelWe need a fallback only if theme.json or global styles haven't set a border style. This is currently handled via a Use Case 4 - Pushing Block Styles To Global StylesSo for the use case in this PR, moving from block styles to global styles, the
Yes, if the style engine could access the global styles data and apply some logic during style generation, the fallback could be applied there. The ability to add some logic to the style generation would also be useful to clean up the global styles for borders which currently has to be the longhand version. This is to overcome the scenario where a theme.json defines different borders per side (longhand). In this case, if a user sets a uniform flat border (shorthand), the theme.json's longhand styles take precedence overriding the user's choice. Effectively, we need to conditionally generate styles via the style engine, or avoid merging longhand values when the user origin sets a shorthand definition. FWIW the current fallback approaches pre-date the style engine. |
|
Hey peeps, how likely is it that this can still be included in 6.3? It's a regression from 6.2 so could still be added during RC if we're confident it won't impact anything else. |
@tellthemachines I will take a look at reviving this tomorrow. It fixes bugs that have been present since the apply globally feature was introduced rather than a regression. Given it is still a bug fix, it would be good to include it if possible. Addressing the concerns around a better solution to the fallback logic can be worked on post-6.3. |
Oh, I mentioned this as a regression because I'm unable to reproduce #48048 in WP 6.2.2. That's what this PR fixes, right? Or is its scope wider than that? |
The scope is wider as there were bugs in the
The regression aspect might have been caused by #47098 which duplicated config arrays from the block editor package and those were out of date or incomplete. |
|
This PR should be ready for a final review now 🙏 The console error for clearing custom border colors has been fixed and this PR now fixes the detection of block gap changes etc. A more holistic approach to border-style fallbacks is outside the scope of this PR and will be considered a follow-up. |
aaronrobertshaw
requested review from
ramonjd,
glendaviesnz and
kevin940726
and removed request for
ramonjd
|
Thanks for your continuous work on this issue, @aaronrobertshaw! The series of processes starting with In the global style, apply the border style in one direction only. Then apply the border style on the block other than that direction and push it globally. Then the border styles are merged. Is this the intended behavior? For padding, margin and border radius it seems to Screencastborder-style-merge.mp4Border control does not allow only border style to be applied in one direction when unlinked. To select a border style, you must set either border width or border color. Screencastborder-style-unlinked.mp4Of the link colors, the default color is successfully pushed globally, but the hover state color is not. Screencasthover-color.mp4#52370 adds Behaviors to the global styles and also updates the files this PR is changing. I noticed that when I push the behaviors setting globally, it doesn't clear the block level setting. Screencastbehaviors.mp4The padding disappears when the background is pushed globally. This may be due to the has-background class disappearing at the block level. Screencastbackground-padding.mp4 |
|
Thank you @t-hamano for the thorough testing, great stuff! 🥇 I'll work through making any required updates to this PR over the next few days.
I'm not certain on what the best approach here is. I'd probably lean towards the current behaviour as the user is seeing the combined results of theme.json, global styles, and individual block styles. The push-to-global-styles, is pushing the current appearance/styling for the block so it applies to all. If the user wants to override the top border, as per your example, they'd only have to apply a I'll investigate the spacing controls and border radius behaviour as I address the other issues raised in your review.
This has been a behaviour on those controls since their inception and not really related to the pushing of global styles. So, I think that would be better suited to a follow-up where the history and context on those controls can be dug up,
Good catch. This looks like another one that was missed with the duplicated config arrays not being kept in sync. A possible follow-up to this might be trying to consolidate those via our private APIs lock/unlock mechanism.
Thanks. When I first created this PR the behaviors weren't merged as you noted. After rebasing this, I wasn't expecting to also have to fix any new additions, so missed that 🤦 I'll update this PR accordingly. The sooner we can get this in the better I think so there is a complete example that can be followed for future additions so this doesn't keep reoccurring.
I'm not sure there's an easy fix for this. That CSS class is added by the color block supports. We also don't currently have a way to conditionally generate global styles for some properties, or add extra CSS classes based on theme.json/global styles values. It's part of the reason, global border styles have to include longhand definitions, even though a shorthand definition has been applied, to ensure they correctly override any longhand definition in theme.json. For now, I'm inclined to say that is out of the scope for this PR. The user can add padding styles to what they are pushing to global styles to restore or maintain padding. If we left the color support values in place, that block would have padding but not the rest of that block type. Erring towards consistency seems like the best bet here to me. I'll update again once I've addressed all the actionable items. Thanks again for the great feedback and review 🙇 |
There was a problem hiding this comment.
I've fixed the pushing of link hover colors and the clearing of their style attribute. This was a little clunky due to the single supports key covering two separate attributes.
The behaviors only needed to have the block attribute cleared but it highlights that the behaviours and styles should probably be being processed together in a single set of changes. If you set behaviors as well as styles, then push them all globally, you'll receive two snackbar notices each with a different "undo" action.
This is better suited to a follow-up once we have this suite of fixes in place.
| if ( userHasEditedBehaviors ) { | ||
| __unstableMarkNextChangeAsNotPersistent(); | ||
| setAttributes( { behaviors: undefined } ); |
There was a problem hiding this comment.
A good potential follow-up here might be to consolidate the updating of attributes and snackbar notices for both styles and behaviours. To keep the scope of this PR manageable for testing I've opted against this larger refactor.
tellthemachines
added
the
Backport to WP Minor Release
glendaviesnz
force-pushed
the
fix/application-of-border-styles-globally
branch
from
b305a17
to
1fcce00
Compare
There was a problem hiding this comment.
Tested well for me:
✅ All typograpy, padding, margin, block spacing, border settings were removed from block settings, but still applied in editor canvas when pushed to global
✅ The settings pushed to global were applied to new blocks
✅ With global settings set first, styles applied after at block level took priority
✅ A range of single border, custom per side settings, different styles, custom colors all pushed successfully to global
aaronrobertshaw
changed the title
|
Thank you for the reviews and testing everyone, I appreciate this one got a bit gnarly 🙇 I've updated the PR description and title to also cover the link hover color and behavior fixes. |
|
This PR can't be cherry-picked cleanly onto the wp/6.3 branch because the file on trunk has diverged too much from the version on that branch (mostly I think because |
|
The fixes for the behaviour changes in this PR are very minor. Hopefully, that will mean spinning up a new PR against wp/6.3 won't take too long. I can do that tomorrow morning. |
|
Thanks Aaron! The conflicts weren't straightforward to solve at all, but you have more context on the feature than I do 😄 |
Another source for the conflicts might be the removal of the lodash function use. I've knocked up a quick draft in #53624 but am out of time, so I'll give it a proper test and once over tomorrow as promised. |
tellthemachines
removed
the
Backport to WP Minor Release
Fixes: #48048
What?
Why?
Currently, the pushing of block instance styles to global styles doesn't work for borders creating unexpected UX.
How?
linkColorneeds to check two style properties for changesTesting Instructions
Apply globallybuttonScreenshots or screencast
Screen.Recording.2023-06-18.at.7.34.45.pm.mp4