[RadioGroup] Add `disabled` prop to `Root`

[YarnSphere]

Description

Allows setting disabled on the root RadioGroup, setting all Radio items as disabled. ~Individual Radio items can still set disabled={false} to override the group's setting.~

The first commit simply fixes a typo that I noticed, hope that's okay to include.

[benoitgrelard]

Individual Radio items can still set disabled={false} to override the group's setting.

I don't think this should be possible, I think if you set it on the root, it should always override the items.

• it's confusing otherwise
• the whole point I feel is to disable the whole control, so there isn't much reason to ever do so anyway
• it's not consistent with how similar APIs work on the DOM (like setting disabled on optgroup, you can't un-disable a specific item then)

[YarnSphere]

My reasoning was 2-fold:

• At least one other library I was testing (Chakra) does this, not that this means much.
• The current behaviour is simply a "shorthand" to manually setting disabled on each item, so it made sense to be able to override on individual items if needed.

I agree with your reasoning, though. I'll change the implementation to do that. I'll further set data-disabled on the root, then, and I was thinking of adding a tabIndex={-1} to the root as well, what are your thought on this?

[benoitgrelard]

From Discord:

@benoit Testing the disabled on the radio-group's root, I noticed something on the current radio-groups that I'm not sure is wanted: why does the group itself have tabIndex="0"? This means that the group will catch focus even if all radios within are disabled.

In a "vanilla" radio group this doesn't seem to happen. Is this behaviour correct? just tested a bit more and this might make sense, it seems like in a "vanilla" group if the currently selected radio is disabled, it doesn't let me tab into the whole group because it tries to tab into the selected one only

Yeah, that's how we handle focusing the correct item when tabbing through the page, focus lands on the group, and the groups throws it back to where it knows it should be.

That does mean that in this case, when the whole group is disabled here, we need to override that tabIndex. I think something like this to only override when we need and otherwise leave it to what is set by RovingFocusGroup:

<RovingFocusGroup.Root
  asChild
  {...rovingFocusGroupScope}
  orientation={orientation}
  dir={direction}
  loop={loop}
+  {...(disabled ? { tabIndex: -1 } : {})}
>

[YarnSphere]

This still doesn't cover the case where all items are manually set to disabled, in which case the group is still focusable. Would you like me to do something about this?

We could use the context to track the number of items vs number of disabled items and set tabIndex to -1 when all items are disabled.

[benoitgrelard]

This still doesn't cover the case where all items are manually set to disabled, in which case the group is still focusable. Would you like me to do something about this?

That's a great point!

We could use the context to track the number of items vs number of disabled items and set tabIndex to -1 when all items are disabled.

It looks like perhaps a better place to deal with this would be in RovingFocusGroup itself, if all items are "non-focusable" then we'd remove the tabIndex requirement from the top.

Then it would just work automatically for radio group here, because all the items would be made disabled/thus non-focusable from RovingFocusGroup POV.

[YarnSphere]

It looks like perhaps a better place to deal with this would be in RovingFocusGroup itself, if all items are "non-focusable" then we'd remove the tabIndex requirement from the top.

I've pushed the other changes for now, I'll try to have a go at implementing that.

[YarnSphere]

It looks like perhaps a better place to deal with this would be in RovingFocusGroup itself, if all items are "non-focusable" then we'd remove the tabIndex requirement from the top.

Then it would just work automatically for radio group here, because all the items would be made disabled/thus non-focusable from RovingFocusGroup POV.

The above is implemented.

This PR should be ready for review. A limitation of the current implementation is that if all focusable items are also hidden (display: none), then the group will still be focusable. I think that handling this edge case might not be worth it.

[YarnSphere]

Rebased against main. This should be ready for review again.

[YarnSphere]

I think the release should be marked as minor instead of patch as it introduces a new API and new behaviour. I'll change the yarn versions file to reflect that.

[YarnSphere]

I marked all changes as minor, let me know if I should mark some dependencies as patch instead.