ryersondmp / sa11y

Sa11y is an accessibility quality assurance tool that visually highlights common accessibility and usability issues. Geared towards content authors, Sa11y straightforwardly identifies errors or warnings at the source with a simple tooltip on how to fix them.
https://sa11y.netlify.app
Other
284 stars 44 forks source link

Question about links with an `aria-label` but no text children being reported as errors #70

Closed Scotchester closed 10 months ago

Scotchester commented 10 months ago

Hi!

I'm trying to figure out why a link that only has an SVG icon for a child but does have an aria-label for a text alternative is being reported as an error.

The markup:

<a href="/mission/hubble/science/science-highlights/"
   class="link-external-false"
   aria-label="Hubble Passes 1-Billion Second Mark">
    <svg class="hds-featured-link-list-button margin-left-auto margin-right-0" viewBox="0 0 32 32" fill="none">
        <circle class="color-nasa-red" cx="16" cy="16" r="16"></circle>
        <path d="M8 16.956h12.604l-3.844 4.106 ... 3.844 4.106H8v1.912z" class="color-spacesuit-white"></path>
    </svg>
</a>

And the error I'm getting:

Link does not have discernable text that is visible to screen readers and other assistive technology.

You can also see the issue on the Sa11y demo's advanced links section. Before toggling the "Links (Advanced) [AAA]" switch on, the item labeled "ARIA Label" is already throwing this error, but there is no indication that this item is supposed to be erroring even without the switch on.

'Links that open in a new tab without warning' section of demo with 'ARIA label' item showing an error icon

Shouldn't the aria-label be sufficient? I've not had any other automated tools report this as an issue.

Thanks for any insight!

adamchaboryk commented 10 months ago

Hey @Scotchester,

Can you please confirm the version? That specific link isn't being flagged as an issue on my end. 😅 The latest version of Sa11y is 3.0.6 — the demo page you linked to should reflect that in the header too.

Scotchester commented 10 months ago

Hi @adamchaboryk, thanks for the quick reply!

On my site, I'm just using the bookmarklet. Inspecting the JS that that retrieves, it also looks like 3.0.6.

The demo also appears to correctly be loading 3.0.6.

Knowing that, I had a hunch, and it turns out that I'm only seeing this false positive in Firefox! It correctly reports nothing in Chrome and Safari. I wonder why Firefox behaves differently.

adamchaboryk commented 10 months ago

Firefox 121.0.1 on MacOS appears fine. Although... I am indeed getting this false positive on Firefox ESR. This is troubling.

I'll look into this tomorrow. Thanks for pointing this out - it's greatly appreciated!

I'll let you know once it is fixed.

Scotchester commented 10 months ago

Ah yes, another good callout. We have to use ESR, here (currently as 115.6.0esr). I would be surprised if ESR has an actual functional difference that persists across versions, but I could be wrong on that…. If this is fixed in later versions of Firefox, then I would assume you don't need to do anything else.

adamchaboryk commented 10 months ago

This isn't the first Firefox ESR issue i've encountered. But I've narrowed down the issue!

The ariaLabel property or element.ariaLabel returns undefined in Firefox ESR.

@itmaybejj We'll have to update computeAriaLabel and swap element.ariaLabel with .getAttribute('aria-label')

Or something like this:

  const ariaLabel = element.getAttribute('aria-label');
  if (ariaLabel && ariaLabel.trim().length > 0) {
    return ariaLabel;
  }
  return 'noAria';

I'll also try to file a bug report with FF.

Scotchester commented 10 months ago

Fascinating! And disheartening :(

Scotchester commented 10 months ago

I do note that on the MDN page you reference, it shows Firefox adding support for element.ariaLabel in 119, which the next ESR release will incorporate, and it looks like that will drop around May, if past ESR major release timing holds going forward. Possibly not worth fixing in the interim?

adamchaboryk commented 10 months ago

It's an easy fix and a pretty serious false positive — I wouldn't want to rely on their timelines!! I'll have the issue patched today. Thanks again for the bug report!!

I think my next job is to figure out how to configure my unit testing with headless Firefox ESR!

adamchaboryk commented 10 months ago

Fixed in 3.0.7

Scotchester commented 10 months ago

Thanks so much!