Add aria attributes to the progress bar #3517

kendallgassner · 2023-07-13T20:38:04Z

What

During passion week we got to play around with the ProgressBar component and we noticed that this component was missing some aria-attributes. I wanted to create a PR proposing adding these attributes to the component to improve accessibility.

I also added a prop tabIndex so the progressBar could be focused if a feature wanted to move focus to the component -- we found this to be essential when working on fileUpload guidance.

Screenshots

Please provide before/after screenshots for any visual changes

Merge checklist

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 · 2023-07-13T20:38:06Z

🦋 Changeset detected

Latest commit: fb50b97

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	Patch

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 · 2023-07-13T20:41:34Z

size-limit report 📦

Path	Size
dist/browser.esm.js	102.82 KB (+0.08% 🔺)
dist/browser.umd.js	103.38 KB (+0.09% 🔺)

broccolinisoup

Thanks for this PR, it is a great improvement! 🙏🏻 I left some comments. Thanks for your time!

broccolinisoup · 2023-07-14T03:15:38Z

src/ProgressBar/ProgressBar.tsx

+      'aria-valuetext': typeof progress === 'string' ? progress : undefined,
+      'aria-valuemin': 0,
+      'aria-valuemax': 100,
+      'aria-busy': progress !== 100,


Should we consider the string type here as well?

@broccolinisoup Thanks for bringing this up. I ended up changing aria attributes a bit since t looks like this component really only works if progress is a number or a string of a number like ("66" or "100") Because the string is really just a number using aria-valuetext isn't actually necessary.

To reduce confusion maybe we should only allow a number passed in?

From the first look at the usages at dotcom, in practicality it seems fine but technically narrowing types are considered as major change. (reference) And I don't know if there is any historical reasons to include string type in the first place. Sorry @kendallgassner we are dealing with a bit of an old component here I guess and lots to improve - I am trying to keep my review only specific to your changes but seems like some overlaps are inevitable 😬 I'll post internally as well to bring some more reviewers to get more context on this!

.changeset/plenty-mangos-sniff.md

broccolinisoup · 2023-07-14T03:21:36Z

src/ProgressBar/ProgressBar.tsx

We also have ProgressBar with multiple items like here and we don't get aria attributes in these I realised. There is no usage of ProgressBar.Item though at dotcom so not sure if it would worth the effort. What do you think??

Good catch!!

To deal with multiple I now throw an error if children are passed in without aria-valuenow or 'aria-valuetext:

throw new Error( 'Use the `aria-valuenow` or `aria-valuetext` so screen reader users can determine the `progress`.', )

What do you think?

Oh I see! I have a different take on this but I am not 100% sure, let me know what you think!

Seems like we are handling these aria attributes internally and I think it is a tad inconsistent to ask this from consumers when there are ProgressBar.Item children.

I am thinking maybe we would want to calculate the total progress internally and update the aria attributes as we do for the single progress item? Something along those lines (not a complete code)

if (children) { let totalProgress = 0 React.Children.toArray(children).forEach(child => { if (React.isValidElement(child) && child.props.progress) { totalProgress += parseInt(child.props.progress) } }) }

And use that totalProgress to calculate the value of aria attributes.

cc @siddharthkp @joshblack I would love their opinion before we decide anything concrete.

Hi!

We could definitely loop through the children if the usage is predictable. However...

I couldn't find any usage of ProgressBar with multiple items, so we might be able to slide it under the rug and deprecate it safely without any remediation :)

Co-authored-by: Armağan <[email protected]>

kendallgassner · 2023-07-14T20:29:43Z

Note:

Because of role: progressbar AXE will break if no accessible name is provided (i.e.aria-label or aria-labelledby). I added an aria-label to all the examples but wonder if I should enforce it at a prop level or throw an error like we do witht he children & progress

siddharthkp · 2023-07-19T13:54:44Z

src/ProgressBar/ProgressBar.tsx

 type StyledProgressContainerProps = {
  inline?: boolean
  barSize?: keyof typeof sizeMap
+  tabIndex?: number


Not sure if you need to add this explicitly 🤔

If you pass tabIndex to ProgressBar, it passes it already passes it down to the html element

<Progress tabIndex={0} progress={50} aria-label="Task completion" />

Amazing! I'll try this out.

It looks like I need the type added:

Is there a better type to use? Maybe React.HTMLProps ?

Maybe this could be an option?

type StyledProgressContainerProps = { inline?: boolean barSize?: keyof typeof sizeMap } & WidthProps & SxProp & React.HTMLAttributes<HTMLSpanElement>

siddharthkp · 2023-07-19T14:02:26Z

src/ProgressBar/ProgressBar.tsx

+    const progressAsNumber = typeof progress === 'string' ? parseInt(progress) : progress
+
+    const ariaAttributes = {
+      'aria-valuenow': progressAsNumber ? Math.round(progressAsNumber) : undefined,


Suggested change

'aria-valuenow': progressAsNumber ? Math.round(progressAsNumber) : undefined,

'aria-valuenow': progressAsNumber || undefined,

Don't think we need to Math.round because parseInt already assumes base 10 and returns an integer

parseInt(20.5) === parseInt(20.5, 10) === 20

Oh good point! I added the math.round before I used parseInt.

Oh wait... I need it in the case that the typeof the value is a number not a string

Ah I see! My bad!

@siddharthkp one note with parseInt, I believe it will infer the base if no radix is provided based on: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/parseInt#radix

If 0 or not provided, the radix will be inferred based on string's value. Be careful — this does not always default to 10!

Oh wow, good note!

TylerJDev · 2023-07-19T14:25:36Z

wonder if I should enforce it at a prop level or throw an error like we do with the children & progress

I think this would be a good idea to add as a prop so it's surfaced better, especially in the documentation! I'm assuming that if it's a required prop then each implementation would need to be remediated to include a label. But we'd want to include the label either way as AXE will surely throw an error if one isn't included 😕.

I do think that it would be beneficial to add it as a required prop, as the usage of the component seems to be limited across dotcom (for now). But would love additional thoughts on this @siddharthkp @broccolinisoup as y'all have more knowledge of the implications here.

joshblack

Thanks so much for making this PR! 🥳 Just left a couple of comments/questions

src/ProgressBar/ProgressBar.tsx

joshblack · 2023-07-19T16:07:12Z

src/ProgressBar/ProgressBar.tsx

+    const progressAsNumber = typeof progress === 'string' ? parseInt(progress) : progress
+
+    const ariaAttributes = {
+      'aria-valuenow': progressAsNumber ? Math.round(progressAsNumber) : undefined,


@siddharthkp one note with parseInt, I believe it will infer the base if no radix is provided based on: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/parseInt#radix

If 0 or not provided, the radix will be inferred based on string's value. Be careful — this does not always default to 10!

joshblack · 2023-07-19T16:11:33Z

src/ProgressBar/ProgressBar.tsx

 type StyledProgressContainerProps = {
  inline?: boolean
  barSize?: keyof typeof sizeMap
+  tabIndex?: number


Maybe this could be an option?

type StyledProgressContainerProps = { inline?: boolean barSize?: keyof typeof sizeMap } & WidthProps & SxProp & React.HTMLAttributes<HTMLSpanElement>

kendallgassner · 2023-07-19T19:07:17Z

src/ProgressBar/ProgressBar.tsx

-    </ProgressContainer>
-  )
-}
+export type ProgressBarProps = React.HTMLAttributes<HTMLSpanElement> & {bg?: string} & StyledProgressContainerProps &


@joshblack I added React.HTMLAttributes<HTMLSpanElement> here!

joshblack

Thanks for doing this! 🥳

I appreciated your comment about the aria-label above btw, I think our ideal for this stuff should be to move it into the type system but I think that would be considered a breaking change 😞 The approach here seems like a great incremental step and I'll make sure to capture this over in: https://github.com/github/primer/issues/1715 for v37!

lesliecdubs · 2023-08-08T21:30:41Z

FYI, I pulled this comment out into an additional action item: #3605

Add aria attributes to the progress bar

814bf19

kendallgassner requested review from a team and siddharthkp July 13, 2023 20:38

kendallgassner temporarily deployed to github-pages July 13, 2023 20:45 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-3517 July 13, 2023 20:46 Inactive

changset

b318e2c

kendallgassner temporarily deployed to github-pages July 13, 2023 20:57 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-3517 July 13, 2023 20:58 Inactive

broccolinisoup reviewed Jul 14, 2023

View reviewed changes

Account for multiple items

7bbd044

kendallgassner temporarily deployed to github-pages July 14, 2023 18:07 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-3517 July 14, 2023 18:07 Inactive

Update .changeset/plenty-mangos-sniff.md

80e41b7

Co-authored-by: Armağan <[email protected]>

kendallgassner temporarily deployed to github-pages July 14, 2023 18:17 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-3517 July 14, 2023 18:18 Inactive

add aria-label

48aafd7

kendallgassner temporarily deployed to github-pages July 14, 2023 20:34 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-3517 July 14, 2023 20:34 Inactive

more aria-labels

5f03895

kendallgassner temporarily deployed to github-pages July 14, 2023 21:03 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-3517 July 14, 2023 21:05 Inactive

kendallgassner requested a review from broccolinisoup July 17, 2023 15:53

Merge branch 'main' into add-aria-to-progress-bar

d8dcf82

kendallgassner temporarily deployed to github-pages July 18, 2023 19:36 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-3517 July 18, 2023 19:36 Inactive

Add display name

7dc7fe9

kendallgassner temporarily deployed to github-pages July 18, 2023 19:57 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-3517 July 18, 2023 19:57 Inactive

kendallgassner requested a review from TylerJDev July 18, 2023 20:55

siddharthkp reviewed Jul 19, 2023

View reviewed changes

joshblack reviewed Jul 19, 2023

View reviewed changes

parse int fix and warning

fb50b97

kendallgassner commented Jul 19, 2023

View reviewed changes

kendallgassner deployed to github-pages July 19, 2023 19:12 — with GitHub Actions View deployment

github-actions bot deployed to storybook-preview-3517 July 19, 2023 19:13 View deployment

kendallgassner requested review from joshblack and siddharthkp July 20, 2023 15:54

joshblack approved these changes Jul 20, 2023

View reviewed changes

kendallgassner added this pull request to the merge queue Jul 20, 2023

Merged via the queue into main with commit cf9d8a5 Jul 20, 2023

kendallgassner deleted the add-aria-to-progress-bar branch July 20, 2023 16:57

primer-css mentioned this pull request Jul 20, 2023

Release tracking #3493

Merged

primer-css mentioned this pull request Jul 27, 2023

Release tracking (next major) #3147

Closed

lesliecdubs mentioned this pull request Aug 8, 2023

Provide API to attach an accessible name to PRC ProgressBar #3605

Closed

skemex mentioned this pull request May 24, 2024

[Snyk] Upgrade @primer/react from 35.26.1 to 35.32.2 skemex/wpe-actions-workshop#45

Open

rbragaru mentioned this pull request Aug 14, 2024

[Snyk] Upgrade @primer/react from 35.15.1 to 35.32.2 rbragaru/rocketdex#4

Open

skemex mentioned this pull request Sep 24, 2024

[Snyk] Upgrade @primer/react from 35.26.1 to 35.32.2 skemex/wpe-actions-workshop#55

Open

skemex mentioned this pull request Nov 10, 2024

[Snyk] Upgrade @primer/react from 35.26.1 to 35.32.2 skemex/wpe-actions-workshop#56

Open

	'aria-valuenow': progressAsNumber ? Math.round(progressAsNumber) : undefined,
	'aria-valuenow': progressAsNumber \|\| undefined,

Add aria attributes to the progress bar #3517

Add aria attributes to the progress bar #3517

Uh oh!

Conversation

kendallgassner commented Jul 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

Screenshots

Merge checklist

Uh oh!

changeset-bot bot commented Jul 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

github-actions bot commented Jul 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

size-limit report 📦

Uh oh!

broccolinisoup left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kendallgassner Jul 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

siddharthkp Jul 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kendallgassner commented Jul 14, 2023

Uh oh!

siddharthkp Jul 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kendallgassner Jul 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TylerJDev commented Jul 19, 2023

Uh oh!

joshblack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joshblack left a comment

kendallgassner commented Jul 13, 2023 •

edited

Loading

changeset-bot bot commented Jul 13, 2023 •

edited

Loading

github-actions bot commented Jul 13, 2023 •

edited

Loading

kendallgassner Jul 14, 2023 •

edited

Loading

siddharthkp Jul 19, 2023 •

edited

Loading

siddharthkp Jul 19, 2023 •

edited

Loading

kendallgassner Jul 19, 2023 •

edited

Loading