Fix Combobox aria roles to match aria 1.2 spec.

[wkeese]

The Combobox currently has strange aria roles, especially where the <input> is inside a role=button.

The template is also outdated in the sense that it has the role=combobox on a wrapper node rather than the <input>. As of the Aria 1.2 spec (https://w3c.github.io/aria-practices/#examples-2), the role=combobox is back on the <input>.

So, it should follow the patterns in https://w3c.github.io/aria-practices/#examples-2. Note that the pattern is a bit different depending on whether or not the combobox is filterable.

[carmacleod]

Note that the old ARIA 1.1 pattern does not work well with screen readers. Having some of the ARIA markup on the wrapper element and some on the input is problematic (because usually, ARIA markup goes on the focusable element). The ARIA 1.2 pattern has the combobox semantics on the focusable element (i.e. the input, in this case). So fixing this issue is important because it will make the Carbon combobox work in all screen readers. The way it is now, the Windows screen readers don't know they're in a combobox - they actually think they're in a menu, which is weird (and misleading, because menus don't have a value). Haven't tried VoiceOver yet.

I'm happy to work with you on this. Here is what needs to be done (maybe more - not sure yet. We can test/review after these).

Note that since I just copy/pasted from the example DOM, the id values happen to have "downshift" in them. They also have the word "menu" in them. If possible, please remove the word "menu" from all names and doc for combobox. Please use either "listbox" or "list" instead.

1. The wrapper div no longer needs any ARIA markup at all, so delete the following from div.bx--list-box__wrapper:

aria-labelledby="downshift-0-label" aria-expanded="true/false" aria-owns="downshift-0-menu"

2. Delete tabindex="0" from the input because an HTML input element is in the focus order by default.

3. Make sure the input has the following ARIA markup (see also points 4 and 5, which depend on state):

role="combobox" aria-autocomplete="list" aria-labelledby="downshift-0-label" 
[note: aria-haspopup="listbox" is the default value for aria-haspopup in a combobox so it is not necessary to add it]

4. In addition to point 3, above, the input needs to have the following ARIA markup when it is in the Closed state:

aria-expanded="false"
[note: it is ok to have aria-controls="downshift-0-listbox" if the listbox is in the DOM but hidden]
[note: it is ok to have aria-activedescendant="" when the combobox is closed]

5. In addition to point 3, above, the input needs to have the following ARIA markup when it is in the Open state:

aria-expanded="true" aria-controls="downshift-0-listbox" aria-activedescendant="downshift-0-item-0"

6. I do not know what the purpose of the div.bx--list-box__field is, but it would be good if we can strip out this element completely. If it must be kept for some reason, then at least remove all of the ARIA (i.e. delete role="button" aria-label="close menu" aria-haspopup="true") and get rid of that tabindex. Note also that div does not support the type attribute, so delete the type="button".

7. [Edit: Actually, I guess we had a conversation in an APG group call about this, and it was decided NOT to hide the button in editable comboboxes because it makes the mobile screen reader experience more reliable. In fact, we need to (semantically) make the chrevron a button. So I will be rewriting this point to more closely match this APG editable combobox example.]
   The SVG for the chevron needs to be completely hidden from screen readers (because the aria-expanded="true/false" semantics are already conveyed, so hearing "Close menu image" is unnecessary, and just adds verbosity). So we need to:

   • delete role="img" aria-label="Close menu" from the chevron svg
   • add aria-hidden="true" to the chevron svg
   • it is ok to keep the <title>Close menu</title> for sighted mouse users, but please change it to <title>Close</title> (which is less verbose, and removes the confusing word "menu")
   • same changes can be made for the "Open" chevron svg

8. Regarding the "Clear" button (the "x"), there are a couple of things to fix up:

   • a custom button needs to handle Space as well as Enter to activate the button (because that's what real html buttons do, and what keyboard users expect)
   • when something has both an accessible name (i.e. from aria-label) and a description (i.e. from title attribute) then screen readers will say both (unless the strings are identical). So this button (which has aria-label="Clear Selection" title="Clear selected item") will be announced as "Clear Selection button Clear selected item" which is awfully verbose for a "Clear" button. Please change this to either aria-label="Clear" title="Clear" or simply use title="Clear" because browsers will set the accessible name from the title attribute as a last resort. (Alternatively, "Clear selection" would be ok, as long as the strings are identical if both aria-label and title are used).

[mashenka123]

@wkeese @carmacleod So could you please fix this bug by the end of Feb the latest? This is affecting our product Code Engine, which will be eGA on 3/21.

[carmacleod]

@joshblack Please see @mashenka123's comment, above.

[joshblack]

Hey all! 👋 Sorry for the delay in response, totally agreed with the issues raised. The Combobox (and related listbox components like multiselect) have markup issues due to some mistakes in v9 along with how we are using a related library (downshift).

We'll definitely look into how we can get these addressed in an upcoming sprint. We're also happy to work with anyone who wants to submit a fix if they have a tight timeline 👍

[joshblack]

Just wanted to post a quick update here for anyone interested 👀

We started taking this on during our sprint (started yesterday) and tackled some of the big items mentioned above. Most of the issues we saw noted about came from either downstream behavior from the library we were using or from us incorrectly passing through what was needed. Just to check-off some of the observations above and leave some notes for what's remaining for @andreancardona and I:

TODOs

• Outermost node has incorrect role and way too much ARIA
• "Choose an item" aria-label is on an uninteractive node and should be removed
• Input is wrapped by a button, button should be a sibling
• remove tabIndex="-1" from list box field
• Use <title> for close button but no aria label We opted for just an aria-label and SVG hidden
• Update our clear custom button to an actual button
• Bug where if we click into field it does not open menu
• Bug where if you click on menu icon it does not toggle menu
• Suggestion to try was to remove the clear button from source order to reduce confusion (similar to how the chevron is no longer in tab order)

Enhancements (?)

We noticed these when looking at the WAI ARIA examples:

• Focus should move from menu button to input after activation
• Focus should move from clear button to input after activation

I opened up a draft PR with some of our changes: #7659 they are currently NOT complete and we will be updating it (and completing it) by the end of the sprint 😄

[joshblack]

As an update here, there is a PR raised for this issue here: #7777

Curious if you have any feedback for it if you get a sec @carmacleod 👀

[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