WordPress / gutenberg

The Block Editor project for WordPress and beyond. Plugin is available from the official repository.
https://wordpress.org/gutenberg/
Other
10.51k stars 4.2k forks source link

Update `ToggleGroupControl` design #64439

Open jameskoster opened 3 months ago

jameskoster commented 3 months ago

When multiple instances of this component appear in quick succession the heavy active state add's a lot of noise and can be overwhelming. Let's explore ways to remedy this in an updated design.

jameskoster commented 3 months ago

One potential solution is to outline the active state and apply a gray background to the overall control:

mirka commented 3 months ago

Fully understanding that this is a difficult design exercise, I'll write down some requirements that ToggleGroupControl has in terms of component states:

I guess it's not off the table that we diverge the "standard" ToggleGroupControl from the "de-selectable" which is more like a button group. However, for overall consistency of look & feel, it may make sense to keep the selected item style in line with how we style aria-pressed buttons. Meaning, if we change the selected item style here, we might also want to consider changing it for pressed buttons.

Pressed toggle button in FontSizePicker
jameskoster commented 3 months ago

I guess it's not off the table that we diverge the "standard" ToggleGroupControl from the "de-selectable" which is more like a button group.

I think that's a good point to discuss. The de-selectable variant is similar to a button group as you pointed out, but also a toolbar. There might be some consolidation to do there.

For me, the base ToggleGroupControl feels different. Semantically it's more like a radio group whereas toolbars/button groups are more flexible in nature, so a different appearance can perhaps be justified.

Potential pathways for controls like Letter case include:

Here are some mockups for the states you described, plus a couple of others:

Examples
mirka commented 3 months ago

Great, thank you for integrating all the states. No more blockers from me 👍

For me, the base ToggleGroupControl feels different. Semantically it's more like a radio group whereas toolbars/button groups are more flexible in nature, so a different appearance can perhaps be justified.

Yes, I'm not opposed to this at all. The semantics and behavior are different enough that it's arguably better for them to be styled differently.

Logistically, it would be easiest if we could switch the Letter case and Decoration controls to a standard ToggleGroupControl, and deprecate the isDeselectable variant altogether, since those two are the only use cases in-app.

I'll note that the Letter case and Decoration controls actually have a "None" option which is deselectable. But the deselectable here shouldn't be strictly necessary, because they're part of a ToolsPanel, which provides the "Reset" functionality. The font size control ToggleGroupControl for example is reset through the ToolsPanel dropdown menu. I think we should be allowed to use the same mechanism for Letter case and Decoration, no?

Decoration control with deselectable none option
jasmussen commented 3 months ago

Appreciate the exploration. The main item to solve here is that the dark toggled state can give prominence to the control itself, which in some cases is not desired. This one achieves contrast through the border, though marinating on this, it loses a little bit of the segmented feel, where each option is mutually exclusive to the others. This is a bit more visible in the icon example. There's something to the idea, though, so let's continue to think about how we might thread the needle, perhaps across all the componentry. Maybe it's not possible, that's fine too.

jameskoster commented 3 months ago

The main item to solve here is that the dark toggled state can give prominence to the control itself, which in some cases is not desired.

Is this not solved in the mockups above? There may be other options to try – happy to continue exploring / create more mockups – but I don't know that we should let 'perfect' be a blocker to improvement here.

Logistically, it would be easiest if we could switch the Letter case and Decoration controls to a standard ToggleGroupControl, and deprecate the isDeselectable variant altogether

This sounds like a reasonable path forwards to me.

jasmussen commented 3 months ago

It's solved, yes, but on reflection, one past instinct you shared (maybe in chat) was that this perhaps looked a bit too much like an input after all, making this specific issue not a clear win. I'm also thinking that this can be a low-boil set of explorations that consider not just this toggle, but several of them together, with no rush. What do you think?

jameskoster commented 2 months ago

That kind of assumes other controls will be changing significantly which seems unlikely in the short term. So it seems a shame for this one detail to hold up any improvements, especially as it's not 100% clear it is actually an issue.

I do agree this isn't high priority though.

mirka commented 1 month ago

I noticed this bug #62981 which should be fixed sooner rather than later. We can fix the functionality part without a design change, but ideally we should have a design where a focus ring can be shown on a single option item without it being selected (like a radio group that doesn't have a selected item yet).

jameskoster commented 1 month ago

Focussed with no selection could look something like this:

focussed no selection

In terms of the individual options, I think they only need focus styles when disabled, right? Otherwise the parent should retain focus?

ciampo commented 1 month ago

In terms of the individual options, I think they only need focus styles when disabled, right? Otherwise the parent should retain focus?

Imagine having a mix of enabled and disabled option items — wouldn't it be weird if the focus rings moved between the whole group (when an enabled option is focused) and a single option (when it's a disabled option?)

I wonder if we could keep the design proposed above, and show a focus ring also on "enabled" tabs:

Screenshot 2024-09-16 at 12 19 00

We also have a dedicated issue for this specific aspect: https://github.com/WordPress/gutenberg/issues/63612

mirka commented 1 month ago

Yes, I think we're going to need to drop idea of the entire control having a focus ring. When there are no options selected and there's only a focus ring on the entire component, there's not enough affordance for the user to understand what the control does. The first option should get the focus ring, much like a radio group or any other Composite-based control.

jameskoster commented 1 month ago

So how would the keyboard interaction work for an instance with no active selection? If the first item receives focus, how would users select it? What happens if they arrowkey right without making a selection – currently this would auto-select the newly 'focused' item, wouldn't that be a bit inconsistent, especially if arrowkey left returns them to the original state and selects.

ciampo commented 1 month ago

Continuing the analogy with a radio group:

https://github.com/user-attachments/assets/da27a645-e7d8-429e-8f39-a537a1832b5a

With this in mind, here is how ToggleGroupControl could work when isDeselectable={false}:

If the first item receives focus, how would users select it?

By pressing spacebar, clicking it, or moving the selection back and forth with arrow keys

What happens if they arrowkey right without making a selection

If the next item is not disabled, pressing the right arrow key would move focus AND select that item.

If the next item is disabled, pressing the right arrow key would move focus to that item, but without making it the new selected item (since it's disabled).

currently this would auto-select the newly 'focused' item, wouldn't that be a bit inconsistent, especially if arrowkey left returns them to the original state and selects.

Not sure I follow entirely this last point, but I think that my replied above should have clarified the behaviour anyway.

In short, it would be very similar (if not identical) to how tab selection works in Tabs when the selectOnMove prop is true.

@mirka what do you think about this proposed behaviour?

mirka commented 1 month ago

@ciampo I think that makes sense.

In the past, I felt strongly that there should be a clear visual distinction between the deselectable and non-deselectable variants, thus showing a focus ring on the entire component when non-deselectable. But I no longer think that's possible 😕

jameskoster commented 1 month ago

Okay, last question is about disabled states. When all options are disabled can you still focus through each item? Arrow left / right would move focus but not select. Does this mean it's not possible for the component itself to be disabled, only individual options?

I think this would cover all the states we've discussed:

Screenshot 2024-09-17 at 13 34 13
ciampo commented 1 month ago

When all options are disabled can you still focus through each item? Arrow left / right would move focus but not select.

It would be a bit of a weird state, but technically yes — i'd expect each disabled item to be focussable, but not selectable.

Does this mean it's not possible for the component itself to be disabled, only individual options?

I would probably also allow for the whole item to be disabled, as a conceptually separate thing.

I guess that a disabled ToggleGroupControl would still be focussable as a whole, but the user wouldn't be able to focus the individual options — ie. the focus could stay on the whole group, in this case. What do y'all think?

jasmussen commented 1 month ago

I like this one, but there are some details about it I'd love to tinker with to get it just right. I think it's mainly the gray backdrop that isn't fully gelling quite yet. Perhaps after the beta period we can seek out the way to thread the needle fully?

jameskoster commented 1 month ago

Please go ahead and tinker! Here's the Figma.