ProgressBar: Adjust ProgressBar.Item for accessibility (#4878)

* Adjust `ProgressBar.Item` for accessibility

* Move warning

* Include if progressAsNumber === 0

* Update snapshots, tests, move aria-* attributes

* Update warning

* Update `ProgressBar.Item` props

* Account for `0`

* Add changeset

* Change children conditional

* Remove warning, add default 0

* Fix lint, test

* Update packages/react/src/ProgressBar/ProgressBar.tsx

Co-authored-by: Josh Black <joshblack@github.com>

* Update packages/react/src/ProgressBar/ProgressBar.tsx

Co-authored-by: Josh Black <joshblack@github.com>

* Update packages/react/src/ProgressBar/ProgressBar.tsx

Co-authored-by: Josh Black <joshblack@github.com>

---------

Co-authored-by: Josh Black <joshblack@github.com>

.changeset/lucky-horses-kiss.md

@@ -0,0 +1,5 @@
+---
+'@primer/react': minor
+---
+
+Move `aria-*` attributes to `ProgressBar.Item` and marks `ProgressBar.Item` as `role="progressbar".

packages/react/src/ProgressBar/ProgressBar.tsx

@@ -5,7 +5,6 @@ import {width} from 'styled-system'
 import {get} from '../constants'
 import type {SxProp} from '../sx'
 import sx from '../sx'
-import {warning} from '../utils/warning'
 
 type ProgressProp = {
   progress?: string | number
@@ -17,7 +16,7 @@ const shimmer = keyframes`
   to { mask-position: 0%; }
 `
 
-export const Item = styled.span<ProgressProp & SxProp>`
+const ProgressItem = styled.span<ProgressProp & SxProp>`
   width: ${props => (props.progress ? `${props.progress}%` : 0)};
   background-color: ${props => get(`colors.${props.bg || 'success.emphasis'}`)};
 
@@ -34,8 +33,6 @@ export const Item = styled.span<ProgressProp & SxProp>`
   ${sx};
 `
 
-Item.displayName = 'ProgressBar.Item'
-
 const sizeMap = {
   small: '5px',
   large: '10px',
@@ -60,37 +57,78 @@ const ProgressContainer = styled.span<StyledProgressContainerProps>`
   ${sx};
 `
 
+export type ProgressBarItems = React.HTMLAttributes<HTMLSpanElement> & {'aria-label'?: string} & ProgressProp & SxProp
+
+export const Item = forwardRef<HTMLSpanElement, ProgressBarItems>(
+  (
+    {progress, 'aria-label': ariaLabel, 'aria-valuenow': ariaValueNow, 'aria-valuetext': ariaValueText, ...rest},
+    forwardRef,
+  ) => {
+    const progressAsNumber = typeof progress === 'string' ? parseInt(progress, 10) : progress
+
+    const ariaAttributes = {
+      'aria-valuenow':
+        ariaValueNow ?? (progressAsNumber !== undefined && progressAsNumber >= 0 ? Math.round(progressAsNumber) : 0),
+      'aria-valuemin': 0,
+      'aria-valuemax': 100,
+      'aria-valuetext': ariaValueText,
+    }
+
+    return (
+      <ProgressItem
+        {...rest}
+        role="progressbar"
+        aria-label={ariaLabel}
+        ref={forwardRef}
+        progress={progress}
+        {...ariaAttributes}
+      />
+    )
+  },
+)
+
+Item.displayName = 'ProgressBar.Item'
+
 export type ProgressBarProps = React.HTMLAttributes<HTMLSpanElement> & {bg?: string} & StyledProgressContainerProps &
   ProgressProp
 
 export const ProgressBar = forwardRef<HTMLSpanElement, ProgressBarProps>(
   (
-    {animated, progress, bg = 'success.emphasis', barSize = 'default', children, ...rest}: ProgressBarProps,
+    {
+      animated,
+      progress,
+      bg = 'success.emphasis',
+      barSize = 'default',
+      children,
+      'aria-label': ariaLabel,
+      'aria-valuenow': ariaValueNow,
+      'aria-valuetext': ariaValueText,
+      ...rest
+    }: ProgressBarProps,
     forwardRef,
   ) => {
     if (children && progress) {
       throw new Error('You should pass `progress` or children, not both.')
     }
 
-    warning(
-      children &&
-        typeof (rest as React.AriaAttributes)['aria-valuenow'] === 'undefined' &&
-        typeof (rest as React.AriaAttributes)['aria-valuetext'] === 'undefined',
-      'Expected `aria-valuenow` or `aria-valuetext` to be provided to <ProgressBar>. Provide one of these values so screen reader users can determine the current progress. This warning will become an error in the next major release.',
-    )
-
-    const progressAsNumber = typeof progress === 'string' ? parseInt(progress, 10) : progress
-
-    const ariaAttributes = {
-      'aria-valuenow': progressAsNumber ? Math.round(progressAsNumber) : undefined,
-      'aria-valuemin': 0,
-      'aria-valuemax': 100,
-      'aria-valuetext': progressAsNumber ? `${Math.round(progressAsNumber)}%` : undefined,
-    }
+    // Get the number of non-empty nodes passed as children, this will exclude
+    // booleans, null, and undefined
+    const validChildren = React.Children.toArray(children).length
 
     return (
-      <ProgressContainer ref={forwardRef} role="progressbar" barSize={barSize} {...ariaAttributes} {...rest}>
-        {children ?? <Item data-animated={animated} progress={progress} bg={bg} />}
+      <ProgressContainer ref={forwardRef} barSize={barSize} {...rest}>
+        {validChildren ? (
+          children
+        ) : (
+          <Item
+            data-animated={animated}
+            progress={progress}
+            aria-label={ariaLabel}
+            aria-valuenow={ariaValueNow}
+            aria-valuetext={ariaValueText}
+            bg={bg}
+          />
+        )}
       </ProgressContainer>
     )
   },

packages/react/src/__tests__/ProgressBar.test.tsx

@@ -6,7 +6,7 @@ import axe from 'axe-core'
 import {FeatureFlags} from '../FeatureFlags'
 
 describe('ProgressBar', () => {
-  behavesAsComponent({Component: ProgressBar})
+  behavesAsComponent({Component: ProgressBar, toRender: () => <ProgressBar aria-valuenow={10} progress={0} />})
 
   checkExports('ProgressBar', {
     default: undefined,
@@ -72,4 +72,50 @@ describe('ProgressBar', () => {
   it('respects the "progress" prop', () => {
     expect(render(<ProgressBar progress={80} aria-label="Upload test.png" />)).toMatchSnapshot()
   })
+
+  it('passed the `aria-label` down to the progress bar', () => {
+    const {getByRole, getByLabelText} = HTMLRender(<ProgressBar progress={80} aria-label="Upload test.png" />)
+    expect(getByRole('progressbar')).toHaveAttribute('aria-label', 'Upload test.png')
+    expect(getByLabelText('Upload test.png')).toBeInTheDocument()
+  })
+
+  it('passed the `aria-valuenow` down to the progress bar', () => {
+    const {getByRole} = HTMLRender(<ProgressBar progress={80} aria-valuenow={80} />)
+    expect(getByRole('progressbar')).toHaveAttribute('aria-valuenow', '80')
+  })
+
+  it('passed the `aria-valuetext` down to the progress bar', () => {
+    const {getByRole} = HTMLRender(<ProgressBar aria-valuetext="80 percent" />)
+    expect(getByRole('progressbar')).toHaveAttribute('aria-valuetext', '80 percent')
+  })
+
+  it('does not pass the `aria-label` down to the progress bar if there are multiple items', () => {
+    const {getByRole} = HTMLRender(
+      <ProgressBar aria-label="Upload test.png">
+        <ProgressBar.Item progress={80} />
+      </ProgressBar>,
+    )
+    expect(getByRole('progressbar')).not.toHaveAttribute('aria-label')
+  })
+
+  it('passes aria attributes to the progress bar item', () => {
+    const {getByRole} = HTMLRender(
+      <ProgressBar>
+        <ProgressBar.Item progress={50} aria-label="Progress" ria-valuenow="50"></ProgressBar.Item>
+      </ProgressBar>,
+    )
+    expect(getByRole('progressbar')).toHaveAttribute('aria-valuenow', '50')
+    expect(getByRole('progressbar')).toHaveAttribute('aria-label', 'Progress')
+  })
+
+  it('provides `aria-valuenow` to the progress bar item if it is not already provided', () => {
+    const {getByRole} = HTMLRender(<ProgressBar progress={50} />)
+    expect(getByRole('progressbar')).toHaveAttribute('aria-valuenow', '50')
+  })
+
+  it('applies `0` as a value for `aria-valuenow`', () => {
+    const {getByRole} = HTMLRender(<ProgressBar progress={0} aria-valuenow={0} aria-label="Upload text.png" />)
+
+    expect(getByRole('progressbar')).toHaveAttribute('aria-valuenow', '0')
+  })
 })

packages/react/src/__tests__/__snapshots__/ProgressBar.test.tsx.snap

@@ -34,16 +34,15 @@ exports[`ProgressBar respects the "progress" prop 1`] = `
 }
 
 <span
-  aria-label="Upload test.png"
-  aria-valuemax={100}
-  aria-valuemin={0}
-  aria-valuenow={80}
-  aria-valuetext="80%"
   className="c0"
-  role="progressbar"
 >
   <span
+    aria-label="Upload test.png"
+    aria-valuemax={100}
+    aria-valuemin={0}
+    aria-valuenow={80}
     className="c1"
+    role="progressbar"
   />
 </span>
 `;