BorderControl: Replace style picker with ToggleGroupControl

[mirka]

What?

Replaces the custom implementation of the border style picker with a standard ToggleGroupControl.

Why?

Simpler code and improved consistency across similar-looking components (like animations).

How?

The behavior has been slightly modified, prompted by a limitation in ToggleGroupControl's value-handling logic (related: #47473).

As a backward-compatible way of determining whether it's being used in a controlled or uncontrolled way, on every user-triggered change the ToggleGroupControl compares its new value with the previous value, and stays in uncontrolled mode if the value was unchanged.

BorderControl uses ToggleGroupControl in controlled mode for its border style picker. It has a value sanitization feature (shouldSanitizeBorder) that resets the border style to undefined if a user tries to pick a border style without setting a border width or color. This means that, even though we need to use ToggleGroupControl in controlled mode, the value passed to ToggleGroupControl can start as undefined and still be undefined after a user-triggered change. ToggleGroupControl interprets this as being in uncontrolled mode, and now the internal value ignores the value prop.

However, even if we fixed this limitation so we can replicate the original behavior in trunk, it is actually not a good user interaction because it feels like the buttons are unresponsive.

CleanShot.2024-01-11.at.06.24.41.mp4

When the border styles can't be set, that needs to be communicated to the user in some way, rather than just have the buttons seemingly not respond to clicks. The best way would probably be to implement a disabled state for ToggleGroupControl (#57862), and perhaps couple that with help text that explains why it's disabled.

For the purposes of this PR however, I propose a more incremental workaround/improvement which is to simply hide the ToggleGroupControl until it's ready to be set.

It's also worth mentioning that the only instance of BorderControl with shouldSanitizeBorder enabled in the Gutenberg app is the BorderBoxControl in unlinked mode, so the impact should be low.

Testing Instructions

1. Go to the Storybook story for BorderControl. When shouldSanitizeBorder is true, the border style picker should only be displayed when it can actually be set.
2. Go to the story for BorderBoxControl. The border style picker should always be visible, as before.

Screenshots or screencast

CleanShot.2024-01-05.at.07.06.19.mp4

[mirka]

I refactored the logic of this sanitizeBorder function so it can be reused to determine the isStyleSettable boolean. I renamed it and inverted the logic to isValidBorder for better readability within the file.

[mirka]

Just an addition to an internal subcomponent prop — will not affect public-facing types.

[aaronrobertshaw]

Thanks for working on this @mirka!

Now the ToggleGroupControl supports deselecting an option, using that here makes perfect sense 👍

While testing, I ran into two issues.

The first is pretty minor and also occurs on trunk. In BorderControl if you set a width unit but no value, then open the color/style dropdown, you'll still get the style picker even though there isn't really a width.

The second is is a more important one from the view of the editor and Gutenberg. While originally creating these controls some of the feedback was that when overriding global styles, the border controls needed to allow a simple change to the border style without having to ascertain what the actual border values are and manually reenter them.

By forcibly omitting the style picker here, unless there is a color or width, it prevents users making those simple border style only changes.

Screen.Recording.2024-01-11.at.2.35.20.pm.mp4

I know this leaves some additional edge cases that still need to be smoothed out but unless the post editor gets all the merged global styles so they can be set as default values in the block inspector, that use case still stands.

The best way would probably be to implement a disabled state for ToggleGroupControl, and perhaps couple that with help text that explains why it's disabled

Agreed.

The proposal to update the ToggleGroupControl to support a disabled state looks the most promising to me.

[ciampo]

The behavior has been slightly modified, prompted by a limitation in ToggleGroupControl's value-handling logic (related: #47473).

As a backward-compatible way of determining whether it's being used in a controlled or uncontrolled way, on every user-triggered change the ToggleGroupControl compares its new value with the previous value, and stays in uncontrolled mode if the value was unchanged.

BorderControl uses ToggleGroupControl in controlled mode for its border style picker. It has a value sanitization feature (shouldSanitizeBorder) that resets the border style to undefined if a user tries to pick a border style without setting a border width or color. This means that, even though we need to use ToggleGroupControl in controlled mode, the value passed to ToggleGroupControl can start as undefined and still be undefined after a user-triggered change. ToggleGroupControl interprets this as being in uncontrolled mode, and now the internal value ignores the value prop.

