Add radio option to ButtonGroup

[ajlende]

Description

This adds an option to make the ButtonGroup behave as a radiogroup by using the prop mode="radio".

const MyRadioButtonGroup = () => {
	const [ checked, setChecked ] = useState( 'medium' );
	return (
		<ButtonGroup mode="radio" onChange={ setChecked } checked={ checked }>
			<Button value="small">Small</Button>
			<Button value="medium">Medium</Button>
			<Button value="large">Large</Button>
		</ButtonGroup>
	);
};

The behavior is consistent with what the design guidelines describe.

Accessibility

And it follows WAI-ARIA 1.1 Radio Group Practices (not nested in a toolbar) for accessibility.

Keyboard Interaction

• Tab and Shift + Tab: Move focus into and out of the radio group. When focus moves into a radio group:
  • If a radio button is checked, focus is set on the checked button.
  • If none of the radio buttons are checked, focus is set on the first radio button in the group.
• Space: checks the focused radio button if it is not already checked.
• Right Arrow and Down Arrow: move focus to the next radio button in the group, uncheck the previously focused button, and check the newly focused button. If focus is on the last button, focus moves to the first button.
• Left Arrow and Up Arrow: move focus to the previous radio button in the group, uncheck the previously focused button, and check the newly focused button. If focus is on the first button, focus moves to the last button.

WAI-ARIA Roles, States, and Properties

• The radio buttons are contained in or owned by an element with role radiogroup.
• Each radio button element has role radio.
• If a radio button is checked, the radio element has aria-checked set to true. If it is not checked, it has aria-checked set to false.
• Each radio element is labelled by its content, has a visible label referenced by aria-labelledby, or has a label specified with aria-label.
• The radiogroup element has a visible label referenced by aria-labelledby or has a label specified with aria-label.
• If elements providing additional information about either the radio group or each radio button are present, those elements are referenced by the radiogroup element or radio elements with the aria-describedby property.

* Not sure if the last ones can really be enforced here except by throwing warnings into the console. They might just be up to the user to add.

How has this been tested?

Screenshots

Types of changes

New feature

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• My code has proper inline documentation.
• I've included developer documentation if appropriate.
• I've updated all React Native files affected by any refactorings/renamings in this PR.

[ItsJonQ]

@ajlende Very cool!! 😍

As far as a11y, I'm not sure how to correctly handle ButtonGroups x Radio. I've worked on them individually, but I've never combined the functionality.

@diegohaz I'd love to hear your thoughts :) (I just poked around Reakit for inspiration)

I took a quick peek at the code. Instead of using cloneElement, what do you think about maybe using React.createContext instead? Maybe we can have an internal ButtonGroup.Context. Something that Button can use.

It will achieve the same effect, but I think this code path will be clearer and more stable.

Thanks!!

[ajlende]

When I'm thinking about a11y, I think about the intention behind an interaction. It could just as easily be a horizontal radio group, but it just happens to be styled as a button group instead which is why I think the keyboard and aria attributes should reflect that. 🤷‍♂

And thanks for the suggestion! The implementations I found in the wild that used children were Base Web and Mineral UI which is probably why I thought of cloneElement instead of something else. I'll take a look at trying useContext instead to see how that reads/feels.

[gziolo]

Awesome work here. Thank you for taking care of the experience around radio groups that should be used in more places in Gutenberg.

While on the visual level and accessibility-wise everything seems to be great, I'm a bit on the fence whether adding extra functionality to the ButtonGroup component is the best approach here. The note added For a traditional radio group, use a RadioControl component. made me think about it in the first place.

The question is whether we can make RadioControl flexible enough to work with button groups or toolbar groups and keep the existing API as is but add whatever is necessary to achieve such a goal. I can share an updated example from Reakit that present how such customization could be materialized, based on https://reakit.io/docs/radio/#default-value:

function Example() {
  const radio = useRadioState( { state: "orange" } );
  return (
    <RadioControl { ...radio } aria-label="fruits" as={ ButtonGroup }>
      <Radio { ...radio } label="Apple"  value="apple" as={ Button } />
      <Radio { ...radio } label="Orange" value="orange" as={ Button} />
      <Radio { ...radio } label="Watermelon" value="watermelon" as={ Button } />
    </RadioGroup>
  );
}

The challenge is that it's far from how RadioControl works as of today :(

@diegohaz might have better ideas what's the best way to approach it. In general, I'd be in favor of introducing a separate component for this use case rather than adding conditions to the existing ButtonGroup component.

[ajlende]

The Reakit approach is interesting. I'm not a fan of having to pass a bundle of props to everything, but separating out the behavior from which components are rendered is cool. It seems that approach is very core to how Reakit works—it's a different paradigm than what I've seen around here.

I should note that the behavior of a radio group in the context of a toolbar is different from on its own, like here. So if we did take that approach, you wouldn't be able to style RadioControl as a toolbar. You'd need a separate toolbar behavior which could get styled as you see fit.

