Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(progress-bar): fix label accessibility violation #9479

Merged
merged 2 commits into from
Aug 30, 2021

Conversation

janhassel
Copy link
Member

Fixes an accessiblity violation reported by the IBM Equal Access Accessibility Checker: A progress bar requires a label. The current implementation used a <label> element which is unsupported in combination with the target element (progress bar), because it's a <div> element. Therefore, the implemenation needs to change to use aria-labelledby instead.

Changelog

Changed

  • Use <span> instead <label> for label
  • Update tests

Testing / Reviewing

  • Ensure the label is still shown with no visual regression
  • Ensure there are no violations reported by the IBM Equal Access Accessiblity Checker

@janhassel janhassel requested a review from a team as a code owner August 16, 2021 11:35
@netlify
Copy link

netlify bot commented Aug 16, 2021

✔️ Deploy Preview for carbon-react-next ready!

🔨 Explore the source changes: ad3ac85

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-react-next/deploys/612d195cdd46d60007a186f4

😎 Browse the preview: https://deploy-preview-9479--carbon-react-next.netlify.app

@netlify
Copy link

netlify bot commented Aug 16, 2021

✔️ Deploy Preview for carbon-elements ready!

🔨 Explore the source changes: ad3ac85

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-elements/deploys/612d195cac0a2d00084b1976

😎 Browse the preview: https://deploy-preview-9479--carbon-elements.netlify.app

@netlify
Copy link

netlify bot commented Aug 16, 2021

✔️ Deploy Preview for carbon-components-react ready!
Built without sensitive environment variables

🔨 Explore the source changes: ad3ac85

🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-components-react/deploys/612d195cf3cb6c000815efed

😎 Browse the preview: https://deploy-preview-9479--carbon-components-react.netlify.app

Copy link
Contributor

@dakahn dakahn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think using a non-semantic element as a label here is the way we want to go. We should be able to either a) use a for attribute on the label to associate it with the progressbar or b) explicitly label the progress bar by wrapping it in the <Label> element.

That said I think we potentially have an Accessibility Checker bug on our hands here. In my testing I wrapped the progressbar in the label element like this

<label>A Progress Bar                                                                                                                         
    <div role="progressbar" aria-valuenow="20" aria-valuemin="0" aria-valuemax="100">20
    </div>                                                 
</label>

(you can check it out yourself in this Codepen)

Which, barring some little known quirk with role="progressbar" should be clearing this error. In fact if we look at the Accessibility Tree we see the progressbar labelled correctly
image

@tombrunet does this track with you?

@janhassel
Copy link
Member Author

@dakahn Strictly speaking, wouldn't nesting a <div> element inside a <label> element violate the HTML specification since a <div> is not considered a labelable element (which would also be true for the initial implementation with for="[div-id]")?

@tombrunet
Copy link
Contributor

@dakahn label elements are only defined to work with input elements - pairing native HTML elements with native HTML elements. When using ARIA, you're expected to label them the ARIA way. I'll do a little digging to make sure something hasn't changed with recent ARIA in HTML specs, but mixing labels with non-inputs isn't valid as far as I know.

@dakahn
Copy link
Contributor

dakahn commented Aug 19, 2021

@tombrunet Thanks for clarifying that makes sense -- don't know why I was mentally thinking of the progressbar as an input

@janhassel you're totally right! Ignore my previous review 👍🏾

@kodiakhq kodiakhq bot merged commit 9723ed6 into carbon-design-system:main Aug 30, 2021
@janhassel janhassel deleted the progressbar-a11y branch October 21, 2021 12:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants