Update SegmentedControl to improve the experience for assistive technologies (#2220)

* updates role and focus behavior for SegmentedControl buttons, fixes style bug for full-width icon-only buttons, fixes bug for icon-only buttons where onClick and other shared props weren't being passed

* updates tests

* adds changeset

* fixes visual regression

Author: mperrotti

.changeset/old-experts-applaud.md

@@ -0,0 +1,7 @@
+---
+'@primer/react': patch
+---
+
+- Fixes `role` and keyboard behavior for SegmentedControl.
+- Fixes a bug where icon-only SegmentedControl buttons did not fill the parent width when the `fullWidth` prop was set
+- Fixes a bug where click handlers were not passed correctly when the responsive variant was set to `'hideLabels'`

src/SegmentedControl/SegmentedControl.test.tsx

@@ -197,55 +197,6 @@ describe('SegmentedControl', () => {
     expect(handleClick).toHaveBeenCalled()
   })
 
-  it('focuses the selected button first', async () => {
-    const user = userEvent.setup()
-    const {getByRole} = render(
-      <>
-        <button>Before</button>
-        <SegmentedControl aria-label="File view">
-          {segmentData.map(({label, id}, index) => (
-            <SegmentedControl.Button selected={index === 1} key={label} id={id}>
-              {label}
-            </SegmentedControl.Button>
-          ))}
-        </SegmentedControl>
-      </>
-    )
-    const initialFocusButtonNode = getByRole('button', {name: segmentData[1].label})
-
-    expect(document.activeElement?.id).not.toEqual(initialFocusButtonNode.id)
-
-    await user.tab() // focus the button before the segmented control
-    await user.tab() // move focus into the segmented control
-
-    expect(document.activeElement?.id).toEqual(initialFocusButtonNode.id)
-  })
-
-  it('focuses the previous button when keying ArrowLeft, and the next button when keying ArrowRight', () => {
-    const {getByRole} = render(
-      <SegmentedControl aria-label="File view">
-        {segmentData.map(({label, id}, index) => (
-          <SegmentedControl.Button selected={index === 1} key={label} id={id}>
-            {label}
-          </SegmentedControl.Button>
-        ))}
-      </SegmentedControl>
-    )
-    const initialFocusButtonNode = getByRole('button', {name: segmentData[1].label})
-    const nextFocusButtonNode = getByRole('button', {name: segmentData[0].label})
-
-    expect(document.activeElement?.id).not.toEqual(nextFocusButtonNode.id)
-
-    fireEvent.focus(initialFocusButtonNode)
-    fireEvent.keyDown(initialFocusButtonNode, {key: 'ArrowLeft'})
-
-    expect(document.activeElement?.id).toEqual(nextFocusButtonNode.id)
-
-    fireEvent.keyDown(initialFocusButtonNode, {key: 'ArrowRight'})
-
-    expect(document.activeElement?.id).toEqual(initialFocusButtonNode.id)
-  })
-
   it('calls onChange with index of clicked segment button when using the dropdown variant', async () => {
     act(() => {
       matchMedia.useMediaQuery(viewportRanges.narrow)

src/SegmentedControl/SegmentedControl.tsx

@@ -1,14 +1,19 @@
-import React, {RefObject, useRef} from 'react'
+import React, {useRef} from 'react'
 import Button, {SegmentedControlButtonProps} from './SegmentedControlButton'
 import SegmentedControlIconButton, {SegmentedControlIconButtonProps} from './SegmentedControlIconButton'
-import {ActionList, ActionMenu, Box, useTheme} from '..'
-import {merge, SxProp} from '../sx'
+import {ActionList, ActionMenu, useTheme} from '..'
+import sx, {merge, SxProp} from '../sx'
 import {ResponsiveValue, useResponsiveValue} from '../hooks/useResponsiveValue'
 import {ViewportRangeKeys} from '../utils/types/ViewportRangeKeys'
-import {FocusKeys, FocusZoneHookSettings, useFocusZone} from '../hooks/useFocusZone'
+import styled from 'styled-components'
 
 type WidthOnlyViewportRangeKeys = Exclude<ViewportRangeKeys, 'narrowLandscape' | 'portrait' | 'landscape'>
 
+// Needed because passing a ref to `Box` causes a type error
+const SegmentedControlList = styled.ul`
+  ${sx};
+`
+
 type SegmentedControlProps = {
   'aria-label'?: string
   'aria-labelledby'?: string
@@ -28,7 +33,10 @@ const getSegmentedControlStyles = (isFullWidth?: boolean) => ({
   borderStyle: 'solid',
   borderWidth: 1,
   display: isFullWidth ? 'flex' : 'inline-flex',
-  height: '32px' // TODO: use primitive `control.medium.size` when it is available
+  height: '32px', // TODO: use primitive `control.medium.size` when it is available
+  margin: 0,
+  padding: 0,
+  width: isFullWidth ? '100%' : undefined
 })
 
 const Root: React.FC<React.PropsWithChildren<SegmentedControlProps>> = ({
@@ -41,7 +49,7 @@ const Root: React.FC<React.PropsWithChildren<SegmentedControlProps>> = ({
   variant,
   ...rest
 }) => {
-  const segmentedControlContainerRef = useRef<HTMLSpanElement>(null)
+  const segmentedControlContainerRef = useRef<HTMLUListElement>(null)
   const {theme} = useTheme()
   const responsiveVariant = useResponsiveValue(variant, 'default')
   const isFullWidth = useResponsiveValue(fullWidth, false)
@@ -74,26 +82,7 @@ const Root: React.FC<React.PropsWithChildren<SegmentedControlProps>> = ({
 
     return React.isValidElement<SegmentedControlIconButtonProps>(childArg) ? childArg.props['aria-label'] : null
   }
-  const sx = merge(getSegmentedControlStyles(isFullWidth), sxProp as SxProp)
-
-  const focusInStrategy: FocusZoneHookSettings['focusInStrategy'] = () => {
-    if (segmentedControlContainerRef.current) {
-      // we need to use type assertion because querySelector returns "Element", not "HTMLElement"
-      type SelectedButton = HTMLButtonElement | undefined
-
-      const selectedButton = segmentedControlContainerRef.current.querySelector(
-        'button[aria-current="true"]'
-      ) as SelectedButton
-
-      return selectedButton
-    }
-  }
-
-  useFocusZone({
-    containerRef: segmentedControlContainerRef,
-    bindKeys: FocusKeys.ArrowHorizontal | FocusKeys.HomeAndEnd,
-    focusInStrategy
-  })
+  const listSx = merge(getSegmentedControlStyles(isFullWidth), sxProp as SxProp)
 
   if (!ariaLabel && !ariaLabelledby) {
     // eslint-disable-next-line no-console
@@ -141,12 +130,11 @@ const Root: React.FC<React.PropsWithChildren<SegmentedControlProps>> = ({
     </ActionMenu>
   ) : (
     // Render a segmented control
-    <Box
-      role="toolbar"
-      sx={sx}
+    <SegmentedControlList
+      sx={listSx}
       aria-label={ariaLabel}
       aria-labelledby={ariaLabelledby}
-      ref={segmentedControlContainerRef as RefObject<HTMLDivElement>}
+      ref={segmentedControlContainerRef}
       {...rest}
     >
       {React.Children.map(children, (child, index) => {
@@ -180,6 +168,7 @@ const Root: React.FC<React.PropsWithChildren<SegmentedControlProps>> = ({
             children: childPropsChildren,
             ...restChildProps
           } = child.props
+          const {sx: sharedSxProp, ...restSharedChildProps} = sharedChildProps
           if (!leadingIcon) {
             // eslint-disable-next-line no-console
             console.warn('A `leadingIcon` prop is required when hiding visible labels')
@@ -190,12 +179,12 @@ const Root: React.FC<React.PropsWithChildren<SegmentedControlProps>> = ({
                 icon={leadingIcon}
                 sx={
                   {
-                    '--separator-color':
-                      index === selectedIndex || index === selectedIndex - 1
-                        ? 'transparent'
-                        : theme?.colors.border.default
+                    ...sharedSxProp,
+                    // setting width here avoids having to pass `isFullWidth` directly to child components
+                    width: !isFullWidth ? '32px' : '100%' // TODO: use primitive `control.medium.size` when it is available instead of '32px'
                   } as React.CSSProperties
                 }
+                {...restSharedChildProps}
                 {...restChildProps}
               />
             )
@@ -205,7 +194,7 @@ const Root: React.FC<React.PropsWithChildren<SegmentedControlProps>> = ({
         // Render the children as-is and add the shared child props
         return React.cloneElement(child, sharedChildProps)
       })}
-    </Box>
+    </SegmentedControlList>
   )
 }
 

src/SegmentedControl/SegmentedControlButton.tsx

@@ -3,7 +3,7 @@ import {IconProps} from '@primer/octicons-react'
 import styled from 'styled-components'
 import {Box} from '..'
 import sx, {merge, SxProp} from '../sx'
-import {getSegmentedControlButtonStyles} from './getSegmentedControlStyles'
+import {getSegmentedControlButtonStyles, getSegmentedControlListItemStyles} from './getSegmentedControlStyles'
 
 export type SegmentedControlButtonProps = {
   /** The visible label rendered in the button */
@@ -26,19 +26,25 @@ const SegmentedControlButton: React.FC<React.PropsWithChildren<SegmentedControlB
   sx: sxProp = {},
   ...rest
 }) => {
-  const mergedSx = merge(getSegmentedControlButtonStyles({selected, children}), sxProp as SxProp)
+  const mergedSx = merge(getSegmentedControlListItemStyles(), sxProp as SxProp)
 
   return (
-    <SegmentedControlButtonStyled aria-current={selected} sx={mergedSx} {...rest}>
-      <span className="segmentedControl-content">
-        {LeadingIcon && (
-          <Box mr={1}>
-            <LeadingIcon />
-          </Box>
-        )}
-        <Box className="segmentedControl-text">{children}</Box>
-      </span>
-    </SegmentedControlButtonStyled>
+    <Box as="li" sx={mergedSx}>
+      <SegmentedControlButtonStyled
+        aria-current={selected}
+        sx={getSegmentedControlButtonStyles({selected, children})}
+        {...rest}
+      >
+        <span className="segmentedControl-content">
+          {LeadingIcon && (
+            <Box mr={1}>
+              <LeadingIcon />
+            </Box>
+          )}
+          <Box className="segmentedControl-text">{children}</Box>
+        </span>
+      </SegmentedControlButtonStyled>
+    </Box>
   )
 }
 

src/SegmentedControl/SegmentedControlIconButton.tsx

@@ -2,12 +2,9 @@ import React, {HTMLAttributes} from 'react'
 import {IconProps} from '@primer/octicons-react'
 import styled from 'styled-components'
 import sx, {merge, SxProp} from '../sx'
-import {
-  borderedSegment,
-  getSegmentedControlButtonStyles,
-  directChildLayoutAdjustments
-} from './getSegmentedControlStyles'
+import {getSegmentedControlButtonStyles, getSegmentedControlListItemStyles} from './getSegmentedControlStyles'
 import Tooltip from '../Tooltip'
+import Box from '../Box'
 
 export type SegmentedControlIconButtonProps = {
   'aria-label': string
@@ -35,26 +32,28 @@ export const SegmentedControlIconButton: React.FC<React.PropsWithChildren<Segmen
   sx: sxProp = {},
   ...rest
 }) => {
-  const mergedSx = merge(getSegmentedControlButtonStyles({selected, isIconOnly: true}), sxProp as SxProp)
+  const mergedSx = merge(
+    {
+      width: '32px', // TODO: use primitive `control.medium.size` when it is available
+      ...getSegmentedControlListItemStyles()
+    },
+    sxProp as SxProp
+  )
 
   return (
-    <Tooltip
-      text={ariaLabel}
-      sx={{
-        // Since the element rendered by Tooltip is now the direct child,
-        // we need to put these styles on the Tooltip instead of the button.
-        ...directChildLayoutAdjustments,
-        // The border drawn by the `:after` pseudo-element needs to scoped to the child button
-        // because Tooltip uses `:before` and `:after` to render the tooltip.
-        ':not(:last-child) button': borderedSegment
-      }}
-    >
-      <SegmentedControlIconButtonStyled aria-pressed={selected} sx={mergedSx} {...rest}>
-        <span className="segmentedControl-content">
-          <Icon />
-        </span>
-      </SegmentedControlIconButtonStyled>
-    </Tooltip>
+    <Box as="li" sx={mergedSx}>
+      <Tooltip text={ariaLabel}>
+        <SegmentedControlIconButtonStyled
+          aria-pressed={selected}
+          sx={getSegmentedControlButtonStyles({selected, isIconOnly: true})}
+          {...rest}
+        >
+          <span className="segmentedControl-content">
+            <Icon />
+          </span>
+        </SegmentedControlIconButtonStyled>
+      </Tooltip>
+    </Box>
   )
 }