I saw in #16514 the desire to also have checkbox functionality added. With the way it is now, that would be achieved by adding a checkbox mode. There isn't an existing checkbox group that it would map nicely onto like this could map to radio control, but it could easily just be another separate component too.

[ItsJonQ]

@ajlende Wowow! Thank you for these updates 😍 .

I tested it locally, and it's working as advertised. I do like how simple this configuration is to set up (from a consumer perspective). I think using Context feels better!

There's something about the children mapping that I feel like can be improved. However, I'm not too sure what yet. I gave it a shot today (separately), but was unable to come up with a better solution.

cc'ing @diegohaz to see if he has any thoughts 😊

[diegohaz]

Thanks for the work @ajlende! I'm following this and researching more about radio/checkbox groups. Will have a deep look into the code very soon.

[diegohaz]

Thanks for the great work, @ajlende!

Most of the things I tested work really well:

• If there's no default checked value, moving focus into the radio group doesn't check them.

• VoiceOver can move between radio buttons using VO+Arrow keys without checking them (needs VO+Space to check).

• Since radio buttons are being registered by their index in the children array, rendering radio buttons in a late render doesn't mess with their order (when using arrow keys), which is really cool! :)

  

Nice job! 👍

Things I missed:

• There's no RTL support. Arrow keys will behave in the inverted order when rendering the button group in an RTL context. I'm not sure we fully support RTL though.

• The fact that it needs each radio button to be a direct child may be a problem. This wouldn't work, for example:

  const OtherOptions = () => (
  	<React.Fragment>
  		<Button value="medium">Medium</Button>
  		<Button value="large">Large</Button>
  	</React.Fragment>
  );
  
  
  const MyRadioButtonGroup = () => {
  	const [ checked, setChecked ] = useState( 'medium' );
  	return (
  		<ButtonGroup mode="radio" onChange={ setChecked } checked={ checked }>
  			<Button value="small">Small</Button>
  			<OtherOptions />
  		</ButtonGroup>
  	);
  };

  I believe we may have places where we would need to abstract some options into separate components.

I also think that, instead of updating ButtonGroup and Button, which will eventually not scale well, this should be a new component (RadioGroup?).

In case you need any reference, I was playing with combining Reakit's Radio component with @wordpress/components' ButtonGroup. ButtonGroup still needs to implement React.forwardRef internally, but it looks pretty good: https://codesandbox.io/s/reakit-radiogroup-wordpress-buttongroup-9wk9b

[gziolo]

I believe we may have places where we would need to abstract some options into separate components.

I also think that, instead of updating ButtonGroup and Button, which will eventually not scale well, this should be a new component (RadioGroup?).

In case you need any reference, I was playing with combining Reakit's Radio component with @wordpress/components' ButtonGroup. ButtonGroup still needs to implement React.forwardRef internally, but it looks pretty good: https://codesandbox.io/s/reakit-radiogroup-wordpress-buttongroup-9wk9b

It aligns with my previous comment. It might make sense to have a separate RadioGroup as you did it in the example, but the idea stays the same. Using as prop allows passing ButtonGroup as it exists today and the RadioGroup takes care of all the accessibility logic. It also means that we can refactor RadioControl to use RadioGroup and Radio internally to share as much code as possible. Do I get it right, @diegohaz?

Aside, it's time to bring more low-level APIs from Reakit. I see that we share too much logic between those two projects and obviously Reakit has way much better testing coverage because it's more targeted. I personally admire the whole composability patterns used there that make all those integrations possible.

[ajlende]

Alright, I think this last batch of changes should have covered all the feedback so far. RTL doesn't work right now as mentioned earlier, and I held off from refactoring RadioControl in this PR.

@diegohaz I appreciated the codesandbox! Using Reakit was easy enough to get working once I realized it had been updated on master. Having the interactions nicely bundled like that is really nice. 🙂

[diegohaz]

Tested it on Safari/VoiceOver and it's working very well! Great job! 🤩

Made a few comments and suggestions.

[diegohaz]

It looks good to me! I miss some unit/integration tests, but they can be added later while this is experimental. Other folks may have a different opinion though.

Thanks for working on this! 🚀

[diegohaz]

Very clever! 👍

[gziolo]

I miss some unit/integration tests, but they can be added later while this is experimental. Other folks may have a different opinion though.

I never think of it as a blocker. However it comes with the assumption that it’s going to be tackled in the follow-up unless there are reasons not to add more tests. I can’t see any at the moment. Now that we have React Testing Library integrated it should be only easier to write tests.

Nice work, the final API is really flexible 👍

[gziolo]

Interesting, it’s experimental but we expose it in developer documentation. I hope to have a look into it and figure out how we can improve it. The good part is that https://developer.wordpress.org/block-editor/components/ is generated from the branch that targets WP 5.4 😃

[gziolo]

We might start inlining the comment in id 😂

id=“required-for-server-side-rendering-only”?