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

redesign number spinner interaction #497

Closed
zepumph opened this issue Apr 10, 2019 · 28 comments
Closed

redesign number spinner interaction #497

zepumph opened this issue Apr 10, 2019 · 28 comments

Comments

@zepumph
Copy link
Member

zepumph commented Apr 10, 2019

From phetsims/gravity-force-lab-basics#109

Previous work here was done in
phetsims/gravity-force-lab-basics#62. I'm unsure if this will help

Basically the issue is that on change value text isn't ever communicated to voice over when using a mac. Tested on safari (thanks @KatieWoe) and chrome (thanks @chrisklus).

There was lots of investigation done in phetsims/gravity-force-lab-basics#109 that landed us here as the culprit. When @chrisklus and I went to the scenery-phet demo for NumberPicker and sun demo for NumberSpinner, we found even in the most basic examples that voiceover didn't provide the aria-valuetext. I was very out of the loop when this was originally implemented, and as a result I'm unsure if this has been around for a long time, or is because of work done in phetsims/scenery#951.

Tagging @jessegreenberg so he is aware.

@zepumph zepumph self-assigned this Apr 10, 2019
@jessegreenberg
Copy link
Contributor

Interesting, maybe we should first confirm that aria-valuetext works at all with VO on the markup for AccessibleNumberSpinner

<div role="spinbutton"></div>

@zepumph
Copy link
Member Author

zepumph commented Apr 10, 2019

See phetsims/gravity-force-lab-basics#109 (comment) for a list of items we tested that didn't effect how VO reads out the value text on change.

@zepumph
Copy link
Member Author

zepumph commented Apr 10, 2019

Interesting, maybe we should first confirm that aria-valuetext works at all with VO on the markup for AccessibleNumberSpinner

Yes that is a great idea! It's hard for me to navigate what is custom vs. native browser interaction when looking at AccessibleNumberSpinner

@zepumph
Copy link
Member Author

zepumph commented Apr 15, 2019

changing priority to reflect priority of phetsims/gravity-force-lab-basics#109.

@jessegreenberg
Copy link
Contributor

@zepumph and I took a look at this today on a macOS device with VoiceOver. Here is the current markup for AccessibleNumberSpinner:

<p id="label-377-441-454-631-632-610-609-608-594">‪Mass 1‬</p>
<div id="377-441-454-631-632-610-609-608-594" tabindex="0" aria-valuenow="2000000000" aria-valuetext="2 billion kilograms, half the size of ‪mass 2‬" role="spinbutton" min="1000000000" max="10000000000" aria-labelledby="label-377-441-454-631-632-610-609-608-594"></div>

We discovered that the current markup for the AccessibleNumberSpinner doesn't work with aria-valuetext or aria-valuenow in other contexts outside of PhET either. @KatieWoe helped us identify that aria-valuetext and aria-valuenow do work if the element is an input element rather than a div. We tested with this JSFiddle and examples:
https://jsfiddle.net/facrpj07/1/
https://rawgit.com/w3c/aria-practices/issue125-add-spinbutton-example/examples/spinbutton/spinbutton.html

And got similar results for each.

In phetsims/gravity-force-lab-basics#62 (comment) we listed some problems with using an input element. But since the input is the only element that supports aria-valuetext well we will have to either accept or address them. For example, we discussed using a map to convert 1000000000 -> "1 Billion" so that JAWS and VO don't read out "One zero zero zero ...".

@zepumph
Copy link
Member Author

zepumph commented Apr 15, 2019

Then in the above commits @jessegreenberg and I made a few changes to our number-spinner implementation that will hopefully get things working on a mac:

  • changed from "div" to "input"
  • set input type to "number"
  • set the input value on property change.
  • set aria-valuenow on all AccessibleValueHandler instances
  • made sure that we were preventing default on the number input, and just adding in the listeners that we wanted.

Hopefully this would work on a mac now. The best way to know will be to proceed with phetsims/gravity-force-lab-basics#109 and see if that works. @jessegreenberg what do you think is next for this issue? Perhaps it should passed over to @terracoda to review the html, and maybe talk to some devs about if it is ok to have a number input in phetsims if we are preventing default?

Another thought. Perhaps we don't need to prevent default. Would we ever allow this sort of keyboard input for number spinners?

@zepumph
Copy link
Member Author

zepumph commented Apr 15, 2019

Note that currently the left/right arrow keys don't work, and they actually sound really strange on NVDA in firefox/chrome. Sometimes saying "blank" or sometimes reading the virtual cursor char by char through the aria-valuetext. I'm not sure how to get around this, since it seems to be tied to the AT. If it was the solution, I wonder if we can just not support left/right arrow interaction for this. It isn't in the spec to, and even https://rawgit.com/w3c/aria-practices/issue125-add-spinbutton-example/examples/spinbutton/spinbutton.html doesn't support it, even though the tweaker buttons are on the left and right side of the input.

