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 thermostat update from focus group meeting results #1755

Closed
wants to merge 47 commits into from

Conversation

jongund
Copy link
Contributor

@jongund jongund commented Feb 8, 2021

I have updated the thermostat slider example using the vertical slider and two text sliders:

  • Updated example to use SVG graphics elements
  • Added high contrast support
  • Updated code to use APG coding practices
  • Using event.key instead of event.keyCode
  • Use pointer events, instead of mouse events
  • Use the presentation role on SVG element
  • Setting the CSS property forced-color-adjust to auto on the SVG elements.
  • Using stroke-opacity and fill-opacity instead of transparent values for setting stroke and fill colors for the SVG rect used for focus ring for high contrast mode.

Preview slider thermostat example

Review checklist


Preview | Diff

@jongund
Copy link
Contributor Author

jongund commented Feb 9, 2021

@patrickhlauke
Can you checkout this 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's a little difficult to start the drag on the Fan and Heat/Cool sliders unless I zoom in, but then it's ok. Chrome seems to have zoom problems, because it works ok until I zoom in to try to have better control, and then the dragging becomes erratic (i.e. it just stops randomly, even though I haven't lifted my finger). I suspect that this is a Chrome issue, and probably not anything to do with the slider code. We can open a chromium issue after this PR is merged.


I recommend using a div or span to display the temperature at the top of the slider. The problem with using an output element is that it acts as a live region, and so the temperature is spoken twice for every movement of the slider (once for the new slider value, and once again for the live region).

@patrickhlauke
Copy link
Member

patrickhlauke commented Feb 13, 2021

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 only change the value of the vertical slider properly, by double-tap-and-hold-ing the thumb, then moving their finger. Changing percentage value is announced when moving. The Fan and Heat/Cool sliders can't be changed using TalkBack (trying double-tap-and-hold and then moving the finger on the screen doesn't cause any change in value, compared to the HTML range example one).

The fact that it only announces percent values becomes confusing/an issue in the Fan and Heat/Cold examples. Hearing that "Fan" is at "25 percent" is even more meaningless to users in this case. Yes, it's a shortcoming in TalkBack of course, but at least in the case of a numeric slider that represents a range of values, it can "almost" be understandable (though without knowing min/max either, yeah, it's of low information value for these users).

Incidentally, I'm actually wondering if using sliders for these two cases is semantically appropriate. They're not really offering a sliding scale of values that the user jumps between, but conceptually they are mutually exclusive radio buttons?

The touch-action values are set on the outer containers. Particularly due to the size of the vertical one, this can actually cause problems for touch users in general (non-AT), as the whole <div> suppresses vertical scrolling. Once a user is sufficiently zoomed in, they may find themselves unable to actually scroll as their vertical scroll gesture is blocked by the touch-action: pan-x. I'd suggest adding the touch-action to the .slider-group, rather than the .slider-thermostat-vertical. Generally best to just add touch-action to the "most specific / minimal" element to avoid suppressing regular actions unnecessarily.

as with the HTML range example, the fact that the slider for "Fan" was visually shorter than the labels/buttons underneath it was confusing, visually

and again, for some reason, Chrome/TalkBack also didn't announce the actual displayed temperature in the <output>. It just announced the element as "Temperature, status" without announcing the value. Seems TB is quite confused/broken, sadly.

@jongund
Copy link
Contributor Author

jongund commented Feb 16, 2021

@carmacleod
@patrickhlauke
I added the meta tag and removed the output element.
I am not sure I understand the labeling issues, the labels are centered on the position of the thumb when that value is selected.
Can you point me to an example of what you were expecting?

Also the fan and heat/cool could be a radio group, but we are trying to demonstrate use of aria-valuetext and it looks like a slider. So this is the age old question, do ARIA roles and behaviors describe visually what a control looks like (e.g. a slider) or its functional properties (e.g. a radio group).

@patrickhlauke
Copy link
Member

patrickhlauke commented Feb 16, 2021

I added the meta tag

still not seeing it in the markup ... think that was missed out in this commit 96d161b

I am not sure I understand the labeling issues, the labels are centered on the position of the thumb when that value is selected.
Can you point me to an example of what you were expecting?

screenshot in Safari/iOS shows the last label way off track. I was looking at that when I mentioned it in my comment, hadn't even noticed that in other cases it's not as dramatic.

Safari/iOS screenshot

in Android/Chrome, it looks mostly ok. Android/Firefox, it's all looking a bit jumbled.

Android/Firefox screenshot

And even on Windows/Chrome and Windows/Firefox, it doesn't quite "feel" centered, at least not at the start. Feels more left-aligned (added a red vertical line there for reference)

chrome-windows screenshot

So this is the age old question, do ARIA roles and behaviors describe visually what a control looks like (e.g. a slider) or its functional properties (e.g. a radio group)

as an AT user I may possibly be expecting a slider to slide between different numerical values. I don't think it makes quite semantic sense if not associated with some form of value or scale of a kind. Maybe just having the fan speed slider, and removing the auto value, so it essentially does represent fan speed (from no speed to high speed)?

@carmacleod
Copy link
Contributor

@jongund In case it's helpful, I noticed the labels-vs-rail-width problem in the range slider as well: #1757 (comment)

