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

Updated thermostat slider example (slider-2) #1706

Closed
wants to merge 15 commits into from
Closed

Conversation

jongund
Copy link
Contributor

@jongund jongund commented Dec 26, 2020

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

  • Updated example to use SVG
  • Added high contrast support with the SVG
  • Added mobile support using input[type=number] for changing temperature value
  • Added mobile support using buttons to change the fan/heat settings
  • Updated code to use coding practices

Preview slider thermostat example

Review checklist

@jongund jongund requested a review from carmacleod January 10, 2021 14:52
Copy link
Contributor

@charmarkk charmarkk left a comment

Choose a reason for hiding this comment

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

disregard - mistakenly chose approve

Copy link
Contributor

@charmarkk charmarkk left a comment

Choose a reason for hiding this comment

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

Tested with Windows 10, NVDA 2020.3, JAWS 2020.2012.13; Firefox 84.0.1 and Chrome 87.0.4280.141. All worked as expected as far as documented accessibility functionality, keyboard interaction, and screen readers were concerned.

High contrast was mostly good, but the label on the thumb itself did not show up with HCM Black and Firefox. @carmacleod, could I tag you in for MacOS a11y tests? My test Mac is with another co-worker at the moment.

@jongund
Copy link
Contributor Author

jongund commented Jan 11, 2021

@charmarkk
Thank you for the review and verifying the issue with Firefox and SVG high contrast mode.
I am not sure what to do about Firefox, they seem to have a bug in rendering SVG text element support for the currentColor property. currentColor works on other SVG elements for properties for fill and stroke but not for the text element.

@charmarkk
Copy link
Contributor

@charmarkk
Thank you for the review and verifying the issue with Firefox and SVG high contrast mode.
I am not sure what to do about Firefox, they seem to have a bug in rendering SVG text element support for the currentColor property. currentColor works on other SVG elements for properties for fill and stroke but not for the text element.

Good to know! The colors are bit dark in HCM Black with Edge, but the text does show up.

You're quite welcome! I'll mention this on #1704 and, once we verify all is working well with MacOS I'll update my review.

@jongund jongund changed the title Update slider 2 Update thermostat slider example (slider-2) Jan 13, 2021
@jongund jongund changed the title Update thermostat slider example (slider-2) Updated thermostat slider example (slider-2) Jan 14, 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.

The rects for the rails need to have aria-hidden="true" to hide the rect from screen readers. For example, VO in iOS navigates to the rail rect and says "sky-blue image" - which is confusing.

Degrees
</div>
<svg class="slider-group" width="140" height="340">
<rect class="rail" x="60" y="20" width="6" height="300" rx="5"/>
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
<rect class="rail" x="60" y="20" width="6" height="300" rx="5"/>
<rect class="rail" x="60" y="20" width="6" height="300" rx="5" aria-hidden="true"/>

<div class="slider-thermostat-text">
<div id="id-hc-label" class="label">Heat/Cool</div>
<svg class="slider-group" width="210" height="85">
<rect class="rail" x="25" y="25" width="92" height="6" rx="5"/>
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
<rect class="rail" x="25" y="25" width="92" height="6" rx="5"/>
<rect class="rail" x="25" y="25" width="92" height="6" rx="5" aria-hidden="true"/>

<div class="slider-thermostat-text">
<div id="id-fan-label" class="label">Fan</div>
<svg class="slider-group" width="210" height="85">
<rect class="rail" x="25" y="25" width="132" height="6" rx="5"/>
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
<rect class="rail" x="25" y="25" width="132" height="6" rx="5"/>
<rect class="rail" x="25" y="25" width="132" height="6" rx="5" aria-hidden="true"/>

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.

The reason the label on the thumb is not showing up in the currentColor is that SVG text elements use fill, not color, for drawing text.

examples/slider/css/slider-thermostat.css Outdated Show resolved Hide resolved
examples/slider/css/slider-thermostat.css Outdated Show resolved Hide resolved
@carmacleod
Copy link
Contributor

