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

[Badge] Improve accessibility #22070

Closed
1 task done
andrewborstein opened this issue Aug 4, 2020 · 14 comments · Fixed by #26009
Closed
1 task done

[Badge] Improve accessibility #22070

andrewborstein opened this issue Aug 4, 2020 · 14 comments · Fixed by #26009
Labels
accessibility a11y component: badge This is the name of the generic UI component, not the React module! docs Improvements or additions to the documentation ready to take Help wanted. Guidance available. There is a high chance the change will be accepted

Comments

@andrewborstein
Copy link

  • I have searched the issues of this repository and believe that this is not a duplicate.

Summary 💡

It would be helpful to have a little more flexible control over both nodes used to compose the <Badge>, so the badge can display the correct UI for sighted users and also render a helpful label for assistive technology.

Examples 🌈

In my app we have a link in the primary nav that wraps badge wrapping an icon, used to indicate the total number of notifications for the current user.

Screen Shot 2020-08-04 at 5 51 30 PM

Demo: https://codesandbox.io/s/material-demo-cc7rw?file=/demo.js

const notificationsCount = 1000

<a href="/notifications">
  <Badge badgeContent={notificationsCount} color="primary">
    <MailIcon titleAccess="notifications" />
  </Badge>
</a>

But with the above markup the link's accessible name — used when screen readers encounter the link, for example — is

notifications 99+

Screen Shot 2020-08-02 at 11 20 31 PM

when it would ideally be

99+ notifications

Screen Shot 2020-08-02 at 11 21 14 PM

One option is to change the DOM order of these nodes and put the {children} after the <span>{displayValue}</span>. If it's safe to assume that this is a logical DOM order for most users, and it doesn't affect the visual layout, then that seems like a reasonable solution.

But I was hoping to have full props access to the <span>{displayValue}</span> node, so I could find another solution like hiding it from assistive technology using aria-hidden and moving the equivalent of displayValue to the accessible icon title. It's a little annoying that we'd have to recreate the logic of displayValue, but I could live with that.

Screen Shot 2020-08-04 at 6 04 41 PM

Demo: https://codesandbox.io/s/material-demo-3ghl7?file=/demo.js

const notificationsCount = 1000
const max = 99;
const displayValue = notificationsCount > max ? `${max}+` : notificationsCount;

<a href="/notifications">
  <Badge
    badgeContent={notificationsCount}
    color="primary"
    displayValueProps={{ "aria-hidden": true }}
  >
    <MailIcon titleAccess={`${displayValue} notifications`} />
  </Badge>
</a>

Unfortunately that's not part of the <Badge> API, so that's not possible. And using the classes prop won't help here, either, since there's no CSS that will hide a node from assistive technology and preserve it visually, only the opposite (e.g. display: none). So the resulting accessible label is

99+ notifications 99+

Screen Shot 2020-08-04 at 6 04 41 PM

The next option I tried was wrapping the badgeContent in an aria-hidden node directly, but that breaks some more default functionality, like passing in my own displayValue and specifiying that it should be hidden when the count is 0. If it were possible for the <Badge> component to read children as the equivalent of innerText and then parse it as an integer, then <span aria-hidden>8</span> could still be parsed as the number 8 instead of a string or a component, etc.

Screen Shot 2020-08-04 at 6 19 10 PM

Demo: https://codesandbox.io/s/material-demo-fxbpy?file=/demo.js

const notificationsCount = 1000
const max = 99;
const displayValue = notificationsCount > max ? `${max}+` : notificationsCount;

<a href="/notifications">
  <Badge
    badgeContent={<span aria-hidden>{displayValue}</span>}
    color="primary"
    invisible={notificationsCount === 0}
  >
    <MailIcon titleAccess={`${displayValue} notifications`} />
  </Badge>
</a>

Finally, it all works! But it seems unnecessarily painful to get there.

I recognize there are other options, still, like

Demo: https://codesandbox.io/s/material-demo-ycehq?file=/demo.js

const notificationsCount = 1000

<a href="/notifications">
  <Badge badgeContent={notificationsCount} color="primary">
    <MailIcon />
  </Badge>
  <span className="visually-hidden">notifications</span>
</a>

But either changing the default DOM order of the two nodes or providing props access to the displayValue both seem like reasonable enhancements, as well.

Motivation 🔦

I'd love to leverage of all the great existing logic and UI built into <Badge>, instead of re-writing big chunks of it to accomodate an accessible label.

@andrewborstein andrewborstein added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Aug 4, 2020
@eps1lon
Copy link
Member

eps1lon commented Aug 5, 2020

Thanks for the detailed issue. Could you summarize what accessible label you would expect for your original markup. Seems like your original approach was

<a href="/notifications">
  <Badge badgeContent={notificationsCount} color="primary">
    <MailIcon titleAccess="notifications" />
  </Badge>
</a>

Just want to make sure we're on the same page about the "normal" markup one would write. In the end we want a solution that is accessible by default.

@eps1lon eps1lon added accessibility a11y component: badge This is the name of the generic UI component, not the React module! and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Aug 5, 2020
@andrewborstein
Copy link
Author

andrewborstein commented Aug 5, 2020

Thanks for the quick response @eps1lon!

Ideally the original markup would return an accessible lable that has parity with the one encountered by sighted users and that makes sense when read aloud. So that would be {badgeContent} {children} instead of the current {children} {badgeContent}, at least for English locale users where that specific order makes sense. For example, it would return:

  • notifications when the count is 0 and the badge is hidden
  • 5 notifications when the count is 5 i.e. between 1 and the max, and the badge is visible and renders the count as-is
  • 99 plus notifications when the count is greater than the max (note the plus instead of + because screen readers don't reliably pronounce punctuation).

It would also be OK if the accessible label were different given the different constraints, as long as the word order was still correct, i.e.

  • 0 notifications
  • 5 notifications
  • 150 notifications

This is a simpler solution, but with less parity (which may also be surprising to developers). If we think it's good for sighted users to hide the badge content by default when the count is 0, then the same should be true for non-sighted users. And the same reasoning would suggest that all users should get the 99+/99 plus label.

@eps1lon
Copy link
Member

eps1lon commented Aug 5, 2020

I don't think it makes sense to bake this into the component since there are too many edge cases that require domain knowledge.

Have you tried using aria-label i.e.

<a aria-label={notificationsLabel(notificationsCount)} href="/notifications">
  <Badge badgeContent={notificationsCount} color="primary">
    <MailIcon titleAccess="notifications" />
  </Badge>
</a>

function notificationsLabel(count) {
  if (count === 0) {
    return 'notifications'
  }
  if (count > 99) {
    return '99 plus notifications'
  }
  return `${count} notifications`; // 'notifications' should be pluralized by locale
}

Especially since the current label looks quite ok to me ("notifications 99+"). Did users of assistive technology tell you that they find that confusing?

@andrewborstein
Copy link
Author

Unfortunately aria-label isn't an option for us since we have an international user base and need the text to be translatable.

The notifications 99+ label is confusing in as much as it doesn't match other patterns used for other accessible labels, like 8 comments and 1 like and 12 activities etc. We haven't user tested this specific control label, but it's unconventional by comparison.

You're totally right that we can solve this other ways, like

Demo: https://codesandbox.io/s/material-demo-ycehq?file=/demo.js

const notificationsCount = 1000

<a href="/notifications">
  <Badge badgeContent={notificationsCount} color="primary">
    <MailIcon />
  </Badge>
  <span className="visually-hidden">notifications</span>
</a>

We'll still have the issue of the label reading 99 notifications instead of 99 plus notifications, though.

So while it's totally reasonable for MUI to do nothing in response to this issue, I think the following enhancements could be helpful to other developers:

  1. Allow any props to be spread on the {displayValue} node. It seems fairly common in other "compound" components to expose full props access to each discrete node, and that would be another way to solve potential challenges developers could face with the component, including mine.
  2. Render + as plus so it's reliably represented to AT. This would require wrapping the + in an aria-hidden node and wrapping plus in a .visually-hidden node. While it's not a everything-is-broken critical issue, I think it's a step closer to "accessible by default".
  3. Reverse the DOM order of the {children} and {displayValue} node. I recognize this is may be seen as an abitrary change, and that it could theoretically break the functionality of code for other developers. But if the order right now is itself arbitrary, given that the UI will appear exactly the same no matter the DOM order, then putting the {displayValue} first could be seen as a benign improvement to the English locale semantics of [count] [things] as opposed to [things] [count].

@eps1lon
Copy link
Member

eps1lon commented Aug 5, 2020

The notifications 99+ label is confusing in as much as it doesn't match other patterns used for other accessible labels, like 8 comments and 1 like and 12 activities etc. We haven't user tested this specific control label, but it's unconventional by comparison.

Comparison with what? Are you sure that the code you compare against is correct?

This would require wrapping the + in an aria-hidden node and wrapping plus in a .visually-hidden node. While it's not a everything-is-broken critical issue, I think it's a step closer to "accessible by default".

I don't think these patterns are generally good. Letting screen readers make these calls is almost always the better choince since they have better domain knowledge. It might be a configuration issue with the screen reader, or just a bug. In the end the first rule of aria should always be considered: don't use aria.

Reverse the DOM order of the {children} and {displayValue} node.

I'm curious why you link a post title why "aria-label is xenophobe" but then immediately suggest that reversing the order is correct (effectively saying that there's a single correct position for the badge content and direction a language is in). Are you sure this would be correct for every language and every position of the badge content?

Unfortunately aria-label isn't an option for us since we have an international user base and need the text to be translatable.

Why is aria-label not translateable?

heydonworks.com/article/aria-label-is-a-xenophobe
adrianroselli.com/2019/11/aria-label-does-not-translate.html

I don't understand how this applies to React generally or my code specifically?

@oliviertassinari
Copy link
Member

@eps1lon Could #22070 (comment) make a great new ## Accessibility section in the documentation? #20600

@andrewborstein
Copy link
Author

Hey @eps1lon I'm sorry if I've come across wrong, or, on the flip side, if I'm misinterpreting your responses. It feels like this conversation has gone astray somehow. I opened the issue with the understanding it wasn't an obvious show-stopper with a clear solution. But I thought I'd put it out there and that if the maintainers saw merit in some of the suggestions, that'd be great! And if not, that's totally fine too.

Originally, the only suggestion I had written was that it'd be helpful (and seemingly on par for other MUI components) to spread props on the displayValue node, so the component had a tiny bit more flexibility for consumers like myself. But then when I saw the "Motivation" section of the issue template, it seemed more useful to explain why I got to that conclusion in the first place — describing the problem, not just one potential solution.

As I said earlier, I will absolutely understand if MUI doesn't think this is worth tackling. It's not important to me to be right, I was just hoping to find opportunities to contribute to MUI. My teammates and I work with the framework everyday and would love to give back more where it's helpful. So I would love start out on the right foot. 🙏

In any case, I'm still happy to answer your questions best as I understand them.


Comparison with what? Are you sure that the code you compare against is correct?

I don't believe there's an objective "correct" here when talking about strings that correspond to localized written language. If you were to ask me, "How many comments are there on this thing?" I would say, "8 comments" as opposed to "comments 8". That's not "correct" per se, just more conventional, at least in English. So I'm reading the accessible badge label from the same lens.

I'm not sure how MUI's approach to localization would affect this, but my assumption is that the DOM order in this component doesn't change in different locales and that the default locale for the app is considered English. If those things are true, it doesn't seem like a stretch to imagine that some particular formulation(s) of English phrases are more conventional than others and thus worth aiming for in MUI's components. But I definitely don't believe there is one "correct" solution here, and if that makes it more challenging to find an agreeable solution that's totally fair.

I don't think these patterns are generally good. Letting screen readers make these calls is almost always the better choince since they have better domain knowledge. It might be a configuration issue with the screen reader, or just a bug. In the end the first rule of aria should always be considered: don't use aria.

I totally agree about the first rule of aria. And I agree that we should be deferring to AT as a rule of thumb about how to treat web content, that they should be considered the domain experts. But I think this particular case could be considered a real world shortcoming of AT that's worth acknowledging and working around. For example, NVDA ignores the plus and equals symbols. It says “five two seven” when it should say “five plus two equals seven.” They have clearly made that choice for some reason, but it may not align with this component's intent. Given that the screen reader is an entirely different medium and it's my assumption that MUI is not building primarily for AT, the screen reader naturally has to make some assumptions on our behalf.

It's my understanding that the + character in the Badge label is intended to convey that there are more than 99 of something, or whatever the max number is. But for NVDA users (the most popular screen reader) it will always read "99" even when the count is greater than "99". It's obviously not intentional and annoying that we even have to think about it, but it's a small way in which we could gain more parity between sighted and non-sighted users. I think letting all users know there are more than n of something seems to match the spirit of the +.

I'm curious why you link a post title why "aria-label is xenophobe" but then immediately suggest that reversing the order is correct (effectively saying that there's a single correct position for the badge content and direction a language is in). Are you sure this would be correct for every language and every position of the badge content?

I’ll try my best to clarify my earlier statements. I'm sorry if it came across this way, but I don't mean to suggest there's a single "correct" position or that changing the DOM order would create a conventionallly semantic label for all users in all locales.

But if (it's a big if) MUI considers English the default locale, then [count] [things] seems more conventional, that more people might understand what it means. MUI doesn't seem to be intentionally taking a stand about the accessible label by using a specific DOM order. But a given DOM order exists nonetheless, which creates accessible label by default. So if you agreed that the MUI default could better match the default locale of English, then maybe that'd be worth pursuing. I recognize, though, that it's subjective and might be safer to not take a stand at all and let individual consumers decide on accessible labels for themselves.

Why is aria-label not translateable?

It's manually translatable just like any string in a codebase, but some of the common automatic machine translation services (e.g. Google Translate) won't reliably translate that attribute. This is one example Adrian gives in his article, where the aria-label of Left-Pointing Magnifying Glass remains untranslated. So if there are more reliable alternatives like .visually-hidden label text, it's nice to use them. Support for automatically translating aria-label has been improving, but it's still not perfect. And I agree with you that the first rule of aria would apply here, too.

I don't understand how this applies to React generally or my code specifically?

It's not a React problem or specifically related to the <Badge> component, just an issue with aria-label in general. I shared the links to explain why your suggestion about using aria-label text is totally valid (and appreciated!) but not applicable in my specific use case. I'd like to be able to support automatic translation services in the app I'm working in.

@oliviertassinari
Copy link
Member

@andrewborstein We appreciate the initiative to report potential improvements in the library, however, if you could keep it short, it would be even better, we have limited bandwidth and covering long message takes time :).

I went with a quick benchmark to look at what approach the most used badges on the internet are using:

  • Facebook, aria-label on the parent <a> element, badge hidden

Capture d’écran 2020-08-08 à 11 46 16

  • YouTube, aria-label on the <button>, badge hidden

Capture d’écran 2020-08-08 à 11 59 25

In this context, I don't think that we should move forward with 1, nor 2, nor 3 but with:

  1. A new ## Accessibility section [material-ui][docs] Add Accessibility section to all component demo pages #20600
  2. Add the demo proposed by @eps1lon [Badge] Improve accessibility #22070 (comment)
  3. Close :)

@oliviertassinari oliviertassinari added docs Improvements or additions to the documentation ready to take Help wanted. Guidance available. There is a high chance the change will be accepted labels Aug 8, 2020
@oliviertassinari oliviertassinari changed the title Badge - potential accessibility enhancements [Badge] Improve accessibility Aug 8, 2020
@andrewborstein
Copy link
Author

Thanks for the feedback @oliviertassinari! That approach sounds totally fair. And I'll definitely keep it shorter in the future 👍

@eps1lon
Copy link
Member

eps1lon commented Aug 27, 2020

So the idea is that you don't handle translation at all but defer to google translate. And that approach does not work with certain naming techniques.

While I understand that concern I don't see this as a viable restriction for us. aria-label is specified, implemented and used by authors. Are there any plans by the ARIA folks to remove this attribute since it's not translateable and if not what reasons did they give? Especially considering aria-description is on track to become standardized as well. Seems like they're doubling down on problematic attributes.

Seems like

Allow any props to be spread on the {displayValue} node.

be the best approach but I would only start with id for now. That way you could do:

<a href="/notifications" aria-labelledby="display-node icon">
  <Badge badgeContent={notificationsCount} color="primary" displayNodeId="display-node">
    <MailIcon titleAccess="notifications" id="icon" />
  </Badge>
</a>

For the + vs plus issue a bit more research would be appreciated like existing issues, behavior in other screen readers etc. Because you can fix this right now manually with <Badge badgeContent={<React.Fragment>99<div aria-hidden>+</div><span className="visually-hidden">plus</span></React.Fragment>} />

@likitarai1
Copy link
Contributor

@oliviertassinari @eps1lon Can I contribute to this issue?

@oliviertassinari
Copy link
Member

@likitarai1 Yes, what solution do you have in mind?

@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 25, 2021

@likitarai1 From what I understand, we had a good path forward with #22070 (comment). A new section on accessibility would even help with #20600.

@likitarai1
Copy link
Contributor

Yeah I was thinking about implementing #22070 (comment).

likitarai1 added a commit to likitarai1/material-ui that referenced this issue Apr 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accessibility a11y component: badge This is the name of the generic UI component, not the React module! docs Improvements or additions to the documentation ready to take Help wanted. Guidance available. There is a high chance the change will be accepted
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants