Add CheckboxGroup.Choices and RadioGroup.Choices subcomponents which format their children as a list

JeffreyKozik commented 3 months ago

Followup to https://github.com/github/github/pull/329358#discussion_r1647556804 and this discussion in Slack.

Changelog

New

Created the CheckboxGroup.Choices and RadioGroup.Choices subcomponents.

Currently, our Radio Group and Checkbox Groups aren't lists

Screenshot of the Checkbox Group component on Primer React's Storybook showing that it isn't a list

But according to the ARIA Authoring Practices Guide (APG), these groups should be lists:

To enable assistive technology users to perceive the set of checkboxes as a list of four items, each div element that serves as a checkbox is contained within a li element contained by a ul element.

Additionally, one of GitHub Accessibility's "trusted component libraries" Polaris, by Shopify, follows the APG's guidelines of making their Checkbox Groups lists: https://github.com/Shopify/polaris/blob/902db588f6a317f4d6a5a76f4814bdb1f80395db/polaris-react/src/components/ChoiceList/ChoiceList.tsx#L109-L146.

I thought of three different approaches for Primer

Keep our current implementation We currently put the burden on the developer to add <ul> and <li> elements to their code. The positive of this approach is it gives the developer maximal flexibility no matter how they're using CheckboxGroup or RadioGroup. However, the downside is that it means the developer has to manually add these elements nearly every single time, otherwise their code isn't fully accessible. A cursory search of GitHub's codebase shows that is almost never implemented.
Automatically wrap each child in an li element We could do it in the code here: https://github.com/primer/react/blob/e957fef6785277b1c30684b146a0f591d8c6b3e2/packages/react/src/internal/components/CheckboxOrRadioGroup/CheckboxOrRadioGroup.tsx#L125-L134 This removes the onus from the developer to have to manually add the <ul> and <li> elements to their code. The positive of this approach is that the accessibility logic is completely abstracted away into the CheckboxGroup and RadioGroup components. However, the downside is that doing this would be a breaking change for some implementations. For example, this code wraps Radio buttons within an ActionList: https://github.com/github/github/blob/b61537935e3cbdfa0e05a45656e854e24ae674d2/ui/packages/issue-viewer/components/RadioActionListItem.tsx#L19-L28 So automatically adding a ul element here would be wrong because an ActionList is already a list: https://github.com/primer/react/blob/e957fef6785277b1c30684b146a0f591d8c6b3e2/packages/react/src/ActionList/List.tsx#L15 https://github.com/primer/react/blob/e957fef6785277b1c30684b146a0f591d8c6b3e2/packages/react/src/ActionList/List.tsx#L62-L70
The approach I took: Create the CheckboxGroup.Choices and RadioGroup.Choices subcomponents While this approach does still require the developer to explicitly use these subcomponents, it's considerably easier and more consistent than them having to use ul and li elements and apply custom styling. Following this approach, our Radio Group and Checkbox Groups are now lists

Changed

Removed

Rollout strategy

[ ] Patch release
[x] Minor release
[ ] Major release; if selected, include a written rollout or migration plan
[ ] None; if selected, include a brief description as to why

Testing & Reviewing

Merge checklist

[ ] Added/updated tests
[ ] Added/updated documentation
[ ] Added/updated previews (Storybook)
[ ] Changes are SSR compatible
[ ] Tested in Chrome
[ ] Tested in Firefox
[ ] Tested in Safari
[ ] Tested in Edge
[ ] (GitHub staff only) Integration tests pass at github/github (Learn more about how to run integration tests)

changeset-bot[bot] commented 3 months ago

🦋 Changeset detected

Latest commit: 3aba7a654b142e762a6cb74342e7bfb04536365f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

| Name | Type | | ------------- | ----- | | @primer/react | Minor |

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

github-actions[bot] commented 3 months ago

size-limit report 📦

Path	Size
packages/react/dist/browser.esm.js	90.87 KB (-0.01% 🔽)
packages/react/dist/browser.umd.js	91.16 KB (+0.1% 🔺)

JeffreyKozik commented 3 months ago

After further discussion on Slack, accessibility experts @ericwbailey and @patrickhlauke have mentioned that using ul and li for Radio groups and Checkbox groups is actually not needed and can be counterproductive.

Per @ericwbailey

Lists are not required for radio groups and can circumstantially create unnecessary and unneeded verbosity. Screen readers have separate controls for traversing and enumerating radio groups, as well

primer / react