foundation / foundation-sites

The most advanced responsive front-end framework in the world. Quickly create prototypes and production code for sites that work on any kind of device.
https://get.foundation
MIT License
29.65k stars 5.48k forks source link

Documentation: Progress bar element's tabindex / focus ability #12043

Closed ahukkanen closed 3 years ago

ahukkanen commented 4 years ago

Description

In the Progress Bar documentation the examples guide to add a tabindex attribute to the progress bar elements that makes them focusable.

The current example looks like this:

<div class="progress" role="progressbar" tabindex="0" aria-valuenow="50" aria-valuemin="0" aria-valuetext="50 percent" aria-valuemax="100">
  <div class="progress-meter" style="width: 50%"></div>
</div>

Why is this?

We just got a report of an accessibility analysis on a website using Foundation progress bar as instructed in the documentation and one of the reported issues stated that the progress bar should not be focusable.

Possible Solution

Remove the tabindex attribute from the progress bar documentation examples unless there is a valid reason for that.

If there is a valid reason for making it focusable, this valid reason should be documented with the documentation.

Checklist

ahukkanen commented 4 years ago

Another issue that could be added to the documentation is that the valuemin should always be 0 and valuemax should always be 100 for percentage progress bars.

We had a progress bar where the valuemin was marked as 30 that being the "minimum" the user has to select but some of the screen readers will then interpret the selected value incorrectly.

Example:

I know there are valid use cases where the value range of a progress bar can be something else than 0-100 but it would be really beneficial for developers if this was documented with the progress bar element.

DanielRuf commented 4 years ago

Hi @ahukkanen

Another issue that could be added to the documentation is that the valuemin should always be 0 and valuemax should always be 100 for percentage progress bars.

This is something that the users have to do / set, you can also use negative values. This is valid afaik.

I know there are valid use cases where the value range of a progress bar can be something else than 0-100 but it would be really beneficial for developers if this was documented with the progress bar element.

Not sure what you want to document here. We just style the progress bar element and these are only examples.

You are partially right, the tabindex should not be there. Do you want to provide a PR to fix this?

See also https://www.w3.org/TR/wai-aria-1.1/#progressbar https://www.w3.org/TR/wai-aria-1.1/#aria-valuemin https://pressbooks.library.ryerson.ca/wafd/chapter/progress-bars/ https://www.digitala11y.com/progressbar-role/

ahukkanen commented 4 years ago

@DanielRuf

Not sure what you want to document here.

Something along these lines:

The range defined for the progress bar needs to describe the full range of its
possible values. Typically this is 0-100 describing a progress percentage. In this
case you would always set `valuemin="0"` and `valuemax="100"`. Even if you have a
minimum required value for the progress bar, it does not change the described range.

For further information, see:
https://www.w3.org/TR/wai-aria-1.1/#progressbar
https://www.w3.org/TR/wai-aria-1.1/#aria-valuemin
https://www.w3.org/TR/wai-aria-1.1/#aria-valuemax

This particular issue that I described is not clear from the W3C docs or the other resources you linked there.

The W3C documentation particularly says:

If the aria-valuenow has a known maximum and minimum, the author SHOULD provide properties for aria-valuemax and aria-valuemin.

In our case we had a "known minimum" which was 30%. This is still not correct because even in that situation aria-valuemin would need to be 0 because it describes the range minimum, not the "known" minimum value we require.

DanielRuf commented 4 years ago

Not sure I can follow. Our docs do not and can not cover all the different cases for ARIA attributes.

If you think we can improve the docs without adding too much text (keep it simple) a PR with these changes is very welcome.

ahukkanen commented 4 years ago

@DanielRuf The point is that I would expect many cases that would use progress bar elements would use it as a percentage indicator when the possible values are 0-100 as I described above.

Now, think of situation that you have to display a progress bar with the user having to fill at least 30% of the total value, e.g. a situation where you would have to pay at least 30% of your purchase to be eligible for a gift card discount.

In this situation a developer could "intuitively" think that let's set valuemin="30" because this is what I want from users. It is not clear from the w3.org documentation that when you do this decision, you will also change the whole range of the progress bar to 70 (100-30 = 70).

Therefore, now:

I understand it's not really Foundation's job to explain the unclear w3.org specification but it would greatly help the developers if they did not have to look for multiple sources when developing progress bars.

If you have a better way of wording it, I'm more than happy to hear alternatives. I would want to make it perfectly clear, as this is a bug I had to fix because of this misunderstanding from another developer.

EDIT: This is also particularly hard for the developers to see in action because most of the developers unfortunately don't think about screen reader users when developing. Therefore, they miss this bug and it goes live until a blind user will complain about it.

DanielRuf commented 4 years ago

In this situation a developer could "intuitively" think that let's set valuemin="30" because this is what I want from users. It is not clear from the w3.org documentation that when you do this decision, you will also change the whole range of the progress bar to 70 (100-30 = 70).

Makes not much sense and I would not do that as developer. I would set 0 - 100 and set the value to 30. We just style the native progress element. We can not and do not explain the whole element specification.

DanielRuf commented 4 years ago

If you have a better way of wording it, I'm more than happy to hear alternatives. I would want to make it perfectly clear, as this is a bug I had to fix because of this misunderstanding from another developer.

PRs which add some short notice are very welcome.

ahukkanen commented 4 years ago

@DanielRuf I've addressed these issues at #12085. Please review.

Makes not much sense and I would not do that as developer. I would set 0 - 100 and set the value to 30. We just style the native progress element. We can not and do not explain the whole element specification.

I really think it deserves a brief section in the documentation nonetheless which I've included in the PR. This is an actual bug I fixed and I also had to learn it the "hard way" myself where the issue is before I realized how the screen reader is actually behaving.

The original issue was that the original developer had defined a progressbar with aria-valuemin="30" and aria-valuemax="100" because the user was required to fill at least 30% of the progress (as explained above). Now, the screen reader was announcing weird progress numbers as the aria-valuenow was updated and when I was testing it myself, I initially had no idea why this is happening, even after closely inspecting the code (everything seemed "right" to me and I was looking closely at the examples on the Foundation docs).

Even if this is 100% perfectly clear to you, it may not be clear for everyone, including me and the developer who had done it like this. The w3.org docs are not perfectly clear regarding this matter or if they are, feel free to refer an exact location of those docs where this is explained clearly enough. It should not hurt anyone to provide information that developers might otherwise misunderstand.