-
Notifications
You must be signed in to change notification settings - Fork 1.1k
USWDS - Range: Enhance SR callouts #5472
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
Conversation
a11y reviewI'm still working on a unit test for these JS changes but flagging @amycole501 and @alex-hull for the initial accessibility review! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really clear audio descriptions happening for the slider. In JAWS in Chrome I hear "forty percent of one hundred" for example which is great. The only issue I foresee is someone pressing and holding an arrow key and thereby hearing nothing until the audio catches up to the keyboard command.
I believe the keyboard input isn't working as expected. Test is failing when checking updated value
I was able to get the unit test working! I feigned an input using: el.dispatchEvent(new KeyboardEvent("input", { bubbles: true })); @amyleadem @mejiaj Let me know what you both think! The test listens for an input the same way the component does. If we decide to add further event listeners for keyboard inputs we can dig further, but may not be necessary thanks to native keyboard input interaction. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note
Hiding this comment because it was related to a local "Quick nav" setting in my VoiceOver. This comment no longer applies.
Previous comment about screen reader testing failures
Hi @mahoneycm,
I like the possibilities of this improvement but I ran into some screen reader issues during testing:
- Confirm the value updates as expected in Chrome
- With keyboard navigation in Chrome with VoiceOver, I see that the
aria-valuetext
is updating as expected, but sometimes the readout does not happen as expected. Sometimes it will get stuck on a value, skip a value, or read the wrong value. - Generally the Chrome VoiceOver experience is greatly improved over
develop
with this PR, but users would likely be confused by the incorrect readouts. - This behavior matches what I found in USWDS - Range: Remove redundant ARIA attributes PR
- With keyboard navigation in Chrome with VoiceOver, I see that the
- Confirm the value updates as expected in Safari
- With keyboard navigation in Safari with VoiceOver, I cannot change the value from “20 out of 100” even though visually the value appears to change
- This is a regression because the values are read out as expected in
develop
- This is also a regression from the USWDS - Range: Remove redundant ARIA attributes PR
- With keyboard navigation in Safari with VoiceOver, I cannot change the value from “20 out of 100” even though visually the value appears to change
- Confirm the value updates as expected in Firefox
- With keyboard navigation in Firefox with VoiceOver, I can advance the value successfully, but I run into trouble when I try to move backwards. Sometimes the value doesn’t change at all, sometimes it jumps all the way to 0.
- Generally the Firefox VoiceOver experience is greatly improved with this PR, but the quirks still prevent it from being fully usable
- This behavior matches what I found in USWDS - Range: Remove redundant ARIA attributes PR
- Confirm the value updates as expected in iOS Safari
- With touch navigation in iOS Safari with VoiceOver gestures, it will only read “20 percent of 100” when I swipe up or down as instructed (Similar to macOS safari).
- This is a regression. In
develop
, the value changes as expected with up/down swipe. - This is also a regression from the USWDS - Range: Remove redundant ARIA attributes PR
I also had some additional questions about Storybook and the GitHub issues:
- For Storybook, I am a bit confused about the unit test story. Is it necessary? Is it possible to consolidate it with the main range story?
- I am wondering if it makes sense to focus on USWDS - Range: Remove redundant ARIA attributes first and then look at USWDS - Range: Enhance SR callouts. Either that or we combine the PRs (Which I know makes for some messier testing). It just seems like this PR builds on the other one and there is a lot of overlap with testing results. Curious what you think!
I haven't yet gotten to take a deeper dive into the code. I will look more closely once the behavioral issues are figured out. Let me know if you have any questions!
@amyleadem I still need to conduct some more iOS testing but I did swap the I was not able to replicate the other browser bugs but I believe that is due to the "Quick nav" mode we discussed in #5413. RE: iOS VoiceOver, I did find this note on the W3C a11y patterns range slider page.
So we may need to include swipe specific event listeners to trigger key events. Have you worked on any other tickets that have had to have specific swipe inputs? |
@amyleadem I'm able to confirm the regression that I cannot use the single drag functionality to increase or decrease the slider on this branch, but I was able to on I was able to use the I checked out Android TalkBack while I was at it and luckily that screen reader was able to handle the increases and callouts! Since this is a regression on iOS, it's probably a good idea to get this sorted before merge. Do you have any experience with JS changes preventing iOS VoiceOver commands? |
@amyleadem Ok so had a weird hunch about it being a Storybook JS initialization issue and I think it might be. I tested on an iPad again today and found the same issue, then refreshed the page, and tried again and it worked as expected! It's odd that it prevented the input though so definitely something to keep an eye out for. Let me know if it works for you after a refresh and we can discuss next steps! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mahoneycm, I am still working on reviewing the code but wanted to report that after testing the component with quick nav turned off, the component is now working beautifully across browsers with VoiceOver! 🎉 🎉 🎉
One note: I feel like it is safe to remove the unit test story. The unit test and default stories seem to be redundant. What do you think?
Here is what I tested:
- Confirm the value updates as expected in Chrome
- Confirm the value updates as expected in Safari
- Confirm the value updates as expected in Firefox
- Confirm the value updates as expected in iOS Safari
- All controls work as expected
- Was able to successfully customize unit, min, max, and step
- Ran
npm start
npm run build:html
, andtest:unit
without error
I'll come back later to review the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking good, @mahoneycm 👍
I had just a few notes:
- I added a few questions and comments to
index.js
. They all are pretty small items. - Unit tests look good. Way to figure it out!
- In the top of the PR description, can you add details about the new
data-optional-unit
attribute? It would be helpful to explicitly spell out a description of the attribute as well as default values, etc.
Seems redundant; only calls updateCallout() Functionality unchanged after replacing it with updateCallout()
@mejiaj Made a couple updates on this PR. Updated the description and testing instructions as well. Bumping for review when you have the time! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything is working well! Just a couple of small notes in the comments below. There are also some merge conflicts that need to be cleaned out.
Also removes redundant role from slider test templates to match twig file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mahoneycm the component is working well. Thanks for removing the extra space from the readout.
I have resolved the comments that have been addressed and left a comment on the remaining comment. I also found a few more small items. Let me know if you have any questions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great, @mahoneycm! I tested this successfully on Firefox, Chrome, and Safari with VoiceOver.
@amycole501 was also able to re-test this on NVDA and JAWS today and confirmed that the readout is still working as expected.
I resolved all review comments after confirming that they have been addressed. There is only one open comment at this time related to teardown, but I don’t believe any action is needed other than considering if we do in fact need a teardown method here.
I confirmed the following:
- Confirmed that the data attributes correctly update the unit and preposition text in the
aria-valuetext
readout - Confirmed that the data attributes follow a predictable naming structure
- Confirmed that the
aria-valuetext
value updates when the slider value changes in Chrome, Firefox, and Safari - Confirmed that the story controls are logical and work as expected
- Confirmed unit tests as logical and work as expected
- Incorporates the attribute updates from #5413
@mejiaj Bumping for re-review when you have time! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I appreciate the thorough code comments. No issues found in functionality.
Thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work!
Summary
Added optional
data-text-unit
anddata-text-preposition
attributes to range slider. It now reads{{ value }} of {{ max }}
when calling out the range slider value. Theof
can be changed usingdata-text-preposition
as needed for use cases or different languages.Additionally, this PR enhances the storybook controls to easily allow users to set the min, max, step, and newly added data attribute values.
Breaking change
Existing users will not need to change anything by default but will need to edit markup if they’d like to include the new unit feature.
Related issue
Closes #5431
Related pull requests
Changelog and guidance update →
USWDS - Range: Remove redundant ARIA attributes #5413 →
Preview link
Range slider →
Problem statement
When using the range slider with a screen reader, users are left without context. Because sliders rely on context from their implementation, we should allow devs to provide additional context to for screen readers as well as some way to let users know how far along the slider the handle is.
Solution
This PR improves SR callouts in two ways.
The first, is adding the max value to the callout. Now when the slider moves, it will read the current value, then “of max value” where
max value
is the numerical max value.Next, is adding an additional
data-text-unit
attribute. This attribute can be set to any unit the user can choose to include. Some examples may include:Essentially dependent on the use case of the slider.
When included, the screen reader will update to read out along the following examples:
With the addition of the
data-text-preposition
attribute, devs can change the language between the units and the total to fit whatever language they are developing for.For example:
null
40 percent of 100
Spanish:
40 por ciento de 100
New data attributes
data-text-unit
- Set thedata-text-unit
attribute to include a unit with screen reader callouts. This provides additional context to assistive technology users. It is empty by default. Adding a value will be read as: "x units of y".data-text-preposition
- Set the preposition between units and total. By default, we'll useof
(i.e. 40 percentof
100). This will allow devs to update the verbiage depending on language.Further consideration
Devs should still provide proper context before users reach the slider. It is important for both visual, and screen-reader users, that the usability guidance is followed.
This is also a JS solution. JS is required to set update the
aria-valuetext
attribute, which is standard across various accessible slider components researched during this work.You can view some examples from W3C WAI.
Testing and review
data-text-unit
data-text-prepostion
emptyof
data-text-preposition
of
is replaced with your new valueTesting checklist