SegmentedControl a11y fixes

[mperrotti]

These changes fix accessibility defects called out in this comment: https://github.com/github/primer/issues/1495#issuecomment-1332693886

These changes also include updates to ActionMenu that allow us to reference the DOM node that labels the menu.

Closes https://github.com/github/primer/issues/1495

Screenshots

There should be no visual changes

Merge checklist

• Added/updated tests
• Added/updated documentation
• Tested in Chrome
• Tested in Firefox
• Tested in Safari
• Tested in Edge

Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.

[changeset-bot]

🦋 Changeset detected

Latest commit: 5b74f4f

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]

size-limit report 📦

Path	Size
dist/browser.esm.js	94.54 KB (+0.04% 🔺)
dist/browser.umd.js	95.13 KB (+0.05% 🔺)

[kendallgassner]

is there a storybook / doc preview available?

[mperrotti]

@kendallgassner - there is a preview deployment.

Here is the docs page: https://primer-f38a78541d-13348165.drafts.github.io/SegmentedControl

Here are the Storybook stories: https://primer-f38a78541d-13348165.drafts.github.io/storybook/?path=/story/components-segmentedcontrol-features--associated-with-a-label-and-caption

None of the link's paths have changed since your review, you just have to look at them on https://primer-f38a78541d-13348165.drafts.github.io/ intead of https://primer.style/react/

[kendallgassner]

Accessibility Review

Documentation

• - Would it be possible to add a documentation note that this component should not be used in a form?

Semantics

• - I only see the semantics being used in this way on the doc site. Specifically on with a label above and caption below but right now 'File Name' is a semantic label this feels wrong because the component is not a form element (input, select, etc.) We should make this a span instead... I am confused though because the storybook examples do in fact show the File Name as a span.
  A couple of issues in the icon-only story / SegmentedControlIconButton
  • - This example uses the incorrect aria attribute: aria-pressed. The buttons should not act as 'toggle buttons' because users cannot toggle the buttons on/off by clicking on them they must click on a different button to switch the selection. Solution: replace aria-pressed with aria-current .
  • - I notice you are trying to use the tooltip component to label each icon button. With our rails tooltip, this would work great but our react tooltip is not accessible. There is no association between the tooltip text and the icon button so screen reader users only hear 'toggle button selected'. The fix here would be to fix the tooltip component to match the semantics/aria attributes of the rails tooltip (aria-labelledby).
• - In the Variant action menu story, there is no name or label to associate with the menu options. We need to require a sr-only label if an engineer uses a variant drop-down.

Zoom

• - (Tested at 400% ✅ ) If I zoom into the fullwidth narrow story I notice that the component's button text moves outside of the button. We will likely have users who need to zoom into the page to view content.
  

• - When a segmented control includes a label we should move the segmented control to a new line on mobile. For example, if you view associated with a label and caption story on a mobile screen the user must scroll to navigate to the segmented control. The user should not have to scroll to view content.

Misc.

• - I noticed a really tiny CSS bug while reviewing the component. The line separating the second button from the third button appears over the focus indicator when the second button is focused.
  

• - In our deep dive today, @jscholes noticed that the preview something besides the first button selected did in fact have the first button selected.

[kendallgassner]

• - In the Variant action menu story, there is no name or label to associate with the menu options. We need to require a sr-only label if an engineer uses a variant drop-down.

Technically we are not 'requiring' this but all the examples are labeled. @mperrotti can we throw a console warning if there is no aria-labelledby or aria-label in the action menu case

[kendallgassner]

<3

[mperrotti]

can we throw a console warning if there is no aria-labelledby or aria-label in the action menu case

We already do :)

[mperrotti]

@kendallgassner - the inaccessible tooltip is a known issue for @primer/react. We're working on a fix (PR) so we can make it accessible, but it's blocked right now.

I'm going to fix the styling issue you pointed out and re-request a review. It would be nice to merge these other fixes and just leave the a11y review issue open until we replace the tooltip.

[kendallgassner]

Thank you for all of these fixes!