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

Slider: Updated multithumb slider based on slider focus group meeting #1758

Merged
merged 37 commits into from
Aug 24, 2021

Conversation

jongund
Copy link
Contributor

@jongund jongund commented Feb 9, 2021

Updated multi-thumb slider example:

  1. Using SVG for the slider controls to support high contrast
  2. Using pointer events for touch support
  3. Added accessibility feature section
  4. Updated code use the APG coding practices
  5. Use event.key, instead of event.keyCode
  6. Removed flight price range multi-thumb slider
  7. Removed the use of aria-valuetext

Preview multi-thumb slider

Review checklist


Preview | Diff

@jongund jongund changed the title Slider multithumb update based on slider focus group meeting Updated multithumb slider based on slider focus group meeting Feb 9, 2021
@a11ydoer
Copy link
Contributor

a11ydoer commented Feb 9, 2021

@jongund @mcking65 Keyboard supports work well as documented. Great job.

@jongund
Copy link
Contributor Author

jongund commented Feb 9, 2021

@patrickhlauke
Can you checkout this multi-thumb slider example using pointer events when you get a chance.
I would be grateful to you for any feedback you have.

@carmacleod
Copy link
Contributor

Functional review for Windows Chrome/Edge/FF, Mac Safari/Chrome/FF: all work well with mouse and keyboard.
(Used Fn+arrows to test PageUp, PageDown, Home, End on Mac).

Functional review for iOS and iPadOS Safari/Chrome: works pretty well in Safari - it can take a few tries to start the drag unless I zoom in, but then it's ok. Chrome iOS/iPadOS seems to have the same erratic zoom problems as I saw in the temperature slider in #1755 (comment).

Interestingly, if I click on the "Open in CodePen" button in iOS Safari, I see the same erratic drag behavior in Safari CodePen as I saw for zoomed-in Chrome. I wonder if that's a clue, somehow.

Hmmm, one more clue. I think maybe Chrome is being unforgiving. If you drag even a fraction of a pixel in the up/down direction when dragging a thumb left or right, it cancels the drag.

So I still think this is a Chrome iOS/iPadOS problem, but I'm not 100% sure. If it is, I don't know if we can work around it.


One little visual nit: The first time after page reload that I tab to the max thumb, the visible text isn't centered in the focus outline. After typing arrow keys, or dragging with the mouse, it fixes itself.

image

Copy link
Contributor

@carmacleod carmacleod 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 an approving Functional review!
Nice work, @jongund !

I think the erratic drag when zoomed problem is an iOS Chrome bug, and we can open an issue for them after this is merged.

Note that there are still other pending reviews remaining: Accessibility, Visual Design, Editorial, Code and Test.

@patrickhlauke
Copy link
Member

General comment: I'd love to see these examples use

<meta name="viewport" content="width=device-width, initial-scale=1.0">

(though that will then also require some tweaking of the table styles to avoid them pushing out the content/leading to horizontal scrolling of the whole page) to make the experience a bit more mobile/small-screen friendly.

This example works very well on a Surface touchscreen, on iPhone/Safari, reasonably in iPhone/Chrome (seems the webview used for Chrome gets a bit flakey with respecting the touch-action once pinch-zoomed in ... this would become a non-issue once mobile-friendly viewport is added with the above meta). Also works well with iPhone/VoiceOver.

Android/Chrome/TalkBack announces percentage only when focusing a thumb. Users can change value by double-tap-and-hold-ing the thumb, then moving their finger. Changing percentage value is announced when moving.

Overall, combined with the notice/warning that support may not be complete, this is all very cool and, I'd say, good to release.

@patrickhlauke
Copy link
Member

if we're ok with the new rewording from #1746 (comment) suggest adding it here as well

@carmacleod
Copy link
Contributor

@patrickhlauke

if we're ok with the new rewording from #1746 (comment) suggest adding it here as well

That's the plan. @mcking65 changed the words a bit, but definitely incorporated your feedback. Thank-you!
Color slider is set to be merged today (with new warning words), assuming all goes well in the rename of master branch to main.
Thermostat is up next. :)

Base automatically changed from master to main March 3, 2021 18:13
<main>
<h1>Horizontal Multi-Thumb Slider Example</h1>
<div class="advisement">
<p> <p><strong>WARNING!</strong> 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 incrementing and decrementing 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.</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<p> <p><strong>WARNING!</strong> 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 incrementing and decrementing 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.</p>
<p><strong>WARNING!</strong> 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 incrementing and decrementing 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.</p>

Copy link
Contributor

@mcking65 mcking65 left a comment

Choose a reason for hiding this comment

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

Editorial revision complete; this is an approving editorial review.

However, since we'll merge the seek slider before this one, there will be one more update to the list of similar example links.

@mcking65
Copy link
Contributor

@jongund

There are some CSS lint failures.

@mcking65 mcking65 requested a review from jesdaigle May 25, 2021 17:37
@jongund
Copy link
Contributor Author

jongund commented May 25, 2021

@mcking65
The CSS linting error is fixed

@mcking65
Copy link
Contributor

mcking65 commented Jun 1, 2021

@shirsha
You earlier raised HCM issues. They have been resolved ... we believe. Will you please re-review and approve if you agree?

@shirsha
Copy link

shirsha commented Jun 2, 2021

