[badge] Add aria-hidden to badge content and polish docs demos#48471
[badge] Add aria-hidden to badge content and polish docs demos#48471mj12albert wants to merge 6 commits into
aria-hidden to badge content and polish docs demos#48471Conversation
mj12albert
added
accessibility
Deploy previewBundle size
Check out the code infra dashboard for more information about this PR. |
mj12albert
force-pushed
the
fix-badge-a11y
branch
2 times, most recently
from
b7f5157 to
5044297
Compare
| <Badge | ||
| color="secondary" | ||
| badgeContent={0} | ||
| role="img" | ||
| aria-label="no unread messages" | ||
| > |
There was a problem hiding this comment.
If this is not related to aria-hidden, please update the PR title to include fixing docs.
siriwatknp
left a comment
siriwatknp
left a comment
There was a problem hiding this comment.
LGTM. Leave this to @silviuaavram if he wants to split the PR or not.
Minor suggestion to add bullets as summary on the changes in the PR. I was surprise that a lot of demos are changed at first after reading the PR title.
mj12albert
changed the title
aria-hidden to badge contentaria-hidden to badge content and polish docs demos
The demo changes are related since the |
| - **Use badges for supplemental status**: Badges work best for short counts or compact | ||
| states attached to another element, such as unread messages on an inbox button. Don't use | ||
| a badge as the only place where important status appears. | ||
| - **Label the element that owns the badge**: Badge content is hidden from assistive |
There was a problem hiding this comment.
in the examples we are also labelling the badges themselves. maybe we should also mention this.
There was a problem hiding this comment.
I think it would better to make the demos more realistic, in real UIs I think almost all the time badged elements are interactive/clickable (the presence of the badge invites you to touch/click it)
There was a problem hiding this comment.
hmm might be, might be not. for example, a badge might be used to show if user is online. that element is not necessarily interactive. in this case, should we only add aria-label to the interactive examples in the docs?
There was a problem hiding this comment.
The Slack sidebar online indicator example is similar to the new demo with ListItemButton – the badge and avatar aren't interactive, but they're all part of a ListItemButton-like component that should have aria-label describing the name + status indicated by the badge
There was a problem hiding this comment.
Updated the custom styles demo to be similar to this UI: https://deploy-preview-48471--material-ui.netlify.app/material-ui/react-badge/#custom-styles
| export default function BadgeOverlap() { | ||
| return ( | ||
| <Stack spacing={3} direction="row"> | ||
| <Stack spacing={3} direction="row" aria-hidden> |
There was a problem hiding this comment.
why did we go with aria-hidden instead of aria-label attributes on the badges?
There was a problem hiding this comment.
I think the badge itself is just a visual affordance, usually in UIs the badged element is the labelable element like IconButton/ListItemButton in the demos
<IconButton aria-label="show 4 unread messages">
<Badge badgeContent={4}>
<MailIcon />
</Badge>
</IconButton>If the badge itself also needed aria-label, it could cause odd announcements or duplicate content with the badged element's own label
There was a problem hiding this comment.
so, we should decide if we want to add aria-label to the interactive elements only.
in any case, since we are adding aria-hidden to badge content, in the component itself, why add aria-hidden to the stack as well?
There was a problem hiding this comment.
This one is a bit odd because it's an "abstract" demo just to show positioning and not meant to be realistic, it was to avoid reading anything from it but not needed anyway
|
Has this been tested with screen reader? That is all the demos? |
mj12albert
force-pushed
the
fix-badge-a11y
branch
from
5044297 to
bbb8d60
Compare
Yeah all the demos generally involve |
4 participants
Preview: https://deploy-preview-48471--material-ui.netlify.app/material-ui/react-badge/
Add
aria-hiddensince the raw text content (e.g.99+) doesn't indicate any (UI) context and creates noise for SRs, removing it should be an improvement.Added explicit guidance to the docs about providing a descriptive
aria-labele.g. over "99 unread messages" and updated demos accordingly.