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

[LinearProgress] component not accessible to screen reader users #7054

Closed
tuukkao opened this issue Jun 4, 2017 · 3 comments
Closed

[LinearProgress] component not accessible to screen reader users #7054

tuukkao opened this issue Jun 4, 2017 · 3 comments
Labels
accessibility a11y component: LinearProgress The React component. component: progress This is the name of the generic UI component, not the React module!

Comments

@tuukkao
Copy link

tuukkao commented Jun 4, 2017

Problem description

The version of LinearProgress in next is not accessible to screen reader users. aria-valuenow, while set correctly, doesn't convey all the needed semantics for the component to be treated like a progress bar. I've been working on fixing these issues. However, I'd like to go through some design decisions before making a pr.

I've moved the aria attributes from the root div element to the 2 last inner div elements. This is because in buffer mode those two elements represent two different progress bars. The attributes are set conditionally, i.e. only the upper bar is shown in determinate mode.

However, there are some problems with this approach. Right now it is not possible to set any attributes to the actual progress bar, only to the container element. This is a problem when setting tabIndex or a status text in indeterminate/query mode, since this information needs to be associated with the element that has all the other aria attributes (role, aria-valuenow, etc.) I could pass all the relevant attributes to the inner div element, but this would not work in buffer mode where there are two different progress bars which might need to have e.g. different labels.

One solution to this problem could be to expose separate props that would get passed through to the individual progress bars, but would that expose too much of the inner workings of the component? IT would also make the api needlessly complicated in anything other than buffer mode.

Another solution would be to pass all the attributes through to the div element that represents the progress bar and have a separate prop for the buffer bar attributes, but this approach has the downside of making the api somewhat more unpredictable. However, this design feels the most natural to me, since it enforces more complexity only when it is needed.

Any thoughts?

Link to minimal working code that reproduces the issue

https://github.com/callemall/material-ui/blob/next/docs/src/pages/component-demos/progress/LinearBuffer.js

Versions

  • Material-UI: Next (d9ef9f4)
  • React: 15.5.4
  • Browser: Firefox 53.0.3
@oliviertassinari oliviertassinari added the component: LinearProgress The React component. label Jun 4, 2017
@slavab89
Copy link
Contributor

slavab89 commented Jun 4, 2017

I think we should treat the different bars a little different
For determinate progress, there should actually be only 1 inner div that's actually rendered (bar1). The other divs either have no classes/styles or display: none style so they are not shown anyway.
For indeterminate, there is no point in aria-valuenow. First div (dasshed) doesnt need to be rendered
For query there is no point in aria-valuenow. First div (dasshed) doesnt need to be rendered
For buffer, you could have 2 different valueNow attributes on each bar corresponding with the 2 different values that you receive as props.

I didnt fully understand what other attributes or properties you're referring to or what you want to pass down to the progress div itself.. Could you give some examples?
Is tabIndex really relevant for progress? Would you want it to be focusable when navigating with tab? And what does status text mean?

@tuukkao
Copy link
Author

tuukkao commented Jul 6, 2017

For  determinate  progress, there should actually be only 1 inner div that's actually rendered (bar1). The other divs either have no classes/styles or  display: none  style so they are not shown anyway.

Good to know. Thanks. I just assumed they would be rendered since they weren't added conditionally.

For  buffer , you could have 2 different  valueNow  attributes on each bar corresponding with the 2 different values that you receive as props.

This is what I've been doing for now. However, since these are logically two independent progress bars having just an aria-valuenow prop for the buffer might not be enough. I'd rather have a mechanism for passing any attribute down to the div representing the buffer bar.

I didnt fully understand what other attributes or properties you're referring to or what you want to pass down to the progress div itself.. Could you give some examples?

It depends on the use case. Some, for example, might want to add a label to the progress bar. Adding an aria-label to the progress bar component as a whole won't work since the component might represent one or two progress bars, and ideally both should be able to have their own labels. My point was that I don't want to restrict what attributes the user can or cannot set to the bars.

Is tabIndex really relevant for progress? Would you want it to be focusable when navigating with tab?

Depends on the use case. This is something we shouldn't hard code but instead let the users decide since behaviour like this can't really be generalized.

And what does status text mean?

It's text indicating the status of the operation the progress bar is representing. Something that would be displayed on a progress bar if using numbers was unsuitable.

@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 6, 2017

I think we should treat the different bars a little different

@slavab89 Thanks for raising those points, I'm cleaning the component with that feedback.

This is what I've been doing for now. However, since these are logically two independent progress bars having just an aria-valuenow prop for the buffer might not be enough. I'd rather have a mechanism for passing any attribute down to the div representing the buffer bar.

@tuukkao That sounds like a good idea, we could expose a primaryBarProps and secondaryBarProps propery.

oliviertassinari pushed a commit that referenced this issue Jan 29, 2018
* [LinearProgress] Add ARIA role & fix bugs

Closes #7054

* Fix tests

* Fix size limits
@oliviertassinari oliviertassinari added the component: progress This is the name of the generic UI component, not the React module! label Nov 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accessibility a11y component: LinearProgress The React component. component: progress This is the name of the generic UI component, not the React module!
Projects
None yet
Development

No branches or pull requests

3 participants