Regarding SVG and high contrast support:

The dark grey #444444 and the royal blue #005a9c for the thumb both fail 3:1 for non-text contrast in High Contrast Black.
The light grey #cccccc and the light blue #adddff rail colors fail 3:1 non-text contrast in High Contrast White.

I'm not quite sure how to fix this without using @media (forced-colors: active) {}, which is still experimental. It's in the latest Edge, but still behind a flag in Firefox and Chrome. If we use it, then we could specify a couple of system colors for the thumb and rail when high contrast is active.

If we don't use the forced-colors media query, then I think (?) the only color choice we have is currentColor? but that wouldn't help fully because we need more than one color. I could be missing something, though.

@JAWS-test
Copy link

but that wouldn't help fully because we need more than one color. I could be missing something, though.

@carmacleod One color would be enough in my opinion. Alternatively, the graphic can have an outline that has sufficient contrast to the content. Then the graphic would be well visible with any background color

@JAWS-test
Copy link

JAWS-test commented Jan 19, 2021

The slider's contrast ratio of about 1.5:1 in the normal view (without Windows HCM) is clearly below the minimum contrast. This applies to the line in the focused and unfocused state. Only the handle is clearly visible. However, the handle alone is not sufficient to recognize the element as a slider.

slider

@carmacleod
Copy link
Contributor

The slider's contrast ratio of about 1.5:1 in the normal view (without Windows HCM) is clearly below the minimum contrast.

Good point, @JAWS-test - both the light grey #cccccc and the light blue #adddff rail colors fail 3:1 non-text contrast against white. So we need some darker colors for the rail.

One color would be enough in my opinion.

Maybe. But I'm not sure if having the thumb and rail be the same color makes sense (for one thing, they would fail 3:1 non-text contrast against each other).

For example, if we use currentColor for both thumb and rail then both thumb and rail are white in HCM Black, and I don't think it's clear what this component is and what it does:

image

Similarly, in HCM White and even in normal mode if we use currentColor, then both thumb and rail would be black.

I thought it might be clearer to use:

@media (forced-colors: active) {
  .thumb {fill: Highlight}
}

Which gives a cyan blue thumb on a white rail, which looks like this in HCM Dark:

image

Although... argh! The white and cyan still fail 3:1 non-text contrast against each other, because they only have contrast of 1.5:1.

And I haven't even looked at focus states. :(

Colors are hard! 😢

@JAWS-test
Copy link

JAWS-test commented Jan 20, 2021

@carmacleod

One color would be enough in my opinion.

Maybe. But I'm not sure if having the thumb and rail be the same color makes sense

I don't see a problem there. In Firefox, sliders with input type=range are also displayed with only one color in HCM. In Edge and Chrome 2 colors are used (see screenshots). More importantly, I find that the contrasts in the default view and in HCM are sufficient.

Furthermore, I think it makes more sense that in HCM interactive elements are recognizable as such, i.e. for links the set link color is used and for form elements the color for them (in my screenshots: white) and not the color for text (yellow). If the text color is used, it is not recognizable that it is an interactive element. Then it could simply be a display element (without the possibility of operation), e.g. the meter element (see 3rd screenshot).

slider in Firefox:
slider_firefox
slider in Chrome, Edge:
slider_edge
meter in Chrome:
meter_chrome

@JAWS-test
Copy link

JAWS-test commented Jan 20, 2021

@carmacleod

Also interesting is that the Chrome/Edge chose a color for the thumb that passes 3:1 with both black and white, so it contrasts with both the background and the currentColor.

