Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC] Chip markup #20470

Open
eps1lon opened this issue Apr 8, 2020 · 10 comments
Open

[RFC] Chip markup #20470

eps1lon opened this issue Apr 8, 2020 · 10 comments
Labels
accessibility a11y component: chip This is the name of the generic UI component, not the React module! discussion RFC Request For Comments

Comments

@eps1lon
Copy link
Member

eps1lon commented Apr 8, 2020

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

Problem

Chips currently have both a bad a11y and UX story:

  • delete action is inaccessible on iOS devices (and requires knowledge of keyboard shortcuts in any other screen reader)
  • deleteable Chips are announced as buttons
  • clicking the delete icon triggers the same ripple as clicking the Chip itself. The ripple shouldn't bubble to elements that are not activated.
  • a deleteable button is by default selectable but not a basic Chip. Why?
  • demos illustrates a trailing icon that is not used as the delete action. Having it trigger onDelete is misleading (adressed in [Chip] Rename onDelete and deleteIcon #16097)

Current implementation

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

  • Basic: <generic tabindex="-1">Basic</generic>
  • clickable: <button>Clickable</button>
  • deleteable: <button>Deleteable <SVGRoot aria-hidden="true" focusable="false">...</SVGRoot></button>
  • deletable + clickable: <button tabindex="0">Clickable deleteable <SVGRoot aria-hidden="true" focusable="false">...</SVGRoot></button>

Proposal

  • don't nest interactive elements
  • whether the delete action should be in tab order is a hard question. I lean towards having it in tab order and making it easily customizale (for grid-like implementations or Autocomplete)

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

@eps1lon eps1lon added accessibility a11y discussion component: chip This is the name of the generic UI component, not the React module! labels Apr 8, 2020
@eps1lon eps1lon added this to the v5 milestone Apr 8, 2020
@marcosvega91
Copy link
Contributor

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
Copy link
Contributor

ee0pdt commented Mar 24, 2021

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
Copy link
Member

siriwatknp commented Mar 25, 2022

@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

@oliviertassinari oliviertassinari added the RFC Request For Comments label Aug 21, 2022
@CalebJamesStevens
Copy link

CalebJamesStevens commented Nov 17, 2022

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.

  • There is no way to tab to the delete button because there isn't one
  • There's no way to add your own delete button with a custom icon because buttons can not be descendants of buttons which is what the chip role is set as
  • A screen reader only gives direction for how to click the button but not to press backspace to delete the chip

@siriwatknp
Copy link
Member

@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
Copy link

CalebJamesStevens commented Nov 18, 2022

@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
Copy link

CalebJamesStevens commented Nov 18, 2022

<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
Copy link

CalebJamesStevens commented Oct 18, 2023

@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
Copy link
Member

@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
Copy link
Member

DiegoAndai commented Oct 19, 2023

Hey! Yes, we created an issue for getting closer to Joy's approach: #39171. We'll try to get as close as possible without significant breaking changes.

I'll add this RFC to that issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accessibility a11y component: chip This is the name of the generic UI component, not the React module! discussion RFC Request For Comments
Projects
Status: Backlog
Development

No branches or pull requests

7 participants