@zepumph
Copy link
Member Author

zepumph commented Apr 15, 2019

Note in https://www.w3.org/TR/wai-aria-1.1/#spinbutton that it only says we need to support the up and down arrows. @jessegreenberg and I thought that it was maybe alright that it didn't sound good on NVDA using left/right because at least the interaction still works. Perhaps that is enough, because we know the interaction is important, especially when the visual button layout is left and right of the display.

@zepumph
Copy link
Member Author

zepumph commented Apr 16, 2019

I just noticed that although our updated solution above works nicely with NVDA and chrome. It doesn't read any aria valuetext in firefox/NVDA (instead it reads out "6000000000" -> "5000000000"). It seems like on firefox the input's value takes precedent over aria-valuetext. Even when not setting the inputValue, it reads out an empty string instead of the aria-valuetext.
@jessegreenberg could you have a listen on JAWS and report back what you are hearing?

@jessegreenberg
Copy link
Contributor

I just tested with JAWS in Chrome and aria-valuetext is coming through correctly. So aria-valuetext is only supported by Firefox on <div role="spinbutton"> and <input type="range" role="spinbutton"> but not <input type="number" role="spinbutton> or <input type="text" role="spinbutton">.

@zepumph
Copy link
Member Author

zepumph commented Apr 16, 2019

Everything you just said seems to be true in my test. I ran the following code with NVDA on firefox and chrome, and the top two work on both, but the bottom three only say aria-valuetext on chrome:

<body>

<div tabindex="0" role="spinbutton" aria-valuemin="0" aria-valuemax="3" aria-valuenow="0">spinDiv</div>

<hr>
<input type="range" role="spinbutton" aria-valuemin="0" aria-valuemax="3" aria-valuenow="0"/>
<hr>

<input type="number" role="spinbutton" aria-valuemin="0" aria-valuemax="3" aria-valuenow="0"/>
<hr>
<input type=" text" role="spinbutton" aria-valuemin="0" aria-valuemax="3" aria-valuenow="0"/>.

<hr>

<input id="377-441-454-631-632-610-609-608-594" class="a11y-sibling" type="number"
       aria-valuenow="3000000000" role="spinbutton"
       aria-valuemin="1000000000" aria-valuemax="10000000000"
       aria-valuetext="3 billion kilograms"
       aria-label="hello slider">

</body>
<script>
  const children = document.body.children;
  for ( let item of children ) {
    item.addEventListener( 'keydown', () => {
      item.setAttribute( 'aria-valuetext', `new text: ${Math.random().toFixed( 3 )}` );
    } )
  }

</script>

@jessegreenberg
Copy link
Contributor

I just tested the current solution with VO on an iPad Air 2 and I am seeing problems there too. The spinbutton role is not recognized or announced and it does't support input gestures to change the value. I am seeing that in this examxple too: https://rawgit.com/w3c/aria-practices/issue125-add-spinbutton-example/examples/spinbutton/spinbutton.html. That example is sort of of accessible because the user can click the '+' and '-' buttons or double click in the input to bring up the native keyboard for text input, but no role info gets announced.

@terracoda @zepumph can we use a slider for this with aria role of spinbutton?

<input type="range" role="spinbutton" aria-valuetext="2 billion kilograms, half the size of ‪mass 2‬"  max="10" min="1">

I gave it a shot, this sound nice with NVDA in Firefox (including aria-valuetext) and works well with mobile VO.

@terracoda
Copy link
Contributor

@jessegreenberg, I was thinking something similar, but maybe also using aria-roledescription, too, if we want AT to call it something other than a "slider".

We don't really have a typical web spinbutton interaction here anyways as typical spinbutton's have text input to type in a number. We are just changing between a small range of fixed values.

<input type="range" 
aria-roledescription="Mass 1 spinbutton" 
aria-valuetext="2 billion kilograms, half the size of ‪mass 2‬"  
max="10" min="1">

@jessegreenberg, did you check if spinbutton was a valid role for input type="range"? If it is then I think it would be better to try that out first.

The standard keyboard design patterns are very similar for both. I would guess the touch patterns would be similar as well, but I do not know for sure.

@terracoda
Copy link
Contributor

@jessegreenberg, oh, I see you already tested input type="range" role="spinbutton"
Do you want me to test it in on VoiceOver?
If so, please post a link.

@terracoda
Copy link
Contributor

