ToggleGroupControl: support disabled options

[ciampo]

What?

Related to #57862
Closes #57862
New take on #34945

• Adds support for disabled option for the ToggleGroupControl component
• Adds styles for disabled options

Why?

This was a missing feature in the component, a gap that should be filled.

How?

By forwarding the disabled prop to the correct internal components in the ToggleGroupControlOptionBase component.

Testing Instructions

• Review new unit tests and make sure they pass reliably;
• Edit the Storybook examples, by passing the disabled prop to some options, and interact with the component. Make sure that disabled option are correctly skipped and impossible to select

Here's a quick patch to test the storybook examples

diff --git a/packages/components/src/toggle-group-control/stories/index.story.tsx b/packages/components/src/toggle-group-control/stories/index.story.tsx
index 92f1e60762..3b9b9a400f 100644
--- a/packages/components/src/toggle-group-control/stories/index.story.tsx
+++ b/packages/components/src/toggle-group-control/stories/index.story.tsx
@@ -81,7 +81,7 @@ Default.args = {
 	children: [
 		{ value: 'left', label: 'Left' },
 		{ value: 'center', label: 'Center' },
-		{ value: 'right', label: 'Right' },
+		{ value: 'right', label: 'Right', disabled: true },
 		{ value: 'justify', label: 'Justify' },
 	].map( mapPropsToOptionComponent ),
 	isBlock: true,
@@ -125,7 +125,12 @@ WithIcons.args = {
 	...Default.args,
 	children: [
 		{ value: 'uppercase', label: 'Uppercase', icon: formatUppercase },
-		{ value: 'lowercase', label: 'Lowercase', icon: formatLowercase },
+		{
+			value: 'lowercase',
+			label: 'Lowercase',
+			icon: formatLowercase,
+			disabled: true,
+		},
 	].map( mapPropsToOptionIconComponent ),
 	isBlock: false,
 };

Screenshots or screencast

Before (trunk)	After (this PR)
tgc.before.mp4	tgc.after.mp4

✍️ Dev note

ToggleGroupControlOption components can now be marked as disabled via the newly added disabled prop. While disabled, an option cannot be selected by the end user.

[github-actions]

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 props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: ciampo <[email protected]>
Co-authored-by: mirka <[email protected]>
Co-authored-by: tyxla <[email protected]>
Co-authored-by: stokesman <[email protected]>
Co-authored-by: jasmussen <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[ciampo]

@ramonjd just making sure you're ok with those food options

[ramonjd]

Sorry, I was away last week.

I would have preferred risotto instead of just "rice", but I appreciate you asking. 😆

[ciampo]

Ah, good point. Feel free to open a PR

[ciampo]

I don't think it makes sense to have the options accessibleWhenDisabled given how the component works cc @mirka

[mirka]

I actually thought this was one of the cases where we always want the option to be focusable when disabled, similar to ToolbarButton (#63102) or tabs in a tablist. It's also a composite widget where there will be a limited number of options, and the number of tab stops it potentially adds to the tab sequence is either zero or negligible.

Implementing that in the radio case for ToggleGroupControl would require some additional design work though 🤔 But at least Ariakit supports the behavior nicely (StackBlitz demo).

[ciampo]

Implementing that in the radio case for ToggleGroupControl would require some additional design work though

Yep. Currently, since disabled options are not really supported, the backdrop indicator also works as a focus indicator.
If we make the options accessibleWhenDisabled, then we'll need to add separate focus rings.

But at least Ariakit supports the behavior nicely (StackBlitz demo).

Yup. We'll probably need to add the custom logic when rendering individual buttons

[stokesman]

Just going off the screen recording, it’s easy for me to see that an option is disabled 👍. Still, I wonder if the disabled opacity makes the text too light for a11y guidelines. Unless that’s not required of a disabled state but isn’t it?

It’s a thread-the-needle thing because even the enabled text is not that high contrast so lesser opacity will make it harder to see that an option is disabled. If the contrast is an issue perhaps we can come up with something else but given the current lack of usage for this feature it’s probably not too grave a concern. EDIT: I saw the existing use case Lena noted.

[mirka]

@stokesman FWIW, inactive controls are exempt from the contrast guidelines.

[tyxla]

Looks good and tests well 👍

Do we need a design ✅ just in case?

Thanks!

[tyxla]

We have more and more of these disabled styles that do opacity: 0.4. Perhaps this is a good opportunity to generalize at some point.

[ciampo]

Agreed, although this PR is not the right place for it. We should probably assess the current state of shared config in the package, and decide next steps. For example, some variables come from the base-styles package, some are internal to the components package. Some variables are defined in Sass, some as CSS custom properties, and others in a JS object.

[tyxla]

Definitely should be a separate PR 👍

[jasmussen]

Seems good to me, 0.4 opacity is also what appears to be used elsewhere.

[ciampo]

Thank you for all the comments!

I'll look into making disable options focusable and using Tabs-like focus styles.

I'll ask for a new round of review once that's completed

[ciampo]

I applied changes to make disabled options keyboard-accessible (ie. focusable).

The main design change is adding a focus ring for focused, disabled radio options, since the active indicator (backdrop) should not follow focus in that case.

I've also updated tests to reflect the new changes.

tgc.disabled.but.accessible.mp4

[ciampo]

The focus ring is applied to the after pseudo-element so that we can control its size via the inset prop, so that we can leave a little gap between the focus ring and the backdrop indicator.

[ciampo]

Opacity is applied to the button contents, so that the focus ring can be at full opacity

[tyxla]

Nothing further from my perspective. The code looks good, and everything tests well, just as demonstrated on the video.

Thanks @ciampo 🚀

[ciampo]

Thank you @tyxla ! I'll also wait for @jasmussen 's approval specifically for the disabled+focused styles before merging.

[jasmussen]

I'm not the best to ask here, as this particular combo of focusing a component with roving focus and what looks like a classic tab-focus style is peculiar to me, specifically because the arrow-key to focus the disabled option still appears to have the previous item selected. It looks like you have two items focused at once. Is there any prior art to look at? What do other segmented controls with disabled options do with their roving focus?

My instinct would go towards the backdrop still animating to the disabled item.

But if this is the best practice, by all means. I wouldn't think this is a pattern that should be used in the block editor, even if the component supports it.

[ciampo]

@jasmussen it's a tricky one, indeed!

because the arrow-key to focus the disabled option still appears to have the previous item selected.

That's exactly what is going on: focus is on a disabled option, which can't be selected (because disabled) — and therefore the previous item is still selected.

It looks like you have two items focused at once.

Yeah, that's a downside of the current approach. Here is what it looks like when showing the "focus ring" for every focused option (not just disabled):

Kapture.2024-07-15.at.17.24.05.mp4

Is there any prior art to look at? What do other segmented controls with disabled options do with their roving focus?

Other libraries

• NextUI's tabs shows both a focus ring and a visual indicator, but the disabled tab behavior seems buggy (the tab is still selected even when disabled)
• Material UI, Radix, Tamagui, Ant Design don't seem to allow disabled options to be focusable in the first place
• Ariakit and React Aria Components don't seem to have a similar example

Similarities with Tabs

Tabs is quite similar:

• it uses an indicator to show the currently selected item
• its tabs are keyboard-focusable even when disabled
• when a tab is disabled but focused, we keep the "active tab" indicator on the active item, and use a focus ring to show keyboard focus
• this visually works because the focus ring doesn't overlap with the indicator

My instinct would go towards the backdrop still animating to the disabled item.

I hacked together a preview, by having the backdrop animate to the disabled item, but with a faded opacity to convey the sense that the option is disabled. It looks promising:

Kapture.2024-07-15.at.17.59.25.mp4

We'd need to tweak the logic of the component, since currently the backdrop indicator follows the active item, instead of the focused item.

We will also need to refine some aspects of the design and animation, to make sure that the text opacity and color changes are well synced with the backdrop animation.

[ciampo]

I reflected on this a bit more:

1. The "backdrop still animating to the disabled item" solution visually looks the slickest, but it has one big flaw: the indicator becomes a focus indicator (instead of a "selected item" indicator), and the user doesn't have a way of knowing what is the selected value when focus is moved to a disabled item;

Kapture.2024-07-15.at.17.59.25.mp4

2. The current implementation is confusing because it shows a focus ring only on disabled items, and thus the user may think that there are two focused items at once, and/or that they lose the keyboard focus when moving to a non-disabled option (plus, it's also incoherent visually)

3. Always showing a focus ring (like we do in Tabs) feels like the most correct and less confusing option, but it conflicts with the design of the component.

Kapture.2024-07-15.at.17.24.05.mp4

---

In conclusion, I think that the better choice, given the circumstances, is not to allow disabled option items to be keyboard focusable in ToggleGroupControl.

What do y'all think ? @mirka @tyxla @jasmussen

[jasmussen]

The specific behavior of this one is that I set focus on the segmented control itself, and now I can use arrow keys to choose an option. Tab or shift tab moves me to the next or previous focusable item:

This behavior is important to retain.

[tyxla]

This behavior is important to retain.

@jasmussen if I understand correctly, the focus between components shouldn't be affected. This is about moving between tabs, which is performed with the arrow keys.

In conclusion, I think that the better choice, given the circumstances, is not to allow disabled option items to be keyboard focusable in ToggleGroupControl.

From https://www.w3.org/TR/wai-aria-1.0/states_and_properties#aria-disabled:

irrelevant options in a radio group may be disabled. Disabled elements might not receive focus from the tab order.

@ciampo in theory, yes, skipping focus for disabled tabs is a possible path forward. However, I'm a bit concerned that if disabled tabs are not focusable, users with visual impairment will miss that the button even exists. Instead, if we keep the button focusable, the assistive technology will inform them that they're working with a disabled tab.

As a potential solution, I wonder if we could perhaps make the focus of disabled tabs lighter than the regular one?

[jasmussen]

We'll have the same challenge with the TabPanel, it's also designed to be a single focusable, with arrow-key navigation.

I'm not against capturing edgecases that are theoretically used by 3rd party consumers, even if not by the block editor itself yet, but it's important we keep the key flows the components were designed for.

[ciampo]

The specific behavior of this one is that I set focus on the segmented control itself, and now I can use arrow keys to choose an option. Tab or shift tab moves me to the next or previous focusable item [...] This behavior is important to retain.

@jasmussen this behavior is not changing. What needs to be decided is, with regards to ToggleGroupControl:

• should disabled options be keyboard focusable?
• if so, how do we visually let the user know "where is the keyboard focus" vs "what item is currently selected" ?

in theory, yes, skipping focus for disabled tabs is a possible path forward. However, I'm a bit concerned that if disabled tabs are not focusable, users with visual impairment will miss that the button even exists. Instead, if we keep the button focusable, the assistive technology will inform them that they're working with a disabled tab.

I agree, but (as in many other situations) we need to pick the best compromise for a better overall UX.

As a potential solution, I wonder if we could perhaps make the focus of disabled tabs lighter than the regular one?

@tyxla I'm not sure I understand what you're proposing?

We'll have the same challenge with the TabPanel, it's also designed to be a single focusable, with arrow-key navigation.

@jasmussen we already solved this challenge in TabPanel (which will soon be deprecated in favour of Tabs):

• we show a focus ring to indicate where keyboard focus is (even on disabled tabs)
• we use an underline to indicate the currently selected tab

Kapture.2024-07-16.at.11.50.48.mp4

It's tricky to apply the same treatment to ToggleGroupControl because the indicator is a full-height backdrop and would visually clash / overlap with the focus ring. I tried this approach in a previous message:

Kapture.2024-07-15.at.17.24.05.mp4

[jasmussen]

Understood, and I'm not the right person to ask there. As I understand, it appears to be a best practice for disabled buttons to still be focusable. I understand this especially to be the case where, for example, you press "Reset" on a button, where this same action also disables the button as there are no more filters to reset. In that case, it needs to be focusable so as to not cause a focus loss. But in the case of a segmented control, I'm struggling to come up with cases where we'd want to show a segmented control that has disabled options.

[tyxla]

in theory, yes, skipping focus for disabled tabs is a possible path forward. However, I'm a bit concerned that if disabled tabs are not focusable, users with visual impairment will miss that the button even exists. Instead, if we keep the button focusable, the assistive technology will inform them that they're working with a disabled tab.

I agree, but (as in many other situations) we need to pick the best compromise for a better overall UX.

Okay, I think we can take this step-by-step. This PR aims to add support for disabled options, which it does. I suggest that we open an issue for discussing the disabled option focus issue, and move forward with keeping it non-focused for now. After all, as @jasmussen also said, disabled options will likely be a rare use case.

[ciampo]

I merged this PR while keeping disabled option items non-accessible, and opened a new issue to continue the conversation.