mui / material-ui

Material UI: Comprehensive React component library that implements Google's Material Design. Free forever.
https://mui.com/material-ui/
MIT License
93.88k stars 32.26k forks source link

[RFC] Chip markup #20470

Open eps1lon opened 4 years ago

eps1lon commented 4 years ago

Regarding https://material-ui.com/components/chips/#chip-2

Problem

Chips currently have both a bad a11y and UX story:

Current implementation

This uses terminology from the accessibility tree. A <button> does not necessarily represent a HTMLButtonElement

Proposal

Basic

-<generic tabindex="-1">Basic</generic>
+<generic>Basic</generic>

Clickable

These are fine.

deleteable

-<button>Deleteable <SVGRoot aria-hidden="true" focusable="false">...</SVGRoot></button>
+<generic>Deleteable<button><SVGRoot>...</SVGRoot></button></generic>

deletable + clickable

-<button tabindex="0">Clickable deleteable <SVGRoot aria-hidden="true" focusable="false">...</SVGRoot></button>
+<generic><button tabindex="0">Clickable deleteable</button><button tabindex="0"><SVGRoot aria-hidden="true" focusable="false">...</SVGRoot></button></generic>

Context

17708

Closes #19468

marcosvega91 commented 4 years ago

I'm running into this when I see that delete button is really tricky to test with testing-library. The new implementation seams very interesting 💯

ee0pdt commented 3 years ago

So having spent a sleepless night worrying about accessibility of chips in my app, I think that the issue is in how the Material Design language for Chips has been interpreted here. Things I have noticed:

  1. I can't find examples of a deletable chip being used outside a text area in actual Google apps, so the delete and backspace buttons are readily available to remove the last entered input (shown as a chip)
  2. In Google apps the delete button on a chip is not focused by keyboard - rather in certain situations the whole chip is focused and a screenreader prompt states: "To remove X press the backspace button".
  3. In other scenarios the chip turns into an editable text field (keyboard focus and enter key) allowing the user to change their text input

image

So basically, Chips only have a delete button when they are text inputs, and normally the user wants to have the option to edit or delete the entered input when keyboard focussing on the Chip.

siriwatknp commented 2 years ago

@eps1lon What do you think if we introduce ChipButton specifically for the clickable chips? Because at any point, it is either non-clickable or clickable (it reminds me of the <ListItem button> case)

<Chip>text</Chip> // simple chip, renders `div`
<ChipButton>text</ChipButton> // clickable chip, renders `button`
<ChipButton component={Link}>text</ChipButton> // renders `a`

Also get rid of the deletable prop in favor of start/endDecorator + ChipDelete component (A small component that provides the built-in close icon which developers can change the icon via theming).

<Chip endDecorator={<DeleteIcon />}>text</Chip> // simple chip with icon
<Chip endDecorator={<ChipDelete />}>text</Chip> // simple chip with delete button
<Chip endDecorator={<ChipDelete icon={<Trash />} />}>text</Chip> // simple chip with delete button (custom delete icon)

For clickable + deletable chip, use Chip that contains a button inside:

<Chip endDecorator={<ChipDelete onClick={...} />}><button>Text</button></Chip>

This will match the markup in your comment

CalebJamesStevens commented 1 year ago

Any momentum on this issue? Running into the same problem currently. Looks like the current implementation of the chip probably shouldn't be used outside of a text field. I would be all for adding an end decorator or a seperate component for chips that are to be used outside a text field. The current implementation blocks the accessibility needs of anyone interacting with this component.

siriwatknp commented 1 year ago

@CalebJamesStevens Could you take a look at Joy UI Chip? If it looks good to you, we might update Material UI Chip to follow the similar structure.

A screen reader only gives direction for how to click the button but not to press backspace to delete the chip

We could add this to the ChipDelete component.

CalebJamesStevens commented 1 year ago

@siriwatknp

https://mui.com/joy-ui/react-chip/#clickable

This is perfect, if it could be added to mui material that would be amazing

CalebJamesStevens commented 1 year ago
<Box
  aria-hidden
  className={classes.hiddenTagDescription}
  id={`${tag}-description`}
>
 You are current on a list item with the value of {tag}. To delete the list item press the backspace button.
</Box>
<Chip
  aria-describedby={`${tag}-description`}
>

For those stumbling upon this and waiting for the update this is the accessible solution I had where i made the div insivible but had it describe the chip for a screan reader

CalebJamesStevens commented 1 year ago

@siriwatknp

Was there a plan to have this functionality added? We are being audited by a large national bank and they noted this issue and an accessibility issue

siriwatknp commented 1 year ago

@siriwatknp

Was there a plan to have this functionality added? We are being audited by a large national bank and they noted this issue and an accessibility issue

@DiegoAndai What do you think about the current Joy UI approach? Should we collab on this to bring it to Material UI?

DiegoAndai commented 1 year ago

Hey! Yes, we created an issue for getting closer to Joy's approach: https://github.com/mui/material-ui/issues/39171. We'll try to get as close as possible without significant breaking changes.

I'll add this RFC to that issue.