Yeah, that's an edge case that the current logic fails to identify. I guess that using separate value and defaultValue properties is the ultimate solution here, together with not using undefined for the value prop when in controlled mode (we could instead use null, '', etc).

But that's something for a different issue / PR. Regarding this PR, I think Aaron has already written some great feedback.

[mirka]

@aaronrobertshaw Thank you for the additional context!

By forcibly omitting the style picker here, unless there is a color or width, it prevents users making those simple border style only changes.

If I understand correctly, we're specifically talking about the unlinked mode in BorderBoxControl, correct? Although the buttons are visible, they're still not responsive in trunk. This is because these BorderControls in unliked mode here have shouldSanitizeBorder enabled (whereas in linked mode it's disabled).

CleanShot.2024-01-12.at.03.34.27.mp4

(☝️ BorderBoxControl in unlinked mode in trunk)

If we want to allow users to make style-only changes, it's just a matter of disabling the shouldSanitizeBorder prop (at least at the component level).

So given that we are not taking away any functionality available in trunk, would it be reasonable to keep this PR as is, and address the two issues you raised separately?

[mirka]

Please hold off on any additional testing of this PR, because I found a bug in ToggleGroupControl (#57770) that manifests here and I'd like to get that fixed first.

[mirka]

Ok, the ToggleGroupControl bug fix was merged in trunk is now included in this PR branch. This should've eliminated any effective functional differences between the BorderControl in this PR vs. trunk (most specifically the BorderPanel as used in Global Styles, where picking a border color did not correctly re-render the border style picker with the default solid style).

This PR is ready for further review 👍

I still maintain that this delivers incrementally better UX than trunk, where the border style picker can seem unresponsive.

[ciampo]

Code changes look good overall. I'll let @aaronrobertshaw take another look (especially around how the component works in the editor) and approve in case 🚀

[aaronrobertshaw]

I still maintain that this delivers incrementally better UX than trunk, where the border style picker can seem unresponsive.

This aspect and the switch to ToggleGroupControl in general are definitely an improvement 👍

If I understand correctly, we're specifically talking about the unlinked mode
in BorderBoxControl, correct? Although the buttons are visible, they're still
not responsive in trunk.

Yes, I was referring to the unlinked border controls.

From what I recall, users were supposed to be able to simply override or set a border style value only when using the control in the block inspector panel.

Either I'm misremembering or when the global styles and block support panels were merged, something changed.

I'm a little short on bandwidth to go digging into it, at the moment, unfortunately.

I did give disabling the shouldSanitizeBorder for the split border controls a quick run and it results in empty objects within the block's attributes.

If we go down that path to sort out tweaking global border style values, the sanitization of the border returned from this control might need some improvement as well. I'm leaning slightly towards that being in the component rather than any consumer needing to be aware of that. What do you think?

[mirka]

@aaronrobertshaw I did some digging to find regressions, but the sanitization defaults and logic have been the same since the inception of these components, so I doubt that any consumer (i.e. the block editor) could've ever overriden the internal sanitization in BorderBoxControl unlinked mode. The values are sanitized before they hit the consumer's onChange handler, and we never exposed a prop to disable it. Maybe you were recalling the regression in #50498?

If we go down that path to sort out tweaking global border style values, the sanitization of the border returned from this control might need some improvement as well. I'm leaning slightly towards that being in the component rather than any consumer needing to be aware of that.

I'm not sure what exactly you have in mind, but it sounds like we want BorderBoxControl to enable sanitization when setting a global style, but not when overriding a global style? Or do they both need to be sanitized, just with different logic? I'd need more detailed specs to give an opinion on where that handling should live.

In any case, changing the sanitization defaults/logic is out of scope for this PR, so I think that should be a separate task unless you see any blockers in this PR specifically.

[aaronrobertshaw]

Sorry for the delay @mirka and thank you for digging into the history here 🙇

Maybe you were recalling the regression in #50498?

That might have been what was confusing me 👍

In any case, changing the sanitization defaults/logic is out of scope for this PR, so I think that should be a separate task unless you see any blockers in this PR specifically.

Agreed. Let's merge this and anything else can be detailed in an issue and followed up on separately.

[aaronrobertshaw]

I've given this another quick smoke test.

LGTM 🚢

[github-actions]

Flaky tests detected in a63d7ee.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7637771739
📝 Reported issues:

• [Flaky Test] Only the first block is an empty paragraph block #58161 in /test/e2e/specs/editor/blocks/paragraph.spec.js