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

Color Viewer Slider: Improve high contrast support, touch AT support, and APG code quality guidelines support #1746

Merged
merged 28 commits into from
Mar 3, 2021

Conversation

jongund
Copy link
Contributor

@jongund jongund commented Feb 2, 2021

Updated color viewer slider based on the results slider update focus group meeting.

Changes:

  • Using APG coding practices and created as single object for color viewer.
  • Using SVG graphic elements, instead of HTML elements for the visual rendering of the sliders. Including using the fill of the rail rect to indicate the color the slider is associated with. The fill to the left of the thumb is the color associated with the slider.
  • Using currentColor value for SVG stroke property to support high contrast modes in operating systems.
  • Using event.key, instead of event.keyCode for responding to keyboard events.
  • Updated focus styling to have a ring around the thumb.
  • Using pointer instead of mouse events for moving the slider thumb to support touch devices.
  • Added group role and accessible name for the color viewer widget.

Preview link of color viewer slider

@carmacleod
Copy link
Contributor

@jongund

Nice! Sighted users on touch screens can drag the slider now.

Tested in Safari on iPhone and iPadOS, and Chrome on iPhone. I don't have any other touch devices.

I did notice a couple of problems:

  1. If the user taps on the value text above the thumb, the text gets selected. Then they can't drag the thumb. I'm not sure how to work around that - it seems to be fairly easy to do by accident on touch screens. Maybe CSS user-select: none; on the text element?
    image

  2. The focus ring for the thumb goes too close to the focus ring around the whole slider. This is not a problem on desktop, although it does look a bit "squished" (see first snapshot below), but on mobile the thumb focus ring bites into the slider focus ring (see second snapshot below).
    image
    image

@carmacleod
Copy link
Contributor

carmacleod commented Feb 3, 2021

@jongund There seem to be 2 extra files accidentally committed to this PR?

  • examples/treeview/js/treeview-navigation.js
  • test/util/assertAriaOwns.js

Edit 1: Never mind - sorry. The heading above those 2 files says, "Unchanged files with check annotations". So I see that they weren't accidentally committed. They're just suddenly failing for some reason. That's pretty annoying, because it's distracting and confusing. @nschonni Can we turn off whatever github feature is making those files show up in Jon's PR?

Edit 2: Never mind, Nick! I forgot that you already created PR #1728 to fix this!

@jongund
Copy link
Contributor Author

jongund commented Feb 3, 2021

@carmacleod
Can you give it another try, I did some update this morning.
Thank you for your reviews.

@carmacleod
Copy link
Contributor

@jongund

Wow, Jon! Not only does this work better on touch, it actually works really well! The sliders slide beautifully. 🎉
Tested in Safari and Chrome on iOS and iPadOS.

Pointer-events are awesome!

Also, as @patrickhlauke discovered, this actually works with VoiceOver now, too!!

Not quite sure how VoiceOver is controlling the slider, though. The AOM user actions table says that slider increment/decrement should synthesize arrow key events, but that doesn't explain what's going on here because:

  • the example was already handling arrow keys before these updates, and it didn't work with VoiceOver before
  • the example is now using pointerup/down/move instead of mouseup/down/move, and now VoiceOver works

So is the platform using synthesized pointer events to control the slider?
And if so, then as @patrickhlauke pointed out, where are the x, y change values coming from?

@cookiecrook Do you know if iOS/iPadOS/VO has implemented something to make ARIA sliders work?

@patrickhlauke
Copy link
Member

It does indeed seem that Safari/VO now fires faked keyboard events. The reason why the old/current slider example doesn't work is because VO sends left/right arrow fake key events for the horizontal slider, but the current slider example listens for up/down arrow.

TalkBack on Android doesn't seem to fire these fake events, but seems to have implemented some other sort of accommodation, allowing users to double-tap and hold, and then slide left/right while holding the finger down to change the value. However, it only announces the percentage of the slider, not the actual value text. Also, it seems for some reason - at least in my testing - that the entire slider doesn't get announced at all unless it's been double-tapped to activate it. Really not sure what is going on there...

@carmacleod
Copy link
Contributor

@patrickhlauke

The reason why the old/current slider example doesn't work is because VO sends left/right arrow fake key events for the horizontal slider, but the current slider example listens for up/down arrow.

Thanks for confirming that it's key events being faked! I see the difference between the old APG slider and the one in this PR.
The old slider did handle left/down (for decrement) and right/up (for increment), but it was still using event.keyCode (which I assume VO is not setting). As part of this modernization update for the old slider example, @jongund switched to using event.key, so that is likely the important difference there.

However, there's still something mysterious, because the preview slider in PR #1721 was also updated to use event.key and it handles left/right (and up/down) arrows, but it doesn't work with VO touch.

The only potentially relevant difference I can see is that the working slider in this PR hooks keydown on one additional element - an svg text element that is displaying the valuenow - but I don't see how that could be the important difference.

TalkBack on Android ... implemented some other sort of accommodation...

Good to know that TalkBack kinda-sorta works also.

However, it only announces the percentage of the slider, not the actual value text.

Seems to be a TalkBack bug. Even happens with range input, according to this blog post.

Also, ... the entire slider doesn't get announced at all unless it's been double-tapped to activate it.

Agreed that this is weird and seems buggy. Is this still the only way to report TalkBack bugs?
https://support.google.com/accessibility/contact/feedback

@jongund jongund changed the title Slider color viewer update Slider color viewer update based on focus group results Feb 8, 2021
@jongund jongund changed the title Slider color viewer update based on focus group results Slider color viewer update from focus group meeting results Feb 8, 2021
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 for Windows, Mac, iOS, and iPadOS,
and an approving Accessibility review for Jaws/NVDA in Chrome/Edge/Firefox.

Keyboard works well on Windows and Mac (used Fn+arrows for PageUp, PageDown, Home, End on Mac),
and left-right tap and drag gestures work well on iOS and iPadOS.

Nice work, Jon! This is vastly improved from what was there before. Thank-you so much for your perseverance!

Copy link
Contributor

@smhigley smhigley left a comment

Choose a reason for hiding this comment

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

Looks good! Just one issue in the tests, where it looks like some lines were (likely accidentally) removed.

examples/slider/js/slider-color-viewer.js Outdated Show resolved Hide resolved
test/tests/slider_slider-color-viewer.js Show resolved Hide resolved
Copy link
Contributor

@a11ydoer a11ydoer left a comment

Choose a reason for hiding this comment

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

Touch gesture in Android Talkback is a bit flaky(See above comments). However, it seems to be working. I was also able to "seek control" setting in Android to add actual percentage for RGB value.

@mcking65
Copy link
Contributor

mcking65 commented Mar 1, 2021

@jongund
I changed the heading levels inside the example so they are 3 and 4 instead of 2 and 3. That way, they both fall under the level 2 example heading. I didn't look at the css to see if that needed any adjusting .... pls fix if that impacted any styling.

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.

This is approving editorial review.

@mcking65
Copy link
Contributor

mcking65 commented Mar 1, 2021

I've finished edits for editorial review. It'd be good for people to have a final look at the warning at the top and the accessibility features just to make sure I don't have any typos or the like.

Otherwise, this is, I believe, ready to ship!

@jongund
Copy link
Contributor Author

jongund commented Mar 1, 2021

@jongund
The statement in the note "To use the currentColor value, the SVG graphics must be inline. " seems a little awkward to me, for example you can use currentColor value on referenced SVG graphics, it just doesn't change the styling for our purposes. Maybe something more like: "The use of currentColor value is only supported on inline SVG graphics at this time." But its up to you, I think developers will get the gist of the note as you wrote it.

@jnurthen
Copy link
Member

jnurthen commented Mar 1, 2021

@a11ydoer for the Android issues it is probably worth re-testing with chrome 90. There are a bunch of input type=range bug fixes in there which could help
https://issuetracker.google.com/issues?q=id%3A(175959402%7C175917356%7C175930703%7C175917355)

@carmacleod
Copy link
Contributor

@mcking65

I agree with @jongund that the inline SVG statement seems a little awkward. How about (fewest words possible 😄 )...

SVGs must be inline to use currentColor.

@mcking65 mcking65 changed the title Slider color viewer update from focus group meeting results Color Viewer Slider: Improve high contrast support, touch AT support, and APG code quality guidelines support Mar 3, 2021
@mcking65 mcking65 added Code Quality Non-functional code changes to satisfy APG code style guidelines and linters enhancement Any addition or improvement that doesn't fix a code bug or prose inaccuracy Example Page Related to a page containing an example implementation of a pattern labels Mar 3, 2021
@mcking65 mcking65 added this to the 1.2 Release 1 milestone Mar 3, 2021
@mcking65
Copy link
Contributor

mcking65 commented Mar 3, 2021

Thank you @jongund for leading the work to make this awesome set of improvements!!!

@mcking65 mcking65 merged commit dec293c into main Mar 3, 2021
@mcking65 mcking65 deleted the slider-color-viewer-update branch March 3, 2021 18:58
kdoberst pushed a commit to kdoberst/aria-practices that referenced this pull request May 14, 2021
… and APG code quality guidelines support (w3c#1746)

Makes the following changes to the color viewer slider example:
* Align with current APG code quality guidelines, including using a single object for the color viewer.
* Use SVG graphic elements instead of HTML elements for the visual rendering.
* Use currentColor  value for SVG stroke  property to support high contrast modes in operating systems.
* Use event.key  instead of event.keyCode for responding to keyboard events, which also provides support for iOS and Android screen readers.
* Updated focus styling to have a ring around the thumb.
* Use pointer  events instead of mouse  events, which provides support  for moving the slider thumb with touch.
* Added group  role and accessible name for the color viewer widget.

Co-authored-by: Matt King <[email protected]>
kdoberst pushed a commit to kdoberst/aria-practices that referenced this pull request May 14, 2021
… and APG code quality guidelines support (w3c#1746)

Makes the following changes to the color viewer slider example:
* Align with current APG code quality guidelines, including using a single object for the color viewer.
* Use SVG graphic elements instead of HTML elements for the visual rendering.
* Use currentColor  value for SVG stroke  property to support high contrast modes in operating systems.
* Use event.key  instead of event.keyCode for responding to keyboard events, which also provides support for iOS and Android screen readers.
* Updated focus styling to have a ring around the thumb.
* Use pointer  events instead of mouse  events, which provides support  for moving the slider thumb with touch.
* Added group  role and accessible name for the color viewer widget.

Co-authored-by: Matt King <[email protected]>
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 enhancement Any addition or improvement that doesn't fix a code bug or prose inaccuracy Example Page Related to a page containing an example implementation of a pattern
Development

Successfully merging this pull request may close these issues.

8 participants