[Slider] Allow mobile VO users to interact with Sliders #23902

CodySchaaf · 2020-12-09T06:51:56Z

I have followed (at least) the PR section of the contributing guide.

Preview: https://deploy-preview-23902--material-ui.netlify.app/components/slider/#vertical-sliders
a11y snapshot changes: https://github.com/eps1lon/mui-scripts-incubator/pull/1468/files

mui-pr-bot · 2020-12-09T06:55:01Z

Details of bundle changes

Generated by 🚫 dangerJS against da05e8d

oliviertassinari

The approach looks solid.

packages/material-ui-unstyled/src/SliderUnstyled/SliderUnstyled.js

packages/material-ui/src/Slider/Slider.test.js

eps1lon

Could you clean up the changes to the test files so that it's obvious what is new and what changed. So far it looks like a pretty big breaking change.

packages/material-ui/src/Slider/Slider.test.js

eps1lon · 2020-12-11T07:48:40Z

packages/material-ui/src/Slider/Slider.test.js

+      stub(container.firstChild, 'getBoundingClientRect').callsFake(() => ({
+        width: 100,
+        left: 0,
+      }));


We also run tests in real browsers where we shouldn't need this. You can look at existing tests how we determine whether we run in JSDOM or not.

This was the approach this file was using originally in order to setup and simulate mouse movements since the code uses const { width, height, bottom, left } = slider.getBoundingClientRect(); to determine what the new value should be in relation to where the users finger is. I borrowed this from the existing code, as a simple way to ensure the width was 100 in order to keep the translation to a value clear (math is a little simpler and easier to spot this way). That being said I can look at updating those old tests or seeing if just working with the width the slider is getting naturally for the new tests.

Makes sense. I'll follow-up after this is merged to make sure we're simulating actual layout. We shouldn't need to simulate it in our browser test suite since those run headful i.e. with layout.

eps1lon · 2020-12-11T07:54:07Z

Also: Could you share how you verified that this approach works? A codesandbox and testing instructions would be ideal. If you don't know how to use this change in a codesandbox you should check out How can I use a change that wasn't released yet?

CodySchaaf · 2020-12-11T18:48:05Z

Could you clean up the changes to the test files so that it's obvious what is new and what changed. So far it looks like a pretty big breaking change.

Hi @eps1lon, Happy to try and clean up the test file a bit. The issue I ran into and discussed with @oliviertassinari (see here #23506 (comment)) and why the test file ended up looking the way it does was because with this new approach the browser now handles all of the keyboard interactions. The browser swallows these keyboard events and updates the value accordingly. So triggering them in test or even through the console in browser has no effect.

The approach we decided on for the test file was to remove the tests that were testing keyboard interactions since that would be re-testing the actual browser, and ended up not being possible because of how the browser handles these interactions. In doing so we discovered that some of those tests were also testing other things, so I broke those out, moved them under their appropriate prop's describe section in the test, and then adjusted the tests to not rely on the keyboard since that no longer works.

Let me know if you have a suggestion on how to revise this. Thanks.

CodySchaaf · 2020-12-11T19:11:24Z

Also: Could you share how you verified that this approach works? A codesandbox and testing instructions would be ideal. If you don't know how to use this change in a codesandbox you should check out How can I use a change that wasn't released yet?

Here is the code sandbox with a very simple example https://codesandbox.io/s/material-ui-issue-forked-q7bh2, I can add a few different variations if you'd like but I tested it out on all of the slider examples present in the docs and it worked really nicely.

Visit the above sandbox on a mobile phone ( I only have an iphone X so that is what I used)
Enable VO in settings (you can enable triple tap to enable which I use to make it easier to enable)

Accessibility -> VoiceOver -> Enable

Travel to the slider by swiping right
once on the slider it will read "Swipe up or down with one finger to adjust the value"
you will see the slider move as you swipe up and down and hear feedback about the current value

I also tested with VO on safari desktop and google desktop and It worked as expected, although those were less broken originally. As well as testing in the docs the more traditional keyboard navigation without VO, testing using the mouse, and testing traditional dragging on mobile using yarn start -H <my-ip>

eps1lon · 2020-12-11T21:28:47Z

It's a bit unfortunate that we have to change the internals now but that was something I should've pushed more during the initial review. Thanks for the work and fixture. I'll test this over the weekend to make sure we're not failing other screen readers.

Once that's done I'll prepare some migration instructions for tests. Last time we changed the public testing approach (on Select) we got quite a bit of issues so I want to make sure we're prepared for that.

packages/material-ui-unstyled/src/SliderUnstyled/SliderUnstyled.test.js

packages/material-ui-unstyled/src/SliderUnstyled/SliderValueLabelUnstyled.js

eps1lon · 2020-12-12T18:51:32Z

Some results from our automated a11y tests:

NVDA still produces the same output on rudimentary keyboard navigation 👍
changes to the produced AOM:
- aria-disabled is now properly picked up. Was probably a bug in Chrome since role=slider does support aria-disabled
- aria-orientation is ignored. Might be a bug in Chrome since role=slider does support aria-orientation and input[type="range"] maps to role="slider"
axe-core (these are old issues flagged under a different rule):
- cannot deal with scientific notation in min and max (https://deploy-preview-23902--material-ui.netlify.app/components/slider/#small-steps) and flags these as invalid attributes
  It probably reads the attributes not the properties since the AOM does convert them to numbers (hello, IEEE 754)

So we should check if the vertical slider is still supported. Could you add one to your fixture (https://codesandbox.io/s/material-ui-issue-forked-q7bh2) and test if VO is recognizing it as a vertical slider? Be sure to also check if it worked prior to this PR.

Update:
NVDA does not support aria-orientation (nvaccess/nvda#6283). I tested our current implementation with JAWS + Firefox and JAWS announced the vertical slider ("up/down slider"). However, the new implementation is announced as "left/right slider". This is less of a problem with the default use because the arrow key behavior ignores orientation. But if you're using a slider and restrict the supported arrow keys to the orientation then the new implementation is misleading.
We should probably add some documentation to the vertical slider regarding keyboard navigation regardless. I would not consider this blocking since, all things being equal, I would prefer mobile VO over JAWS. And JAWS "only" has an announcement problem (can be fixed with verbose labeling) while mobile VO is not operable at all.

CodySchaaf · 2020-12-13T02:59:28Z

Re: slider orientation

It looks like there are a lot of conflicting options some of which are non-standard and some of which are deprecated. So I think that could be an entire pr of its own.

firefox has: orient={orientation}
webkit has: '-webkit-appearance': 'slider-vertical',
and then there is also 'writing-mode': 'bt-lr',

So I think it would be best to hold off if you are comfortable with that. It does seem to still work with VO and changing the value as expected for a non-visual users as is. https://codesandbox.io/s/material-ui-issue-forked-q7bh2

As for

But if you're using a slider and restrict the supported arrow keys to the orientation then the new implementation is misleading.

I'm not even sure you would be able to do that since the those interactions seem to be highjacked by the browser, but I guess anything is possible, so some documentation couldn't hurt.

oliviertassinari · 2020-12-13T11:23:35Z

@CodySchaaf I have rebased on HEAD to fix the conflict with #23974

eps1lon · 2020-12-13T11:30:11Z

It looks like there are a lot of conflicting options some of which are non-standard and some of which are deprecated. So I think that could be an entire pr of its own.

Generally, no. If a PR regresses on certain behaviors then the PR should handle those. In this specific instance I would agree since we want to add some documentation around it and investigate how different browsers/screen readers work.

I'm not even sure you would be able to do that since the those interactions seem to be highjacked by the browser, but I guess anything is possible, so some documentation couldn't hurt.

I would hope that they just work like any other keyboard events in that the change is the default behavior and you can event.preventDefault() it. I don't know if we respect event.defaultPrevented in our key handler so that's definitely something I want to check first. Not that the new implementation blocks a possible feature that we do want to get in.

If you are on a deadline and don't care about vertical orientation then I would recommend you cherry-pick this change into a separate fork. We need to review it a bit more before we can merge it with confidence.

CodySchaaf · 2020-12-13T18:22:54Z

Oh no I didn't mean to try and rush this, I just thought that is what you were suggesting. Happy to keep trying to help.

I was not comfortable with the things I had tried to add to fix this. I ended up with similar results as to what you see here: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/range#Putting_it_all_together

On chrome it ends up reversing the arrow keys so right -> decrease / left -> increase which I don't think is desirable. As well as the console actually shows the property as ~~-webkit-appearance: slider-vertical;~~ with it striked through even though it still clearly has an effect when you toggle it on and off.

Sort of seems like they don't want us to use that since it is non-standard. Let me know if you have any additional ideas or if I should keep investigating. Thanks for all your testing!

eps1lon · 2020-12-13T18:38:55Z

I was not comfortable with the things I had tried to add to fix this. I ended up with similar results as to what you see here: developer.mozilla.org/en-US/docs/Web/HTML/Element/input/range#Putting_it_all_together

They even have a whole section. I didn't see that 👍 . And here I was thinking appearance: slider-vertical was clever 😁

Edit: I filed https://bugs.chromium.org/p/chromium/issues/detail?id=1158217 to get some clarification about aria-orientation on input[type="range"]

oliviertassinari

We need to update the Airbnb demo to forward the children https://deploy-preview-23902--material-ui.netlify.app/components/slider/#customized-sliders and we should be good, I can't find anything else 👌.

eps1lon · 2020-12-28T20:06:54Z

We need to update the Airbnb demo to forward the children deploy-preview-23902--material-ui.netlify.app/components/slider/#customized-sliders and we should be good, I can't find anything else .

I hope this is caused by a different change I ultimately didn't ship because I thought it wasn't necessary. Will take a look tomorrow.

eps1lon · 2020-12-29T10:46:05Z

We need to update the Airbnb demo to forward the children deploy-preview-23902--material-ui.netlify.app/components/slider/#customized-sliders and we should be good, I can't find anything else .

Fixed in 4f9c003 (#23902)

I don't like the new requirement since it's easy to get wrong. It should be impossible to remove the input i.e. effectively remove slider behavior.

I'll follow-up with a different approach with the goal of matching VoiceOver's focus indicator for input[type="range"] while making sure input[type="range"] will always be rendered. Probably going to ask the folks over at react-aria what their conclusion was regarding the focus indicator. I suspect they missed it because the (IMO currently broken indicator) only appears with certain values depending on the orientation.

eps1lon · 2020-12-31T17:25:23Z

Putting on hold because keyboard navigation is reversed for the vertical slider when using horizontal keyboard arrows (ArrowLeft and ArrowRight). See https://deploy-preview-23902--material-ui.netlify.app/components/slider/#vertical-sliders and https://github.com/eps1lon/mui-scripts-incubator/pull/1468/files#diff-e67a117eb1ef378927f4c4122e1409650751a063e7e7ea86cf575fee7ea19d17L206-R232

oliviertassinari · 2020-12-31T17:38:40Z

@eps1lon is this a bug in the implementation of -webkit-appearance: slider-vertical we should report?

eps1lon · 2021-01-01T20:37:39Z

Going to revert the appearance (adjusting the test accordingly) and add to the documentation possible workarounds with different tradeoffs for https://bugs.chromium.org/p/chromium/issues/detail?id=1162640 and https://bugs.chromium.org/p/chromium/issues/detail?id=1158217

eps1lon · 2021-01-02T12:13:32Z

docs/src/pages/components/slider/slider.md

+**WARNING**: Chrome, Safari and newer Edge versions i.e. any browser based on WebKit exposes `<Slider orientation="vertical" />` as horizontal ([chromium issue #1158217](https://bugs.chromium.org/p/chromium/issues/detail?id=1158217)).
+By applying `-webkit-appearance: slider-vertical;` the slider is exposed as vertical.
+
+However, by applying `-webkit-appearance: slider-vertical;` keyboard navigation for horizontal keys (`ArrowLeft`, `ArrowRight`) is reversed ([chromium issue #1162640](https://bugs.chromium.org/p/chromium/issues/detail?id=1162640)).
+Usually, up and right should increase and left and down should decrease the value.
+If you apply `-webkit-appearance` you could prevent keyboard navigation for horizontal arrow keys for a truly vertical slider.
+This might be less confusing to users compared to a change in direction.


@mbrookes Could you take a look at the wording if you got time?

What do you think about moving this new section under ## Accessibility?

I added it to the vertical section on purpose because it's directly related to it. Accessibility is a general section or rather an umbrella section for everything that doesn't fit into other sections.

packages/material-ui-unstyled/src/SliderUnstyled/SliderUnstyled.test.js

docs/src/pages/components/slider/VerticalAccessibleSlider.tsx

oliviertassinari · 2021-01-02T14:37:11Z

docs/src/pages/components/slider/slider.md

+**WARNING**: Chrome, Safari and newer Edge versions i.e. any browser based on WebKit exposes `<Slider orientation="vertical" />` as horizontal ([chromium issue #1158217](https://bugs.chromium.org/p/chromium/issues/detail?id=1158217)).
+By applying `-webkit-appearance: slider-vertical;` the slider is exposed as vertical.
+
+However, by applying `-webkit-appearance: slider-vertical;` keyboard navigation for horizontal keys (`ArrowLeft`, `ArrowRight`) is reversed ([chromium issue #1162640](https://bugs.chromium.org/p/chromium/issues/detail?id=1162640)).
+Usually, up and right should increase and left and down should decrease the value.
+If you apply `-webkit-appearance` you could prevent keyboard navigation for horizontal arrow keys for a truly vertical slider.
+This might be less confusing to users compared to a change in direction.


What do you think about moving this new section under ## Accessibility?

oliviertassinari · 2021-01-02T14:42:00Z

docs/src/pages/components/slider/slider.md

+**WARNING**: Chrome, Safari and newer Edge versions i.e. any browser based on WebKit exposes `<Slider orientation="vertical" />` as horizontal ([chromium issue #1158217](https://bugs.chromium.org/p/chromium/issues/detail?id=1158217)).
+By applying `-webkit-appearance: slider-vertical;` the slider is exposed as vertical.
+
+However, by applying `-webkit-appearance: slider-vertical;` keyboard navigation for horizontal keys (`ArrowLeft`, `ArrowRight`) is reversed ([chromium issue #1162640](https://bugs.chromium.org/p/chromium/issues/detail?id=1162640)).


Suggested change

However, by applying `-webkit-appearance: slider-vertical;` keyboard navigation for horizontal keys (`ArrowLeft`, `ArrowRight`) is reversed ([chromium issue #1162640](https://bugs.chromium.org/p/chromium/issues/detail?id=1162640)).

However, by applying `-webkit-appearance: slider-vertical;` keyboard navigation for horizontal keys (<kbd>ArrowLeft</kbd>, <kbd>ArrowRight</kbd>) is reversed ([chromium issue #1162640](https://bugs.chromium.org/p/chromium/issues/detail?id=1162640)).

With kbd we want the actual keys. ArrowLeft refers to the value of event.key.

Suggested change

However, by applying `-webkit-appearance: slider-vertical;` keyboard navigation for horizontal keys (`ArrowLeft`, `ArrowRight`) is reversed ([chromium issue #1162640](https://bugs.chromium.org/p/chromium/issues/detail?id=1162640)).

However, by applying `-webkit-appearance: slider-vertical;` keyboard navigation for horizontal keys (<kb>←</kbd>, <kb>→</kbd>) is reversed ([chromium issue #1162640](https://bugs.chromium.org/p/chromium/issues/detail?id=1162640)).

Would it work with a space to match what WAI ARIA is doing? I'm not sure that ← is obvious enough

Suggested change

However, by applying `-webkit-appearance: slider-vertical;` keyboard navigation for horizontal keys (`ArrowLeft`, `ArrowRight`) is reversed ([chromium issue #1162640](https://bugs.chromium.org/p/chromium/issues/detail?id=1162640)).

However, by applying `-webkit-appearance: slider-vertical;` keyboard navigation for horizontal keys (<kbd>Arrow Left</kbd>, <kbd>Arrow Right</kbd>) is reversed ([chromium issue #1162640](https://bugs.chromium.org/p/chromium/issues/detail?id=1162640)).

Also note this case https://material-ui.com/components/data-grid/accessibility/#navigation as how we tried to solve this problem in the past.

Would it work with a space to match what WAI ARIA is doing?

That's an actual argument. Why do we also need a review roundtrip for that?

Resolved with 552f608 (#23902)

Why do we also need a review roundtrip for that?

Because I didn't think about this solution/approach in the first place :), your comment has helped create connections.

packages/material-ui-unstyled/src/SliderUnstyled/SliderUnstyled.test.js

Co-authored-by: Olivier Tassinari <[email protected]>

eps1lon · 2021-01-04T11:51:43Z

@CodySchaaf Thanks for the work and explanation 👍 . Really glad we can use native elements and contribute some bugs to browser vendors.

CodySchaaf · 2021-01-06T17:20:52Z

Thanks so much @eps1lon for getting this across the finish line. It was great working with you all!

github-actions bot mentioned this pull request Dec 9, 2020

Update snapshots for pr/23902 eps1lon/mui-scripts-incubator#1468

Closed

oliviertassinari added accessibility a11y component: slider This is the name of the generic UI component, not the React module! labels Dec 9, 2020

oliviertassinari reviewed Dec 9, 2020

View reviewed changes

packages/material-ui-unstyled/src/SliderUnstyled/SliderUnstyled.js Outdated Show resolved Hide resolved

packages/material-ui-unstyled/src/SliderUnstyled/SliderUnstyled.js Outdated Show resolved Hide resolved

packages/material-ui/src/Slider/Slider.test.js Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

vercel bot deployed to Preview December 11, 2020 02:19 View deployment

eps1lon requested changes Dec 11, 2020

View reviewed changes

vercel bot deployed to Preview December 11, 2020 23:12 View deployment

oliviertassinari approved these changes Dec 11, 2020

View reviewed changes

mnajdova reviewed Dec 12, 2020

View reviewed changes

packages/material-ui-unstyled/src/SliderUnstyled/SliderUnstyled.test.js Outdated Show resolved Hide resolved

vercel bot deployed to Preview December 12, 2020 17:21 View deployment

eps1lon mentioned this pull request Dec 12, 2020

[system] Remove visuallyHidden and move to /utils #23974

Merged

eps1lon reviewed Dec 12, 2020

View reviewed changes

packages/material-ui-unstyled/src/SliderUnstyled/SliderValueLabelUnstyled.js Outdated Show resolved Hide resolved

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Dec 13, 2020

oliviertassinari force-pushed the fix/slider/fix-a11y-for-mobile branch 2 times, most recently from 5d5f64c to 09729d5 Compare December 13, 2020 11:20

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Dec 13, 2020

vercel bot deployed to Preview December 13, 2020 11:25 View deployment

eps1lon self-assigned this Dec 13, 2020

fix es module interop with prop-types

8b4c50c

oliviertassinari requested changes Dec 28, 2020

View reviewed changes

eps1lon added 2 commits December 29, 2020 11:11

Merge branch 'next' into fix/slider/fix-a11y-for-mobile

72096c3

Fix airbnb slider

4f9c003

Fix propTypes

5201ab3

eps1lon requested a review from oliviertassinari December 29, 2020 11:23

oliviertassinari approved these changes Dec 29, 2020

View reviewed changes

eps1lon added the on hold There is a blocker, we need to wait label Dec 31, 2020

eps1lon added 2 commits January 2, 2021 10:17

Merge branch 'next' into fix/slider/fix-a11y-for-mobile

6ba9062

Revert webkitAppearance and document everything we know so far

c9daecd

eps1lon reviewed Jan 2, 2021

View reviewed changes

eps1lon requested a review from oliviertassinari January 2, 2021 12:13

oliviertassinari reviewed Jan 2, 2021

View reviewed changes

packages/material-ui-unstyled/src/SliderUnstyled/SliderUnstyled.test.js Show resolved Hide resolved

Apply suggestions from code review

d94e918

Co-authored-by: Olivier Tassinari <[email protected]>

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 3, 2021

Merge branch 'next' into fix/slider/fix-a11y-for-mobile

f691059

eps1lon removed the on hold There is a blocker, we need to wait label Jan 4, 2021

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 4, 2021

eps1lon added 2 commits January 4, 2021 10:50

Use kbd for arrow keys

552f608

use sx

da05e8d

eps1lon merged commit 9499083 into mui:next Jan 4, 2021

	However, by applying `-webkit-appearance: slider-vertical;` keyboard navigation for horizontal keys (`ArrowLeft`, `ArrowRight`) is reversed ([chromium issue #1162640](https://bugs.chromium.org/p/chromium/issues/detail?id=1162640)).
	However, by applying `-webkit-appearance: slider-vertical;` keyboard navigation for horizontal keys (<kbd>ArrowLeft</kbd>, <kbd>ArrowRight</kbd>) is reversed ([chromium issue #1162640](https://bugs.chromium.org/p/chromium/issues/detail?id=1162640)).

[Slider] Allow mobile VO users to interact with Sliders #23902

[Slider] Allow mobile VO users to interact with Sliders #23902

Conversation

CodySchaaf commented Dec 9, 2020 • edited by eps1lon Loading

mui-pr-bot commented Dec 9, 2020 • edited Loading

oliviertassinari left a comment

Choose a reason for hiding this comment

This comment has been minimized.

eps1lon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eps1lon commented Dec 11, 2020

CodySchaaf commented Dec 11, 2020

CodySchaaf commented Dec 11, 2020 • edited Loading

eps1lon commented Dec 11, 2020

eps1lon commented Dec 12, 2020 • edited Loading

CodySchaaf commented Dec 13, 2020 • edited Loading

oliviertassinari commented Dec 13, 2020

eps1lon commented Dec 13, 2020

CodySchaaf commented Dec 13, 2020 • edited Loading

eps1lon commented Dec 13, 2020 • edited Loading

oliviertassinari left a comment • edited Loading

Choose a reason for hiding this comment

eps1lon commented Dec 28, 2020

eps1lon commented Dec 29, 2020

eps1lon commented Dec 31, 2020

oliviertassinari commented Dec 31, 2020 • edited Loading

eps1lon commented Jan 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari Jan 2, 2021 • edited Loading

Choose a reason for hiding this comment

eps1lon Jan 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari Jan 4, 2021 • edited Loading

Choose a reason for hiding this comment

eps1lon commented Jan 4, 2021

CodySchaaf commented Jan 6, 2021

CodySchaaf commented Dec 9, 2020 •

edited by eps1lon

Loading

mui-pr-bot commented Dec 9, 2020 •

edited

Loading

CodySchaaf commented Dec 11, 2020 •

edited

Loading

eps1lon commented Dec 12, 2020 •

edited

Loading

CodySchaaf commented Dec 13, 2020 •

edited

Loading

CodySchaaf commented Dec 13, 2020 •

edited

Loading

eps1lon commented Dec 13, 2020 •

edited

Loading

oliviertassinari left a comment •

edited

Loading

oliviertassinari commented Dec 31, 2020 •

edited

Loading

eps1lon commented Jan 1, 2021 •

edited

Loading

oliviertassinari Jan 2, 2021 •

edited

Loading

eps1lon Jan 4, 2021 •

edited

Loading

oliviertassinari Jan 4, 2021 •

edited

Loading