Skip to content

Conversation

mahoneycm
Copy link
Contributor

@mahoneycm mahoneycm commented Aug 29, 2023

Summary

Added optional data-text-unit and data-text-preposition attributes to range slider. It now reads {{ value }} of {{ max }} when calling out the range slider value. The of can be changed using data-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.

Important
The new JS file does not contain a teardown method at this time. Compared to other components, it appears we only teardown dynamic components, remove event listeners, and reset logic-related values.

It did not seem necessary to add a teardown to remove the aria-valuetext attribute at this time.

Breaking change

⚠️ This is potentially a 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:

  • Percent
  • Temperature
  • Stars (rating)
  • Decibles
  • etc…

Essentially dependent on the use case of the slider.

When included, the screen reader will update to read out along the following examples:

  • “30 percent of 100”
  • “72 degrees Fahrenheit of 90”
  • “3.5 stars of 5”
  • “3 pizza slices of 8”
  • etc.

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:

Attribute Value
data-text-unit Percent
data-text-preposition null
Output 40 percent of 100

Spanish:

Attribute Value
data-text-unit por ciento
data-text-preposition de
Output 40 por ciento de 100

Note
Screen readers will still rely on system settings to read numbers in correct language

New data attributes

data-text-unit - Set the data-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 use of (i.e. 40 percent of 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.

Note
The examples at the above link are all nonsemantic examples, and are built using divs, ARIA attributes, and JS

Testing and review

  1. Visit Range slider →
  2. In controls, add a data-text-unit
  3. Leave data-text-prepostion empty
  4. Refresh page
  5. Activate screen reader
  6. Interact with the range slider
  7. Confirm callouts have been improved
  8. Confirmed preposition defaults to of
  9. In controls, add a data-text-preposition
  10. Interact with the range slider
  11. Confirm of is replaced with your new value
  12. If desired, play around with the unit to listen to other use cases
  13. Feel free to play with min, max, and step if you want to customize the range slider for different use cases

Testing checklist

  • If unit is present, screen reader reads out unit with value
  • If unit is not present, screen reader still reads value and max value
  • Screen reader callouts are justifiably enhanced
  • Context is improved for screen reader users

Note
Sometimes storybook has trouble initializing JS. If changes aren’t taking place or if readout is not reading properly, try refreshing the page and listening again.

@mahoneycm
Copy link
Contributor Author

a11y review

I'm still working on a unit test for these JS changes but flagging @amycole501 and @alex-hull for the initial accessibility review!

Copy link

@amycole501 amycole501 left a 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
@mahoneycm
Copy link
Contributor Author

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.

Copy link
Contributor

@amyleadem amyleadem left a 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
  • 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
      image
    • 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
  • 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!

@mahoneycm mahoneycm requested a review from amyleadem September 20, 2023 14:23
@mahoneycm
Copy link
Contributor Author

@amyleadem I still need to conduct some more iOS testing but I did swap the input event listener for change which should resolve the Safari bug.

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.

Warning
Some users of touch-based assistive technologies may experience difficulty utilizing widgets that implement this slider pattern because the gestures their assistive technology provides for operating sliders may not yet generate the necessary output. To change the slider value, touch-based assistive technologies need to respond to user gestures for increasing and decreasing the value by synthesizing key events. This is a new convention that may not be fully implemented by some assistive technologies. Authors should fully test slider widgets using assistive technologies on devices where touch is a primary input mechanism before considering incorporation into production systems.

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?

@mahoneycm
Copy link
Contributor Author

@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 develop.

I was able to use the double tap, hold, then drag to increase or decrease on both branches but it didn't hear callouts on either.

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?

@mahoneycm
Copy link
Contributor Author

@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!

Copy link
Contributor

@amyleadem amyleadem left a 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, and test:unit without error

I'll come back later to review the code.

Copy link
Contributor

@amyleadem amyleadem left a 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()
@mahoneycm mahoneycm requested a review from amyleadem October 19, 2023 15:50
@mahoneycm
Copy link
Contributor Author

@mejiaj Made a couple updates on this PR. Updated the description and testing instructions as well. Bumping for review when you have the time!

@mahoneycm mahoneycm requested a review from mejiaj October 30, 2023 15:17
Copy link
Contributor

@amyleadem amyleadem left a 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.

Copy link
Contributor

@amyleadem amyleadem left a 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.

@mahoneycm mahoneycm requested a review from amyleadem November 1, 2023 15:31
Copy link
Contributor

@amyleadem amyleadem left a 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

@mahoneycm
Copy link
Contributor Author

@mejiaj Bumping for re-review when you have time!

Copy link
Contributor

@mejiaj mejiaj left a 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!

Copy link
Contributor

@thisisdano thisisdano left a comment

Choose a reason for hiding this comment

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

Nice work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

USWDS - Bug: Range slider screen reader experience lacks context
5 participants