Also noticed that it is NOT a problem in the currently-released thermostat slider example: https://w3c.github.io/aria-practices/examples/slider/slider-2.html

Maybe there's a clue in there somewhere. :)

@jongund
Copy link
Contributor Author

jongund commented Feb 17, 2021

@patrickhlauke
That is a good idea about removing the "Heat/Cool" slider and changing the values of the "Fan" slider.
I will also update the "Fan" slider to use SVG text elements for the labels, that should fix some of the alignment issues in mixing HTML and SVG between browsers.
Thanks so much for your feedback.

@jongund
Copy link
Contributor Author

jongund commented Feb 18, 2021

@patrickhlauke
@carmacleod

I have completed the updates:

  1. Removed the Heat/Cool slider, since it is not a range.
  2. Updated Fan slider to use off, low, medium and high values.
  3. Changed button elements to SVG role=button for more consistent positioning across browsers and OSes.
  4. Added the meta element,

Would appreciate your feedback on the changes whenever you get a chance.

@patrickhlauke
Copy link
Member

@jongund that's looking really good. thanks for taking on board the feedback about the heat/cool and fan slider - i think this now makes very nice logical sense. works great with iOS/VO.

Android/TalkBack in both Chrome and Firefox is ... weirdly broken, but in roughly the same way as before (announcing slider values in percent only, and for some reason in Chrome the Fan slider just can't be operated with double-tap-and-hold-and-then-drag type gesture - it goes into text selection instead; Firefox lets you operate both with this gesture, but announces nothing when things change). Don't think it's anything related to your approach here, but just overall lack of polish/support for this sort of thing in TalkBack...

@jongund
Copy link
Contributor Author

jongund commented Mar 23, 2021

@jscholes
I have updated the slider thermostat example to use role=presentation on the SVG elements and aria-hidden on the fan labels. This seems to fix the problem with NVDA reading extra information.

.vnurc Outdated
@@ -20,3 +20,5 @@ The “row” role is unnecessary for element “tr”.
Attribute “aria-activedescendant” value should either refer to a descendant element, or should be accompanied by attribute “aria-owns”.
# https://github.com/w3c/aria-practices/issues/1678
Section lacks heading. Consider using “h2”-“h6” elements to add identifying headings to all sections.
# https://github.com/validator/validator/issues/1096
An “svg” element can have a “role” attribute.
Copy link
Contributor

Choose a reason for hiding this comment

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

The supressions work by string matching, so it's the exact error message (or a regex)

Suggested change
An “svg” element can have a “role” attribute.
Bad value “presentation” for attribute “role” on element “svg”.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @nschonni

@mcking65
Copy link
Contributor

@jongund wrote:

@jscholes
I have updated the slider thermostat example to use role=presentation on the SVG elements and aria-hidden on the fan labels. This seems to fix the problem with NVDA reading extra information.

@jscholes
While I understand the desire not to have the clutter of the fan labels, I don't think hiding them is good practice. And, while they are clickable, I don't think that presenting them as static text to screen reader users violates anything in WCAG. It is no different from a clickable label element on a form field.

Why is using aria-hidden not good?

  1. The text is visible on screen. We do not want to encourage hiding visible text.
  2. Sighted screen reader users should be able to tap the label and get the same effect as anyone else.

@jscholes
Copy link
Contributor

jscholes commented Mar 23, 2021

@mcking65

It is no different from a clickable label element on a form field.

Generally speaking, clicking the label of a form field doesn't change that field's value (with the possible exceptions of a checkbox or radio button). Focus is also placed on the field in question, so in the worst case scenario the state of a checkbox or radio will change but the user will be made aware of that change immediately and in context. There is also a one-to-one relationship between labels and form fields, not four-to-one.

Here, the value of the slider does change, but the only thing that seems to happen is that the screen reader (NVDA at least) announces the new value without really enforcing the fact that it has changed. The textual values are positioned after the element with role="slider" in the DOM, so it's not even likely that a screen reader user will discover the change during the natural progression of continuing to read the page.

I'm happy that the main concern of the graphic announcement has been addressed. But I maintain that having four controls on a page that change the value of another element, with those four controls not having accessible roles and states or being hidden, is a WCAG violation. I also think that the clutter can be played down in this case because the slider only has four possible positions. But if somebody implemented this same pattern for, say, air conditioner modes, you'd quite possibly have eight or more of these redundant elements.

@jongund
Copy link
Contributor Author

jongund commented Mar 23, 2021

This is ready for review, made the following changes:

  1. Setting the CSS property forced-color-adjust to auto on the SVG elements.
  2. Using stroke-opacity and fill-opacity instead of transparent values for setting stroke and fill colors for the SVG rect used for focus indication for high contrast mode support.

Copy link
Contributor

@jesdaigle jesdaigle 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 ready to merge @mcking65

@jscholes
Copy link
Contributor

jscholes commented Apr 6, 2021

@jnurthen mentioned that the hit area really needs increasing regardless of whether this sample is kept in its current form, replaced, updated, etc.

@jongund
Copy link
Contributor Author

jongund commented Apr 13, 2021

@mcking65
The changes requested at the meeting on 6 April 2021 are in pull #1857, so this pull request is not longer needed.

@jongund jongund closed this Apr 13, 2021
@zcorpan zcorpan deleted the slider-thermostat-update branch October 29, 2021 20:00
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.

None yet