[Chip] Document accessibility

[eps1lon]

Documents and tests a11y of different chip variants.
https://deploy-preview-18271--material-ui.netlify.com/components/chips/#accessibility

Fixes:

• not-clickable chips being focusable (by clicking) and having a style for this state Re-evalute chips for v5. We don't hide signifiers for buttons without onClick either and the design system has no notion of "not-clickable" either.
• deleteable but not-clickable chips not being accessible they are since the chip itself is always a button
• closes Making chips more accessible and testable #17708

[mui-pr-bot]

No bundle size changes comparing a83a7b3...29faf65

Generated by 🚫 dangerJS against 29faf65

[oliviertassinari]

Something is not right with the tab on the autocomplete. Compare

1. https://material-ui.com/components/autocomplete/#multiple-values
2. https://deploy-preview-18271--material-ui.netlify.com/components/autocomplete/#multiple-values

Performing the following in 2. yields to two problems, there is no visual tip where the focus is, the alt-tab is stuck on the last input (the chip delete button receives the focus):

done with 1.

[eps1lon]

The chip isn't clickable and therefore not focusable. This is intended. If it's deleteable the delete icon should be focusable. If it's neither clickable nor deleteable then the chip demo has a fundamental issue.

I can fix the issues with the chip demo. Probably best served by writing tests for the Autocomplete first. (This is why we write them)

[eps1lon]

Ok this is an interesting use case that we can enhance.

Currently keyboard users can't delete a specific value. The easiest solution would be to put every single chip in tab order. This does not scale very well with the number of tags. We could implement something like a ChipGroup that handles keyboard navigation with arrow keys. The group could be focusable by tabbing (just put it in tab order) or by another hotkey.

I solved it for now by assuming that <Chip tabIndex /> puts the tabIndex on the element that would usually be in tab order (the chip for clickable or the delete icon for deletable but not clickable).

[oliviertassinari]

@eps1lon It looks like this thread uncovers a couple of different but interdependent concerns. Great! :) I will try to give feedback on each of them.

But first, a benchmark, here are what I could find (something we can use to look at how people did handle or not the problem in the paste), I did collect this list before answering, and from my exisiting sources. I hope it's unbiased:

• https://ej2.syncfusion.com/react/demos/#/material/chips/default
• https://ej2.syncfusion.com/react/demos/#/material/multi-select/default
• https://garden.zendesk.com/react-components/dropdowns/#multiselect-usage
• https://garden.zendesk.com/react-components/tags/
• https://blueprintjs.com/docs/#core/components/tag
• https://blueprintjs.com/docs/#select/multi-select
• https://baseweb.design/components/tag/
• https://polaris.shopify.com/components/forms/tag
• https://atlaskit.atlassian.com/packages/core/tag
• https://atlaskit.atlassian.com/packages/core/select

Keyboard focus visible

I think that we need a way to see that the delete action is keyboard focused. I believe it's not handled yet.

Keyboard users can't delete a specific value

If you refer to the autocomplete, yes, it's no longer possible to use the Left, Right, and Backspace keys to delete a specific value on
https://deploy-preview-18271--material-ui.netlify.com/components/autocomplete/#multiple-values.

But it's possible on v4.6.1.

I solved it for now by assuming that puts the tabIndex on the element that would usually be in tab order (the chip for clickable or the delete icon for deletable but not clickable).

In the case of the Autocomplete, it doesn't work because the Autocomplete tries to call focus() on the DOM nodes that have the data-tag-index attribute. With the tab index on the child delete button, it can't focus it. I would suggest that we always apply the tabIndex on the root element, for consistency.

If it's deleteable the delete icon should be focusable

From the benchmark list, this behavior seems to depend on the implementations. I don't have a specific preference, I think that we could accept both.

---

I think that it would be great to include @mbrookes in the discussion as he has some context on the chip component.

On a side note, I really like @nareshbhatia's MultiSelectChipGroup group concept in https://nareshbhatia.github.io/react-force/?path=/story/multiselectchipcontrol--example.
It makes me think of https://vuetifyjs.com/en/components/chip-groups#chip-groups.

[oliviertassinari]

Probably best served by writing tests for the Autocomplete first. (This is why we write them)

Agree, I think that the multi select autocomplete would benefit from test on the tag keyboard feature. Actually, during the implementation, I have started with a virtual focus approach to later change it for a dom focus approach.

[mbrookes]

IMHO it should work as it currently does in 4.6.x - backspace deletes the prior chip, left arrow selects the prior chip and backspace deletes it. Interestingly, the react-select example still works as expected.

spacing can be very disruptive

In what way?

[eps1lon]

sigh back to writing tests for autocomplete. Wasting too much time otherwise.

[oliviertassinari]

@eps1lon OK, I will write test cases for the tag multi select behavior. The timing is great. I hope it will help you and future iterations.

[oliviertassinari]

Another possible approach I haven't thought of, from Gmail:

[eps1lon]

Another possible approach I haven't thought of, from Gmail:

Yes putting everything in tab order is always a solution. Though it makes navigation pretty annoying since you are reduced to a single button on the keyboard. That's the reason navigation within a widget is usually done with other keys. The same would apply here.

There is still the fundamental issue that chips that aren't clickable are considered buttons.

But if we do follow through with this PR we still haven't answered the accessible name story (which is pretty hard without useUniqueId anyway) for the delete icon.

So I guess I remove all the implementation changes and leave the testing story as is in this PR. Just keep in mind that the a11y story of Chips is problematic at the moment.

[oliviertassinari]

Just keep in mind that the a11y story of Chips is problematic at the moment.

Definitely, having to do the following isn't a good sign:

https://github.com/mui-org/material-ui/blob/a54729e6412067f049b93546077fdae12c15782e/packages/material-ui-lab/src/Autocomplete/Autocomplete.test.js#L67

[oliviertassinari]

Another tradeoff: https://elastic.github.io/eui/#/display/badge.

Regarding how we can move forward. It seems that this is a challenging problem to solve.
In #17708, it seems that the delete icon is not a concern. So maybe we should leave the delete icon aside, move to a problem that could have a better return on time investment?