fix(combobox): update to match ARIA 1.2 criteria

[joshblack]

Closes #6938

Updates the combobox implementation to match the authoring practices reference and includes updates from #6938 as suggested by @carmacleod

Changelog

New

• Add new versions of components for ListBox that more closely align with where we want these components to go, this helps us add the required behavior into combobox without breaking existing components

Changed

• Update ComboBox so that the wrapper node no longer has getRootProps applied to it, these attributes now are on the input
• Convert both the trigger and clear icons to buttons and remove them from tab order (the equivalent functionality already exists through keyboard)

Removed

Testing / Reviewing

• Verify only combobox is impacted by these changes (splash zone includes dropdown, multiselect, filterable multiselect)
• Verify AC has no violations with both closed and open menus
• Verify Combobox can be operated by VO, JAWS, and NVDA
• Verify the following changes have occurred to the markup:
  • Outermost node no longer has combobox role and is now a wrapper div
  • The list box field is no longer interactive and has no additional attributes like ARIA labels
  • The list box input, menu icon, and selection are now siblings instead of descendants of a field which is a button
  • Focus shifts to the input when the menu is open or closed
  • Focus shifts to the input when the input is cleared
  • Both the menu trigger and clear button are not in tab order

[netlify]

Deploy preview for carbon-elements ready!

Built with commit b18204e

https://deploy-preview-7777--carbon-elements.netlify.app

[netlify]

Deploy preview for carbon-components-react ready!

Built with commit b18204e

https://deploy-preview-7777--carbon-components-react.netlify.app

[netlify]

Deploy preview for carbon-elements ready!

Built with commit 9581a5c

https://deploy-preview-7777--carbon-elements.netlify.app

[netlify]

Deploy preview for carbon-components-react ready!

Built without sensitive environment variables with commit 9581a5c

https://deploy-preview-7777--carbon-components-react.netlify.app

[dakahn]

Looks great. Such awesome work. Checked on Windows 10 in Firefox with Accessibility Checker, NVDA, and JAWS latest

The only thing I noticed is that selected options aren't read as 'selected'. WCAG spec allows aria-describedby to reference hidden html elements. So maybe giving the svg the name 'selected' and then setting it to hidden and referencing it's name with aria-describedby on the menu item might be a cool option.

[joshblack]

Issue for the enhancement: #7787

[joshblack]

@laurenmrice just a heads up, with this change the button now receives a focus style and has a click area. It seems like the dimensions of this might need to be updated since it is too narrow, here is a video and an image that I hope will help out with explaining 👀

Screen.Recording.2021-02-11.at.14.45.51.mov

With these changes, what do you think would be the ideal spec for the menu trigger? 👀

[laurenmrice]

@joshblack We should make both focus squares for the close icon and chevron icon 24px. We also do the same thing right now for file uploader.

[joshblack]

Thanks @laurenmrice !

@andreancardona would you want to tackle this or should I? 👀

[mashenka123]

@joshblack This is blocking Code Engine eGA in Mar, could you please finish this bug fix this month? Thank you very much!
https://github.ibm.com/coligo/ui/issues/976
https://github.ibm.com/coligo/ui/issues/940

[mashenka123]

@joshblack This is blocking Satellite eGA on 3/1, could you please fix this asap? Thank you very much! https://github.ibm.com/alchemy-containers/satellite-ui/issues/469

[carmacleod]

Accessibility review

• Please delete aria-owns="downshift-0-menu" from the input (because an input cannot have a listbox child - instead, it controls the listbox (and you already have aria-controls="downshift-0-menu").

• [FIXED IN ANOTHER PR] Please don't use the title attribute on an item unless the item text is too long and is truncated. Chrome and Safari both use title as the menuitem's description, which results in screen readers saying the menuitem name twice. (Firefox wisely prunes out the description when it is identical to the accessible name, so this doesn't happen in Firefox).

• You don't need both <label for> and aria-labelledby to label the input, so go with the HTML labelling method, and delete some excess ARIA. :)

  

• The live region "help/hint text" that says, "6 results are available, use up and down arrow keys to navigate. Press Enter key to select." should just be deleted. Screen reader users usually turn off the help/hint text for their screen reader (as soon as they become proficient with it) in order to reduce verbosity, and since they wouldn't be able to turn this off, it would get annoying pretty quickly. :)

• Please don't use aria-selected="false" on the options. Instead, just remove the aria-selected attribute from options that are not selected. (Some screen readers are saying "unselected", which isn't useful).

Other things I noticed:

• you might want to give the open button outline: none; because it looks a bit unexpected to see the focus outline (very apparent in Chrome/Win) when this element does not take focus.

• the styling for a selected option (light grey background) and the current option (checkmark plus similar light gray background) are confusing when seen side-by-side. To see this, type 2 and then Enter (Option 2 is selected). Now type down arrow. Option 3 currently has focus, but it looks almost the same as Option 2 except for the checkmark. Users may get confused.

[andreancardona]

@carrenelloyd @alisonjoseph Just an update I am working to address all these issues! :)

[andreancardona]

@tay1orjones You were not imagining things - the violation you saw is still there. Maybe you didn't have anything selected the second time you tried it. :)

@andreancardona I'm sorry I wasn't clearer earlier about where to put aria-selected="true" - it should follow focus in a single-select listbox, i.e. it should follow aria-activedescendant in a combobox. If you want to mark up "the item that is currently 'selected' in the combobox" (i.e. the listbox option that has the checkmark), then you can use aria-current="true" (aria-current is not required in a combobox because the input contains the value, but since this UI shows a checkmark it might be a nice way to tie the visual and the semantic together (please do not use aria-checked for this because that indicates something that can be checked/unchecked directly)). Hopefully this screen snapshot is helpful:

Done! :) @carmacleod Thank you so much for catching that!

[oartigue]

In version 11, if I add the aria-label property on Combobox, the property is ignored and the aria-labelledby property is automatically generated. When I use the IBM accessibility Checker, I get this error:
The 'id' "downshift-86-label" specified for the ARIA property 'aria-labelledby' value is not valid for combobox

[mbgower]

@oartigue this was merged 3 years ago. Feel free to cite this PR, but you should open a new issue. it's unlikely to be noticed, otherwise

[oartigue]

In version 11, if I add the aria-label property on Combobox, the property is ignored and the aria-labelledby property is automatically generated. When I use the IBM accessibility Checker, I get this error:
The 'id' "downshift-86-label" specified for the ARIA property 'aria-labelledby' value is not valid for combobox