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

Make ComboBox accessible #314

Closed
jessegreenberg opened this issue Sep 12, 2017 · 46 comments
Closed

Make ComboBox accessible #314

jessegreenberg opened this issue Sep 12, 2017 · 46 comments
Assignees

Comments

@jessegreenberg
Copy link
Contributor

From phetsims/molarity#37, we would like to make ComboBox accessible with keyboard navigation and description. We found recommendation from W3 for best practices and a working example:

https://www.w3.org/TR/2016/WD-wai-aria-practices-1.1-20160317/#combobox
http://demos.telerik.com/kendo-ui/combobox/keyboard-navigation

but it would be great to have a more solid mockup for the accessible HTML before we start adding to ComboBox. @terracoda is going to work on this and provide an example. Assigning to @terracoda, could you please paste links in this issue and reassign to me when ready?

@terracoda
Copy link
Contributor

terracoda commented Oct 20, 2017

Adding some resources for review:

@terracoda
Copy link
Contributor

My understanding is that work on this issue is happening in phetsims/a11y-research#64, and we are almost there with an accessible combobox.

@terracoda
Copy link
Contributor

@jessegreenberg, I have made further recommendations in phetsims/a11y-research#64 (comment)

and am re-assigning these 2 issues for you to distribute.

@zepumph
Copy link
Member

zepumph commented Oct 23, 2018

From meetings today, @emily-phet asked me to work on ComboBox for Molarity. My priorities align nicely to take a look at this in the next week or so.

@jessegreenberg
Copy link
Contributor Author

@terracoda pointed out in #363 design of the general ComboBox could impact this issue. I am not sure if a grid-like combobox was considered in our initial investigation, or whether a 2D combobox will be a totally different a11y implementation.

@zepumph
Copy link
Member

zepumph commented Oct 26, 2018

Using design from phetsims/a11y-research#64. @emily-phet pointed out this morning that I should just implement the current structure. The first use case will be for Molarity.

@terracoda
Copy link
Contributor

@zepumph, yes please make a general combobox for Molarity. I will investigate those other patterns as soon as possible, but not immediately.

@zepumph
Copy link
Member

zepumph commented Oct 26, 2018

First step was to make ComboBox es6.

zepumph added a commit that referenced this issue Oct 26, 2018
@zepumph
Copy link
Member

zepumph commented Oct 26, 2018

@jessegreenberg pointed me to an old branch that had one implementation of it. I'm going to see how far putting that on master takes me, with the goal of eventually supporting the html in ComboBox.md:

	<div tabindex="-1" 
        id="container-for-labels">
	<span id="listbox-static-label">Solute</span>
	  <button id="listbox-option-dynamic-label" 
          tabindex="0" 
         aria-haspopup="listbox" 
        aria-labelledby="listbox-static-label listbox-option-dynamic-label">Drink Mix</button>
	</div>
	<ul role="listbox" tabindex="0" 
        id="listbox" aria-activedescendant="option-1" 
        aria-labelledby="listbox-static-label" 
       style="list-style:none;">
	  <li role="option" id="option-1" 
          class="selected" 
          aria-selected="true">Drink mix</li>
	  <li role="option" id="option-2">Cobalt (II) nitrate</li>
	  <li role="option" id="option-3">Cobalt Chloride</li>
	  <li role="option" id="option-4">Potassium dichromate</li>
	  <li role="option" id="option-5">Gold (III) chloride</li>
	  <li role="option" id="option-6">Potassium chromate</li>
	  <li role="option" id="option-7">Nickel (II) chloride</li>
	  <li role="option" id="option-8">Copper sulfate</li>
	  <li role="option" id="option-9">Potassium permanganate</li>
	  <li role="option" id="option-10">Potassium dichromate</li>
	</ul>
	<!-- help text for combobox -->
	<p>Change a solute and observe differences.</p>

@zepumph
Copy link
Member

zepumph commented Oct 27, 2018

I made a first pass at this. Basically I converted the usages on the previous branch to master. Tomorrow I will work on reworking the structure to look like what is currently in ComboBox.md.

@terracoda
Copy link
Contributor

@zepumph, if it is helpful there was an example in the a11y-research repo in the html sketches folder as well https://github.com/phetsims/a11y-research/blob/master/html-sketches/combobox/combobox.html.

This might be the same HTML as what @jessegreenberg posted above and what's in the combobox.md file.

There are several open issues about combobox, so I am having a little trouble weaving them together.

@zepumph
Copy link
Member

zepumph commented Oct 27, 2018

Thanks @terracoda. I will use that link to combobox.html to base my current implementation on.

  • For now I am completely ignoring the intersection of mouse and a11y. I think it will be simple enough to support that once we have solidified our direction with the PDOM.
  • In the old implementation there was aria-described by stuff. I removed it, but wanted to make sure that we don't want that at some point.

zepumph added a commit that referenced this issue Oct 27, 2018
@zepumph
Copy link
Member

zepumph commented Oct 27, 2018

After the above commit, everything is implemented except active descendant. I will need to implement it first in phetsims/scenery#873. I'll try to get it done this afternoon, and will check back here afterwards with an update.

zepumph added a commit to phetsims/scenery that referenced this issue Oct 27, 2018
zepumph added a commit that referenced this issue Oct 27, 2018
@zepumph
Copy link
Member

zepumph commented Oct 27, 2018

Added aria-activedescendant support above. I am ready for combobox to be reviewed.

@terracoda, @jessegreenberg, and @emily-phet please have a listen here:
https://phet-dev.colorado.edu/html/molarity/1.5.0-dev.1/phet/molarity_en_phet.html?a11y

I took a brief listen on NVDA chrome and it sounds nice! Note that there aren't other labels on things, so just worry about the sound of the comboBox.

You can also see in the a11y view that the HTML is the same as https://github.com/phetsims/a11y-research/blob/master/html-sketches/combobox/combobox.html.

I still have a few todos in ComboBox.js, but I thought it would be good to get an idea about people's first impressions.

My checklist of still to do's:

  • Multiple todos pointing to this issue.
  • Use of aria-desribedby that I removed? (see Make ComboBox accessible #314 (comment))
  • Implement mouse/a11y interface, partially relating to should Event be delivered to a listener that is added to a node further up the Trail? scenery#58 since there is a workaround in ComboBox for that which complicates things for us.
  • I want review on how activedescendant should be updated. Right now it is updated when focus changes (which feels good to me). It is also being updated when items are selected (so even when the list is hidden, it still has an active descendant. On top of this, I wonder if we should update active descendant for any mouse interaction (probably not), but I was wondering about mouse highlight or something.

Note to @pixelzoom that molarity was only committed on to add the a11yLabel option. I was surprised at how non-invasive it was, though we will likely rename that option at some point.

@terracoda
Copy link
Contributor

@pixelzoom, when you examine the example HTML in the design pattern do you see how this fits into the design pattern of a visual ComboBox?

We have a button that has a dynamic label, so that when focus is placed on the Solute button, a blind user knows what Solute is currently selected before activating the actual Solute button.

Once the user activates the button, a navigable list of solutes is popped up, and one of those solute items has to have focus (the one that was already selected). A user can then navigate the solute items and make their selection, or close the parent popped up list if they do not want to make a new selection. When the list of solutes is closed, the user's focus is returned to the Solute button which confirms which solute is selected.

All of that information needs to be communicated and managed visually and non-visually.

To complicate implementation, we don't even use the ARIA widget role="combobox" because that role is not yet well-suppoted by Assistive Technologies. We have to use a popped-up list instead and this seems to work well to communicate the experience of combobox and to make it accessible to more users at the same time.

Designing for A11y is tricky and evolving. I am happy to see your interest in helping and guiding improving how we do things, so it can be as robust as possible from all perspectives.

@pixelzoom
Copy link
Contributor

Thanks for the overview @terracoda, much appreciated. Again, I don't have any problem with the overall design or philosophy of how ComboBox is instrumented. It's the implementation that is overly complicated and lacking, and it doesn't need to be that way to support the design.

@terracoda
Copy link
Contributor

@pixelzoom, great! I am sure @zepumph is going to fix it up nicely for us!

@pixelzoom
Copy link
Contributor

pixelzoom commented Jan 15, 2019

I'm done with changes to ComboBox see #430. I preserved all TODO items related to this issue; search for "TODO sun#314".

Following that work, I confirmed that a11y still works via the following test:

  • Ran Beer's Law Lab with ?a11y. Confirmed that keyboard navigation of ComboBox works. I could navigate to the button (tab), pop up the listbox (spacebar), move between items in the listbox (arrow keys), and select an item in the listbox (spacebar).

I did not succeed in factoring out ComboBoxListBox, the subject of #445. Highly recommended to do that before further a11y work on ComboBox.

Over to @zepumph.

@jessegreenberg
Copy link
Contributor Author

I addressed this TODO since it wasn't working with disposal in its current state. (Found while reviewing #430)

// TODO: sun#314 is this how you have a reference to a listener to remove?
const handleKeyDown = this.listNode.addInputListener( {

That used to be the way we referenced a11y listeners for removal, but this was changed some time ago. This was probably taken from the branch in #334. Now it looks like other non-a11y related listeners in scenery.

pixelzoom added a commit that referenced this issue Jan 20, 2019
pixelzoom added a commit that referenced this issue Jan 20, 2019
pixelzoom added a commit that referenced this issue Jan 20, 2019
pixelzoom added a commit that referenced this issue Jan 20, 2019
pixelzoom added a commit that referenced this issue Jan 20, 2019
pixelzoom added a commit to phetsims/molarity that referenced this issue Jan 22, 2019
pixelzoom added a commit that referenced this issue Jan 22, 2019
pixelzoom added a commit that referenced this issue Jan 22, 2019
pixelzoom added a commit that referenced this issue Jan 22, 2019
@pixelzoom pixelzoom self-assigned this Jan 22, 2019
@pixelzoom
Copy link
Contributor

@zepumph and I spent many hours today mopping up TODO items related to this issue. We used ComboBox.md as the ground through, but that document should be reviewed and possibly updated, see #465.

Since all TODO comments related to this issue have been addressed or moved to new items.... Closing.

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

No branches or pull requests

6 participants