Thanks @jongund
This is working fine in high contrast mode.
Here are few things to discuss:

  • Narrator in scan mode: User is unable to change the slider value for all examples. User is able to change the slider value when native HTML range element is used.

  • The aria-valuemin and aria-valuemax is changed based on the thumb nail.

For eg:
For left thumb (minimum price) , the aria-valuemax is 243 (current max value selected by the user). Should the max value be 400 ?

<g class="minimum" role="slider" tabindex="0" aria-valuemin="0" aria-valuenow="110" aria-valuemax="243" aria-label="Hotel Minimum Price in US dollars">
                ......
            </g>

For right thumb ( Maxmium price range), the aria-valuemin is 110 (current min value selected by the user). should the min value be zero?

<g class="maximum" role="slider" tabindex="0" aria-valuemin="110" aria-valuenow="243" aria-valuemax="400" aria-label="Hotel Maximum Price in US dollars">
.....
            </g>
  • The screenreader JAWS/Chrome in browse mode narrates the previous value instead of the current value for min or max value.
    Attached the video below:
JAWS.narration.for.min.and.max.mp4

@JAWS-test
Copy link

For left thumb (minimum price) , the aria-valuemax is 243 (current max value selected by the user). Should the max value be 400 ?

The question is whether aria-valuemin/max should indicate the theoretically possible number or the currently possible one. In this example it is the currently possible one (which is the better solution in my opinion). The question is not answered by the ARIA specification and HTML can't be used as a reference, because there are no multi-thumb sliders.

@mcking65
Copy link
Contributor

mcking65 commented Jun 2, 2021

@shirsha wrote:

This is working fine in high contrast mode.

Thank you Siri.

  • Narrator in scan mode: User is unable to change the slider value for all examples. User is able to change the slider value when native HTML range element is used.

That is a Narrator issue, thus the warning at the top of the page.

  • The aria-valuemin and aria-valuemax is changed based on the thumb nail.

This is by design. The thumbs are not allowed to cross, and this is communicated to users via min and max.

  • The screenreader JAWS/Chrome in browse mode narrates the previous value instead of the current value for min or max value.

I think this is a JAWS bug because if you press insert+escape, the correct value is read.

@shirsha, based on this information, do you approve merging? If yes, could you please go to the files tab and submit an approving review that lists the types of tests you completed?

Copy link

@shirsha shirsha left a comment

Choose a reason for hiding this comment

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

Thanks @mcking65 for answering my questions.

@jongund
Copy link
Contributor Author

jongund commented Jul 27, 2021

@mcking65
What is left before this PR can be merged?

Copy link

@dinc5150 dinc5150 left a comment

Choose a reason for hiding this comment

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

Can I suggest making the '$' more configurable by adding a data-prefix and data-suffix attribute to each slider thumb.

Adding an optional

data-prefix="$"
data-suffix=".00"

to each slider thumb and then replace dollarValue = '$' + value; with

var prefix = (sliderNode.dataset.prefix) ? sliderNode.dataset.prefix : "";
var suffix = (sliderNode.dataset.suffix) ? sliderNode.dataset.suffix : "";
dollarValue = prefix + value + suffix;

@mcking65 mcking65 added Code Quality Non-functional code changes to satisfy APG code style guidelines and linters Example Page Related to a page containing an example implementation of a pattern Touch Related to gap in guidance about touch interaction in user interfaces labels Aug 24, 2021
@mcking65 mcking65 added this to the 1.2 Release 1 milestone Aug 24, 2021
@mcking65
Copy link
Contributor

@dinc5150 asks:

Can I suggest making the '$' more configurable by adding a data-prefix and data-suffix attribute to each slider thumb.

Adding an optional

data-prefix="$"
data-suffix=".00"

to each slider thumb and then replace dollarValue = '$' + value; with

var prefix = (sliderNode.dataset.prefix) ? sliderNode.dataset.prefix : "";
var suffix = (sliderNode.dataset.suffix) ? sliderNode.dataset.suffix : "";
dollarValue = prefix + value + suffix;

David, I assume this would be to support other currencies? If so, that would be a great suggestion if our goal were to make re-usable components. Building a re-usable component library is specifically a non-goal of APG. The primary goal is illustrating accessibility practices as clearly as possible, so we intentionally avoid adding complexity to the code that you would find in production-ready component libraries. Even though your suggestion is simple and represents trivial complexity for the level of configurability benefit, we'd still avoid it unless we needed that configurability to illustrate an accessibility feature.

Please let me know if I am misunderstanding your intent.

@mcking65 mcking65 merged commit 9e036d7 into main Aug 24, 2021
@mcking65 mcking65 deleted the slider-multithumb-update branch August 24, 2021 15:20
@dinc5150
Copy link

Thanks @mcking65 ,

Yes, my intent was to make the code more useable, and wasn't aware that it wasn't something that the APG wanted. My use case for the component was to append "mm" to the number instead of using currency.

If the goal is to illustrate as clearly as possible without adding complexity, rather than a reusable component, I would recommend removing the $ all together as the currency does not add value to the code or best practices.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Code Quality Non-functional code changes to satisfy APG code style guidelines and linters Example Page Related to a page containing an example implementation of a pattern Touch Related to gap in guidance about touch interaction in user interfaces
Development

Successfully merging this pull request may close these issues.