In HCM, the browser does not choose its own colors. It adopts the Windows colors for all elements. The color green in the screenshot is the Windows green for selected text (I think it is the system color Highlight according to https://drafts.csswg.org/css-color-4/#css-system-colors). If I change the color in Windows, the color in the slider changes too (see in the screenshot: there I changed the color to red). So the contrast has nothing to do with the browser, but is a user setting in Windows. The contrast is therefore irrelevant, because the user chooses it her/himself according to her/his needs.

slider

@carmacleod
Copy link
Contributor

carmacleod commented Jan 20, 2021

The color green in the screenshot is the Windows green for selected text.

Ah, ok. But you must have changed your selected color to green, which confused me - I thought it was the browser. 😄
I never changed mine from the default color, so I have cyan blue for Highlight (aka selection background).
Here's a screenshot of a range input in Edge in HCM Black showing the cyan thumb and partially-filled rail.

image

Which is why I thought it might be clearer to use:

@media (forced-colors: active) {
  .thumb {fill: Highlight}
}

Or even just (if we don't care what color the thumb has in "normal" mode):

.thumb {fill: Highlight}

@JAWS-test
Copy link

But you must have changed your selected color to green, which confused me

No, this is the default color in HCM mode number 1 (see screenshot). I have not changed anything. Maybe Microsoft has country specific colors...?

HCM_no1

Or even just (if we don't care what color the thumb has in "normal" mode)

I think highlight is good for the thumb. For the slider line ButtonBorder would be useful, so that it has not the text color but the color of a form element. I think that is also suitable for the normal mode. In the notes it could be explained that other colors can also be used in normal mode, but not in HCM

@jongund
Copy link
Contributor Author

jongund commented Jan 20, 2021

@carmacleod
@JAWS-test
@charmarkk
I made some updates:

  • Added borders to the rails using currentColor for the stroke-color.
  • Using color to set the text color of the value to currentColor
  • Updated documentation
  • Added aria-hidden to the rect elements used for the rails

Reviews on the updates are welcome.

@JAWS-test
Copy link

@jongund
Thank you for the adjustments. The contrast is good in the default view and in HCM. Only on focus the slider has a color that is not adjusted in HCM and therefore might be badly visible with self-selected colors. But this is a minor problem that does not fall under the current testing procedure of ARIA APG regarding HCM.

@JAWS-test
Copy link

JAWS-test commented Jan 21, 2021

However, I have discovered a problem regarding the keyboard operation. When the slider receives the keyboard focus, the slider does not scroll completely into the visible area. Scrolling is also not possible with the arrow keys, as arrow keys are used to operate the slider. Thus, the slider may not be visible to keyboard users.

The problem is more severe in Firefox than in Chrome. In Firefox, the slider may be barely visible. In Chrome, the slider is always visible, but not its values ("Off", "Low", "High" etc.).

Firefox:
firefox

Chrome:
chrome

@JAWS-test
Copy link

JAWS and NVDA (with Chrome) output around the sliders unlabeled graphics. The sliders themselves are also output as graphics. This is because the slider is inside the SVG graphic. It would probably be better to mark the graphic as a layout graphic or to place role=slider outside the SVG and mark the SVG with aria-hidden=true

@carmacleod
Copy link
Contributor

@JAWS-test
(By the way, regarding green vs cyan highlight, I was looking at HCM Black which uses cyan, and you were looking at HCM #1 which uses green here in Canada, too. 😄 I don't know what translation Microsoft uses for HCM Black - there seem to be many possible translations for "black" in German).

Regarding:

JAWS and NVDA (with Chrome) output around the sliders unlabeled graphics. The sliders themselves are also output as graphics.

I don't understand the first sentence - can you explain more?
I do understand the second sentence, because when navigating using virtual/reading cursor, the sliders are output as "slider graphic" or "graphic slider", which probably isn't the best output. Here's NVDA's virtual cursor output for the first 2 sliders (each line is output after down arrow):

Temperature
spin button  editable    68   °F
graphic    slider    68°F
Fan
graphic    slider    Off

Good point re scrolling.

Regarding colors on focus, there is still one contrast problem, which is that #005a9c blue is only 2.9:1 against black, so the temperature thumb's text of 68°F fails 4.5:1 contrast in HCM Black. Maybe the text needs to be in currentColor, too. I'm not sure how strict we want to be about 2.9:1 not quite being 3:1 for non-text contrast, but the closest blue I could find that passes 3:1 against black or white (or the very light yellow in normal mode) is #005c9C so we could easily change to use that instead (it's only 1 character different).

@JAWS-test
Copy link

JAWS-test commented Jan 21, 2021

JAWS and NVDA (with Chrome) output around the sliders unlabeled graphics.

I don't understand the first sentence - can you explain more?

The SVG is recognized as a graphic. Inside the graphic is the slider.
JAWS: When I tab to the slider, the graphic + slider is output. But if I open the list of all graphics with INS+Ctrl+G, for example, an unlabeled graphic is displayed for each slider. When I navigate to the next graphic with G, the SVG graphic is also output. Since it has no label, the text in front of the graphic is output, but not the label of the slider.

@JAWS-test
Copy link

The fact that the slider is in a graphic causes another problem with JAWS (not with NVDA): The current value of the slider is not output by JAWS when reading with the arrow keys or with quick navigation (F).

The problem does not occur if the slider is not inside a graphic (see https://w3c.github.io/aria-practices/examples/slider/slider-2.html)

@JAWS-test
Copy link

An ARIA slider without a button or spin button cannot be operated with AT on a touch device. Therefore, it is very good that the examples contain buttons or a spin button to enable operation on touch devices.

However, I am considering whether it is sufficiently clear with the screen reader that slider and button/spin button are related, i.e. operation of one causes the changing the value of the other. aria-controls is currently used, but is deliberately no longer output by JAWS, for example.

On a mobile device, I first reach the slider, which I cannot operate, and then the buttons. Wouldn't it make sense to have either the buttons in the reading order before the slider, or at the slider a hint about the buttons?

And would it perhaps make sense to mark the selected button with aria-pressed=true?

@carmacleod
Copy link
Contributor

Regarding mobile support, I don't think we should even try to implement a touch-based slider without increment/decrement events. I don't think we're helping anybody by trying to work around the missing API.

I think we need to copy the warning note from the slider pattern to the top of the slider example pages, and add something like: "If you need a slider that works on mobile, use an HTML range input.", i.e. basically what @ZoeBijl suggested in #8 (comment).

Note that authors can style range inputs quite extensively these days, including rotating them into vertical orientation, etc.
Here are a couple of highly styled range input examples that work on iPhone and iPad (I don't have an Android or touch device):

The problem with trying to work around the missing mobile/touch gesture events is that:

  • For a sighted mobile/touch user who is not using a screen reader, these look like sliders, but they don't slide. Only tap works (and not all that well, possibly because the rail is not very wide?).
  • For a mobile screen reader user, @patrickhlauke's assessment of the problem is still true:

    VoiceOver announces (because of the role="slider") that it's "adjustable" and that a user can "swipe up or down with one finger to adjust the value". However, doing this has no effect...

  • On desktop, the number input and the buttons don't take focus, but they can be used with a mouse. This feels very weird to a sighted keyboard user, who would at least expect to be able to tab into that number input.

I don't know of a way to make an aria slider work nicely on touch/mobile, but I haven't developed for those platforms, so maybe I am missing something.

@carmacleod
Copy link
Contributor

@JAWS-test I somehow missed seeing your mobile comment until after I saved mine. :)

If the group decides to try to create a mobile aria slider, I agree that we need to make those relationships clearer somehow.

I think we would also need to split this example into 2: one for desktop/keyboard and one for mobile/touch.

And would it perhaps make sense to mark the selected button with aria-pressed=true?

Yes, or maybe even use radios. :)

@patrickhlauke
Copy link
Member

incidentally, i recently had some downtime over the holidays to revisit some other patterns and their shortcomings on touch. it's ... not a pretty picture overall. some of the fundamental "expose things like they were native elements and intercept/react to the standard keyboard keys to interact with them" approaches simply fall apart on non-keyboard + AT scenarios #8 (comment)

@carmacleod carmacleod added the agenda Include in upcoming Authoring Practices Task Force meeting label Jan 22, 2021
@carmacleod
Copy link
Contributor

@patrickhlauke

it's ... not a pretty picture overall.

ouch.

So, while some patterns kind of work, others are quite fundamentally borked. particularly for the really broken ones, can I suggest that in addition to having the note (like there is now on the slider pattern) in the main document, examples that are broken also have the same warning right there in the example page itself?

agreed. have added "agenda" label to this pr.

@carmacleod
Copy link
Contributor

I don't know of a way to make an aria slider work nicely on touch/mobile, but I haven't developed for those platforms, so maybe I am missing something.

I looked into how component libraries make sliders work on touch/mobile. Apparently they use an HTML range input but hide it visually, and use other elements as visual stand-ins.

Here's an example react-aria slider that works on my iPhone (with and without VO). It uses pointer events for the tapping and dragging, and the hidden range input for focus and screen reader interaction.

I'm not sure whether this type of thing belongs in the APG or not, but I'm leaning towards "not".
I think the most practical thing for the APG at this time is to have a helpful warning note on each slider example, e.g.:

"The necessary touch api isn't available yet, so if you need this to work on touch/mobile, use an HTML range input (even if you hide it visually)."

Interestingly, even though the documentation for the react-aria slider says:

The <input type="range"> HTML element can be used to build a slider, however it is very difficult to style cross browser.

... all browsers I tested the highly styled vertical and horizontal sliders in rendered them correctly. That includes Chrome, Edge, Firefox, Safari, mobile Safari, and even IE11. (The only thing IE11 didn't support was PageUp and PageDown). So I wonder how far back in browser-versions you have to go to have styling difficulties.

@patrickhlauke
Copy link
Member

patrickhlauke commented Jan 25, 2021

yes, for sliders, that's pretty much the conclusion I think many came to a few years ago - either have a hidden <input type="range"> that is the "actual" control and fake the rest on top of it (and then avoid using any ARIA on it, as keyboard and AT users are effectively interacting with the input itself - see https://codepen.io/patrickhlauke/pen/byWPMX and a hackily modified one with extra explicit buttons I did as a proof of concept for some WCAG 2.1 related work https://codepen.io/patrickhlauke/pen/VORGWe) or just go for a custom-styled <input type="range"> to begin with.

an interesting example i saw using the hidden <input> is Google's material design for the web component https://github.com/material-components/material-components-web/tree/master/packages/mdc-slider#sliders (though puzzlingly enough, while the documentation clearly says that the component is backed by actual <input type="range">, the live catalogue page seems not to have them when inspecting the DOM, but this still works with VoiceOver/iOS and Android/TalkBack ... I assume it does something funky with shadow DOM or similar that i've not sussed out yet https://material-components.github.io/material-components-web-catalog/#/component/slider)

@jongund
Copy link
Contributor Author

jongund commented Jan 25, 2021

@patrickhlauke @carmacleod
The main reason to include mobile support in the thermostat example is to to show the limitations of the ARIA slider pattern for supporting mobile devices. If we don't show the additional controls needed to support mobile, designers and developers will assume the slider pattern has mobile support. So while the thermostat example maybe not be pretty, it will at least gives designers and developers an understanding of the issues with using a pure ARIA slider solution. Maybe we should have an additional example using the hidden range control, showing an alternative solution that maybe more robust than a pure ARIA solution for HTML. To me the examples are important to help people understand the strengths and weaknesses of the current ARIA implementation.

@carmacleod
Copy link
Contributor

@jongund

The main reason to include mobile support in the thermostat example is to show the limitations of the ARIA slider pattern for supporting mobile devices. If we don't show the additional controls needed to support mobile, designers and developers will assume the slider pattern has mobile support.

I think they would just think that the example is broken on mobile, because the slider is there but it doesn't work.

We may be able to make the dragging work for sighted touch users by using pointer events, but I'm not 100% sure of that, because I haven't developed for mobile. If we can at least make it work properly for sighted touch users, then we could maybe hide the slider for mobile screen reader users so that it doesn't cause confusion. We could even consider using a hidden range input instead of a visible number input, just to make it more of a "slider example". :)

[Edit: Actually, apparently you can't tell if you're running on a touch device, so never mind hiding the broken slider for screen reader users on touch devices...]

Even if we get it to work a bit more, I still think a warning note may be more useful to designers and developers.
The warning note would be similar to the one @ZoeBijl suggested in #8 (comment). I'll paste that here for convenience:

Warning: this example does not work with touch based assistive technologies such as VoiceOver or TalkBack. The required events aren’t yet supported. Please use a native input with its type set to range.

Zoe used the styling and placement shown below. The warning text is displayed in a red box right after the Example heading on the relevant example page. (Actually, now that we have the CodePen button, it would go after that.) We can work on the words, but I think the fairest thing to authors is an explicit heads-up.

image

@patrickhlauke
Copy link
Member

I mean I'm almost thinking that the whole pattern itself may be in jeopardy. As the line between touch and non-touch devices, with AT, is blurring, you'll find more instances where the official pattern simply isn't fit for purpose? Don't know, maybe I'm being too overzealous here, but perhaps it should be slightly rethought, salvage what can be salvaged (e.g. suggesting complementing an actual <input type="range"> with additional things like aria-valuetext), but strongly suggesting authors should always go with the <input>?

@carmacleod
Copy link
Contributor

@patrickhlauke

the whole pattern itself may be in jeopardy

I don't think we need to quash the pattern, but it is waiting on as-yet-unspecified AOM events to make it viable on mobile. Screen readers need to be able to say, e.g., "swipe up or down with one finger to adjust the value" and then take those gestures and turn them into events that the component can listen for. Current thinking is "increment" and "decrement" for the events.
@cookiecrook or @alice Are the AOM user action events from AT still moving forward?

suggesting complementing an actual <input type="range"> with additional things like aria-valuetext), but strongly suggesting authors should always go with the <input>?

I think we can warn about this in the APG, and create examples for the APG Redesign.

@jongund
Copy link
Contributor Author

jongund commented Jan 26, 2021

I mean I'm almost thinking that the whole pattern itself may be in jeopardy. As the line between touch and non-touch devices, with AT, is blurring, you'll find more instances where the official pattern simply isn't fit for purpose? Don't know, maybe I'm being too overzealous here, but perhaps it should be slightly rethought, salvage what can be salvaged (e.g. suggesting complementing an actual <input type="range"> with additional things like aria-valuetext), but strongly suggesting authors should always go with the <input>?

@patrickhlauke
You may want to make the comment to the ARIA working group. The more general issue is that the ARIA working group does not have API mappings for mobile assistive technologies. The group conformance claims are based only on the APIs they do support.

@jongund
Copy link
Contributor Author

jongund commented Jan 27, 2021

@JAWS-test
@patrickhlauke
I (Jon Gunderson, [email protected]) am setting up a focus group meeting to discuss the updated slider examples. Please let me know if you are interested in participating. The meeting will hopefully be next week. I have setup a Wiki Page on the Slider Issues to help identify the decisions that need to be made).

Jon

@JAWS-test
Copy link

@jongund Thank you very much for the invitation. Unfortunately, since I do not speak English, I cannot attend. I wish good luck with the further development of the example

@jongund jongund removed the agenda Include in upcoming Authoring Practices Task Force meeting label Feb 2, 2021
@jongund
Copy link
Contributor Author

jongund commented Feb 8, 2021

This pull request is no longer needed due to pull #1755.

@jongund jongund closed this Feb 8, 2021
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.

5 participants