Accessibility: add aria-label for tooltips
#28849
Conversation
MattyQ
changed the title
|
do you have any more information about situations where |
|
Thanks for catching the aria-describedby bit, Patrick, I was conflating describedby with labelledby. The key issue I'm seeing is with elements that use a tooltip, have a title, but no AT-readable content, such as links that display a font-rendered icon (like using Font Awesome for links to GitHub, Twitter, etc). The universal issue is passive reading of these elements. Because bootstrap removes the content of the title attribute, when these elements are read as part of a larger group (like a parent element), the tooltips aren't triggered, and so the aria-describedby attribute isn't added to those elements. They end up being skipped or just read as an element reference because the title attribute is empty and there are no other labels or content present. I've also run into this when actively giving the elements focus using a screen reader (tested using Orca with Firefox and ChromeVox with Chrome). When the element takes focus, at least in the case of these two screen readers, the state of element is grabbed before the tooltip is triggered, so the aria-describedby attribute is never used. I think the simplest fix would be to leave the title attribute populated so AT still has that reference. My concern there, though, is disrupting implementations of bootstrap where people expect that title attribute to be empty. Otherwise, applying an aria-label to the element seemed to make the most sense as it gives a static reference for AT, rather than relying on the dynamic appearance of the tooltip. However, in the case AT manages to capture both the aria-label and the aria-describedby reference, then that's not really a tenable fix. |
|
@patrickhlauke your call here, the code LGTM 👍 |
|
I'm definitely up for tweaking this to find the best solution, just want to make sure that those elements aren't missed. |
|
not had a chance to live test, but as a quick note: can't leave the for when i get a chance to look at this properly, do you have a live example / test case of the problem itself, @MattyQ ? |
|
Yeah, for sure:
The pattern looks to be that the elements have no other AT-readable content. Adding aria-describedby when the tooltip is triggered does work for elements that have content (such as the tooltip examples in the bootstrap doc). If I pull the content out of those examples, my screen readers go back to reporting the element reference only. Note: I found on the bootstraptor site that when using ChromeVox if I click the icon twice, I picked up the added aria-describedby attribute. I hadn't noticed this on the others, as they're links and there wasn't a chance to click a second time. Interestingly, even if I wait a short while to ensure that the tooltip has loaded on bootstraptor, when I let ChromeVox read it, it still doesn't get the description until the second time I trigger a click. However, when using Orca, I was never able to get the description. |
|
Not sure if one of these approaches might be better? In general, I want to defer to your experience, Patrick.
|
|
Updated the code and tests to not apply aria-label attributes to elements that otherwise have readable content. |
|
sorry for taking so long. will review later today. still feel a bit uneasy about this being an overly-aggressive intervention for something authors should think about, but can also see why it wouldn't even occur to authors that it may be a problem since we do magic swapping around of attributes on the element anyway. might need an extra small note in the docs to warn about this automagic behavior too...will have a think. but yes, conceptually it sounds like a fair approach. wonder if this will catch more complex situations like an SVG with a |
|
No apologies necessary, and I can appreciate the concern. In my testing I found textContent did pick up the title element of an SVG, but it's a good question about other complex situations. I know it's a lot of back and forth to end up not using the code, but I think it makes sense that if this may be too hamfisted (I'd rather not introduce more problems to solve a small one), we could retire this pull request and look at trying to solve why AT is missing the aria-describedby on triggered, text-empty elements. I guess that leaves the issue where elements that are read passively won't return clear content, but that could be handled in the doc with guidance not to rely on tooltips for the only readable content of an element. |
|
Hey @MattyQ, I removed QUnit and switch to Jasmine, so now your unit tests should go there: https://github.com/twbs/bootstrap/blob/master/js/src/tooltip/tooltip.spec.js js/tests/unit/tooltip.js have been removed, so if you need any help do not hesitate 😉 |
|
sorry, it's my fault for not looking at this in a more timely fashion...i'll look at this more thoroughly and change up the jira tests etc |
|
My bad this time for being off the grid -- I'll get those tests updated! |
|
@patrickhlauke how do you want to proceed with this? If we want this to land, we'd need to rebase it and migrate it to the new test framework. Also, I'm unsure if we want this backported. |
|
@patrickhlauke friendly ping :) |
|
@patrickhlauke let me understand this patch first; it only adds an |
|
I pushed a patch but I don't know if the place or the method I used are right. I basically create a tooltip in each test and check for the original element's attributes. I'd wait until @Johann-S or someone else can help us |
| done() | ||
| }) | ||
|
|
||
| tooltip.show() |
There was a problem hiding this comment.
it's not needed to show the tooltip this part of the code is executed after the constructor
| done() | ||
| }) | ||
|
|
||
| tooltip.show() |
| done() | ||
| }) | ||
|
|
||
| tooltip.show() |
|
@Johann-S feel free to make any further changes otherwise let's merge it as is. |
|
after the last requested changes it'll be good to go |
|
Like I said, either make the changes yourself or we land it as is. I don't
want to fight with the tests any further myself :P
…On Mon, Nov 16, 2020, 10:46 Johann-S ***@***.***> wrote:
after the last requested changes it'll be good to go
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#28849 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AACVLNPL7FZI5ZVZGK6OWLLSQDRF5ANCNFSM4HRG2ZRQ>
.
|
Update to the tooltip.js to add an aria-label attribute that contains the original title of the element, but only if the element doesn't have an existing aria-label attribute. This is to address cases where screen readers are not capturing the aria-describedby attribute that is added when the tooltip is triggered. This should also avoid a race condition between the screen reader and the appearance of the tooltip.
before overwriting it...
XhmikosR
dismissed
Johann-S’s stale review
This works; any further tweaks could land later
XhmikosR
changed the title
aria-label for tooltips
Update to tooltip.js and tests to add an aria-label attribute that contains the original title of the element, but only if the element doesn't have an existing, valid aria-label attribute.
This is to address cases where screen readers are not capturing the aria-describedby attribute that is added when the tooltip is triggered. This should also avoid a label not being read because of any race conditions between the screen reader and the appearance of the tooltip.
In the case the aria-describedby attribute is added as intended, it overrides the aria-label, so there should be no collision between the two methods of labeling.The aria-label attribute is added conditionally to avoid conflicting with other ways someone might already be adding aria-label attributes to their elements.