carbon-design-system / carbon

A design system built by IBM
https://www.carbondesignsystem.com
Apache License 2.0
7.86k stars 1.82k forks source link

[a11y]: Dropdown MultiSelect button should have role="combobox" instead of button #12596

Closed Natuu closed 1 year ago

Natuu commented 2 years ago

Package

carbon-components-react

Browser

Chrome

Operating System

Windows

Package version

7.59.3 (from @console/pal)

React version

17.0.2

Automated testing tool and ruleset

IBM Equal Access Accessibility Checker - latest version

Assistive technology

No response

Description

The buttons inside the MultiSelect causes issue because parent is a button instead of a role="combobox"

WCAG 2.1 Violation

IBM 4.1.2 Name, Role, Value

Reproduction/example

https://v10.carbondesignsystem.com/components/dropdown/code

Steps to reproduce

  1. Choose MultiSelect
  2. Select an item
  3. Close the input
  4. Run accessibility checker

Code of Conduct

tay1orjones commented 1 year ago

I don't think we'd be able to change the role to combobox without significantly altering the behavior and semantics of the MultiSelect. This is why we have Combobox as a separate component.

That said, I can confirm the issue in the v11 storybook.

image

@mbgower I'm not sure how we might approach this one. Do you have any thoughts? At first glance I'm wondering if this might be a false positive.

mbgower commented 1 year ago

I'm pulling @tombrunet in on this. Too technical for me. My hunch is that you need to make the tag a peer of the listbox. If it is at the same level, it shouldn't spit out this error. But that's ill-informed conjecture :) They've been adding a lot of these aria to html rules, and I suspect this is one of the new ones kicking in.

tw15egan commented 1 year ago

@mbgower I've put up a PR (#13173) that makes the tag a peer of the list box and resolves the issue. That is how we were already structuring FilterableMultiSelect, which was not throwing this violation, so it makes sense to mimic that structure.

mbgower commented 1 year ago

@tw15egan I think there are some problems with that approach, on investigation. I just met with Tom and went through these different components, and we have suggestions across the board on the whole 'dropdown' family. Tom is going to summarize some of those here, but I'm also sending you a small video we just made.

tombrunet commented 1 year ago

The high level issue is that most widgets aren't intended to be containers (other than maybe grid). If a widget has content (like button), that content is typically just a shorthand for the aria-label. So the question is, how do you handle these widgets that have multiple mouse actions associated with them?

The net is, the multiple mouse actions are a mouse accommodation for the fact that the mouse is limited on what it can do on the widget - focus or activate mostly. A keyboard doesn't have that limitation, so it doesn't necessarily need all of those other visual buttons to perform actions. For a combobox / dropdown, typically, the escape key is used to clear while on the combobox (no need to tab off to perform the action).

A good reference for this is the APG guide for combobox: https://www.w3.org/WAI/ARIA/apg/patterns/combobox/examples/combobox-autocomplete-both/

Some basic rules of thumb:

tw15egan commented 1 year ago

Okay, that makes sense. I've adjusted the PR for now to include the following:

Just pushed up some changes to the same PR, deploy preview should be updated in a few minutes

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

mbgower commented 1 year ago

All nice changes! The other one I'm seeing, which I texted you about separately, unfortunately, is that the string that gets inserted in the mult-select filter should go away when the user leaves the input

In a combobox, in some implementations, if I type in “o” and then tab away from the input, the “o” persists. To me, that is a little weird but in a true combobox that is an allowed action, since you don’t just choose existing options. You can add new strings that become options. However, in a multiselect (whether filterable or not) that is not a possible thing. You cannot add new values. So IMO when focus leaves the input in filterable multi-select, any filter string typed in should disappear into the ether. That means the Esc key consistently can be used to clear the tag in a multiselect when a user reaches an input with a 2x (or whatever number) tag

https://deploy-preview-13173--carbon-components-react.netlify.app/?path=/story/components-multiselect--filterable

I don't think we want something like this to persist when the focus has moved on... image

Also, pressing "Esc" is not clearing it either once it's already in the input. (And, it also lacks the affordance to tab to the x, which I agree we don't want/need, but when Esc does NOT work, we lose the regular way to invoke the 'delete/clear' function). Note that the user CAN clear the string except by just typing something else (since it's highlight by default on refocus) or backspace. They can also press Delete if the letters are highlighted

tw15egan commented 1 year ago

@mbgower just pushed some updates that should address these issues 👍🏻

mbgower commented 1 year ago

You work fast, my friend :) I think there's_ something not quite working properly there. I can get the list to reappear even when there's... it's retaining the listbox location even after clearing values (when they appear again). Let me just poke at it a bit...

mbgower commented 1 year ago

Say, while doing so, and comparing interaction across all these things I noticed that pressing Enter was NOT opening the filtering multiselect listbox when it had focus. I thought it should, so went and looked at the others just to make sure everything was consistent. While doing so, noticed a couple of other things (sorry!):

  1. On your dropdown, Enter works fine to toggle the list open (and put focus on the item selected) and closed. I noticed that Down Arrow on a closed dropdown is opening but ALSO advancing past the currently selected item. I don't think that should happen. We definitely want the down arrow to open the listbox, but my expectation is that the action is identical to the Enter key: opens listbox and puts focus on the currently selected item. I think the APG select-only comboxbox reflects this pattern pretty accurately. BTW, I think I had an existing ticket on this one.
  2. Your default multiselect is opening on Enter, but not opening on Down Arrow. (the complete opposite of the filerable variant.) Any reason for that? My expectation would be that the Enter would reveal the listbox, as would the Down Arrow. Unfortunately, there's really nothing directly parallel to this in the APG.
  3. Still talking about the multiselect, the behaviour on open has some considerations. Normally I'd expect the list to open with the focus going to the first item (And BTW, the behaviour with multiselect is that where something is already selected it becomes the first item). However, because Enter is also used to select, this gets a little tricky. If the focus goes to something in the list, a user trying to toggle the list closed by pressing Enter could inadvertently either select the first unselected item or unselect the first selected item. So, my inclination is to say that opening a multiselect with Enter should open the list, but not put focus inside the list. The user would have to arrow down to select items. I'd invite discussion on this. IF the multiselect is opened with the Down Arrow, I think I'd expect regular behaviour: open and move focus to first item.
  4. And just to return to the top of this comment, I confirm my expectation that the filtering multiselect should open on Enter
mbgower commented 1 year ago

PS, in the time it took me to start that, have a couple of meetings, and update it and post the comment, I see you've partially addressed my prior comment on interaction curiosities.

Now, I'd like Tom to weigh in on the behaviour I'm now seeing on the filterable mutliselect, which is that if I've entered in a filter string and am arrowing through the filtered items that match, when I press Esc, the list collapses AND the string is cleared (leaving me just with the 2x or whatever label showing how many are selected).

I think that works, and I also think it makes an interesting comparison with the action we got you to do on the combo box (pressing Esc once only collapse, not clears string). @tombrunet would you please play with it and give your two bits?

Obviously for consistency, it would make more sense to just collapse the list, like the combo box. I'm just having troubles imagining why we need that intermediary step in the filterable multiselect. Unlike the combo box, there is no case for the string to stick around as a potential new value. I can add to or alter the string without the need to collapse the list. So to me, if someone collapses this variant, ditching the string almost seems like an intentional feature?