@jessegreenberg, I think spinbutton seems like it would be a valid widget role for the input element if I am reading this the ARIA 1.1. spec correctly here: https://www.w3.org/TR/wai-aria-1.1/#input.

Though I am very fuzzy on what an "abstract role" is.

@terracoda
Copy link
Contributor

Additional comments from my email (April 23rd)

Indeed, the native aria spinbutton role is not meant to work with left/right arrow keys, and PhET does often create number tweekers that visually display buttons with left/right arrow keys.

As has been mentioned in the issue, I think we should explore input type="range" until both HTML5’s input type="number" and ARIA’s role=“spinbutton” are better supported across the board.

I think we are finding ourselves in a bumpy place where AT, Browsers and Standards have not merged into a well-supported space yet. I can’t remember all the things Jesse and I explored initially, but at some point spinbutton looked reasonable. That said there was still the issues about it being read out as "non-editable" which was not desirable.

Since the range/slider interaction works nicely, I think we should explore how we can make this interaction work for smaller ranges that are generally represented by spinbutton interactions.

@jessegreenberg
Copy link
Contributor

did you check if spinbutton was a valid role for input type="range"?

I just tested the following HTML with https://validator.w3.org/ and unfortnately it complains:

"Bad value spinbutton for attribute role on element input.

<!DOCTYPE html>
<html lang="en">

<head>
    <meta charset="UTF-8">
    <title>title</title>
</head>

<body>
    <label for="slider">My Slider</label>
    <input id="slider" type="range" role="spinbutton" aria-valuemax="100" aria-valuemin="0" aria-valuenow="25">
</body>

</html>

But the same HTML is OK with https://achecker.ca/checker/index.php.

If the type is changed to number the error goes away. I can't find info in the spec about which type attributes are/aren't OK to use with role="spinbutton".

What if we just used aria-roledescription like you were suggesting @terracoda,

<input type="range" aria-roledescription="spinbutton">

@zepumph zepumph changed the title AccessibleNumberSpinner aria valuetext not working on mac redesign number spinner interaction Apr 24, 2019
zepumph added a commit that referenced this issue Apr 24, 2019
@zepumph
Copy link
Member Author

zepumph commented Apr 24, 2019

Here is GFLB using input with type range and role spinbutton
https://phet-dev.colorado.edu/html/gravity-force-lab-basics/1.0.0-dev.35/phet/gravity-force-lab-basics_en_phet.html

Here is GFLB using input with type range and aria-roledescription spinbutton
https://phet-dev.colorado.edu/html/gravity-force-lab-basics/1.0.0-dev.36/phet/gravity-force-lab-basics_en_phet.html

@terracoda @jessegreenberg feel free to inspect the a11y-view to get the exact HTML, but I thought it may be nice to try it out in a sim. Here is the PDOM copy from the first version:

<li data-trail-id="394-458-471-648-649-627" id="394-458-471-648-649-627" data-focusable="false" class="a11y-sibling">
  <p
      data-trail-id="394-458-471-648-649-627-626-625-611" id="label-394-458-471-648-649-627-626-625-611"
      data-focusable="false" class="a11y-sibling">&#8234;Mass 1&#8236;</p>
  <input
      data-trail-id="394-458-471-648-649-627-626-625-611" id="394-458-471-648-649-627-626-625-611" data-focusable="true"
      class="a11y-sibling" type="range" aria-valuenow="2000000000" role="spinbutton" min="1000000000" max="10000000000"
      step="1" aria-valuetext="2 billion kilograms, half the size of &#8234;mass 2&#8236;"
      aria-labelledby="label-394-458-471-648-649-627-626-625-611">
</li>

To me with limited NVDA testing they sounded equally good on firefox, and on chrome the role="spinbutton" was not as good when using left/right arrow keys.

@terracoda
Copy link
Contributor

@jessegreenberg and @zepumph, I got no errors with the W3C validator with:

<!DOCTYPE html>
<html lang="en">

<head>
    <meta charset="UTF-8">
    <title>title</title>
</head>

<body>
    <label for="slider">My Slider</label>
    <input id="slider" type="range" aria-roledescription="My Slider described as Spinbutton" aria-valuemax="100" aria-valuemin="0" aria-valuenow="25">
</body>

</html>

And @zepumph and I tested similar code. We got access to left/right arrow keys, and it sounded nice, too.

I think the aria-roledescription solution will be more future proof. If JAWS does not support aria-roledescription now, it will eventually. I don't think it is a problem for blind users of JAWS if the "visual spinbutton" is described as a "slider" as both interactions are range-type interactions and there's no technical mismatch with slider as the default role for these users.

@terracoda
Copy link
Contributor

And as a design note, we can user whichever word we think is best for the control. For example, for Gravity Force Lab Basics, here are some options:

  • Mass 1 spinbutton
  • Mass 1 stepper
  • Mass 1 number spinner
  • Mass 1 number picker
  • Mass 1 mass changer

I think I like "Mass 1 mass changer" and "Mass 1 number spinner" best.

@jessegreenberg
Copy link
Contributor

Thanks for making testing links @zepumph, that is very helpful. I just tested dev.36 with JAWS in Chrome and it sounds great. Works with all arrow keys, I hear all alerts and aria-valuetext, and I hear the aria-roledescription! So JAWS supports the attribute. It also sounds great with mobile VO too, including the roledescription.

@terracoda @zepumph I agree that seems like the way to go.

In choosing a name for the role, I like "Number Spinner" best as it is most general and will work for all PhET NumberSpinners in the future.

@terracoda
Copy link
Contributor

terracoda commented Apr 27, 2019

@jessegreenberg, that's wonderful news about JAWS and mobile VoiceOver! I was a little worried that JAWS didn't yet support aria-roledescription.

Also, I like number spinner, too, as a general name. We can go with that, but keep in mind with aria-role description we have to include both the name and the function of the control in the attribute's value. We cannot just provide the function.

@zepumph
Copy link
Member Author

zepumph commented Apr 29, 2019

Yay! I'm glad to hear that the roledescription solution is sounding well everywhere. I will put that to master and we can tweak from here.

we have to include both the name and the function of the control in the attribute's value

@terracoda perhaps we should keep "number spinner" and the rest should be solved by changing the Accessible Name of the component, instead of "Mass1" maybe "Mass 1 changer" or "Change Mass 1".

@zepumph zepumph removed their assignment Apr 29, 2019
@terracoda
Copy link
Contributor

@jessegreenberg and @zepumph, I am not sure I was clear about name and function for the aria-roledescription attribute. What I mean is that our A11y API will need to be able to fill in the accessible name automatically or allow the developer to fill it. The element's name might always have to be included directly in the aria-roledescription attribute together with the general function name of "number spinner" for both to be read out.

Due to ARIA naming algorithms, I am not sure if the accessible name provided by a label element will be overwritten by the presence of an aria-roledescription attribute on an input element with which thelabel element is associated.

For example, this first code block may be required over the second code block:

<label for="mass1-control">Mass 1</label>
<input id="mass1-control" type="range" aria-roledescription="Mass 1, number spinner">
<label for="mass1-control">Mass 1</label>
<input id="mass1-control" type="range" aria-roledescription="number spinner">

I am not 100% sure, but I think that aria-roledescription might over-write the accessible name. It did for the Chemistry book in Friction, but we were not using a label in that case.

We will need to test to be sure that the accessible name is being provided and that the API makes providing the name nice and easy whenaria-roledescription is needed.

@zepumph
Copy link
Member Author

zepumph commented Apr 30, 2019

@terracoda I think we may be getting different results depending on screen reader. I have never heard the aria-roledescription override the accessible name for a component. In all browsers (chrome, firefox, ie, and edge) I heard the same thing for the above examples. For the first, on focus it said:

"Mass 1 Mass 1, number spinner 50"

and the second said:

"Mass 1 number spinner 50"

In master we currently have "option 2" implemented like so:
this.setAccessibleAttribute( 'aria-roledescription', 'number spinner' );

It did for the Chemistry book in Friction, but we were not using a label in that case.

I don't remember that case, though I am learning all the time (everyday in fact) and perhaps just forgot.
@terracoda could you please see if "option 2" above or GFLB master omits the accessible name while using VoiceOver?

So JAWS supports the attribute.

@jessegreenberg I assume that here you meant that JAWS spoke the role description ("number spinner") AND said the accessible name, such that to support JAWS we don't need to add the accessible name into the role description value. Please correct me if I'm wrong.

If we can't currently reproduce the case where accessible name is overridden by role description, than I think we should move forward with this solution, and can revisit if problems come up.

@zepumph
Copy link
Member Author

zepumph commented May 1, 2019

@terracoda and I confirmed tonight that we do hear the accessible name, so we don't need to provide it in the aria-roledescription.

@jessegreenberg over to you to confirm all looks good in JAWS before we close.

@jessegreenberg
Copy link
Contributor

JAWS spoke the role description ("number spinner") AND said the accessible name, such that to support JAWS we don't need to add the accessible name into the role description value. Please correct me if I'm wrong.

@terracoda @zepumph that is correct for JAWS as well.

@zepumph
Copy link
Member Author

zepumph commented May 2, 2019

Excellent! I'm going to close, and we will continue as we go through testing for GFLB.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants