Accessibility Enhancements

Of course this is not a permanent solution, but you could set the variable extlink_label to an empty string, since this module relies on it and will output (link is external) if it is not set.
This can be done by pasting variable_set("extlink_label", ""); to the end of your template.php and removing again, when it was executed (on page reload).

I'd suggest you dig around in the older issues to understand it better. But I am sure that the span was required for something. It may have been older browsers though, so we might be able to get away with it now.

Just for your information, we also found this issue on Google Search Google... so we decided to remove this module from our websites :-(

For accessibility reasons, any information that is available visually (such as the icon that indicates an external link) also has to be available non-visually. For images, that's usually done with alt text. In this case the extlink icon is a background image, and so the text is added with element-invisible.

Ideally this should be done in a way that's independent of any particular technology. Whether a screenreader will inform the visitor that the link destination is external varies by what the screenreader is capable of and how the end-user has configured it. Also you have to take into account other technologies such as screen magnifiers and alternative input devices, and differences between mobile/touch and desktop/keyboard implementations.

It ought to be possible to satisfy the accessibility requirement using an aria-label on the span instead of invisible text. (It doesn't seem like Google indexes aria-labels now, but who knows if they will eventually.)

Here's a patch that uses aria-label instead of invisible text.

Not sure why the admin functionality test is failing, it appears to be a file permissions issue but there are no changes to that test, only to the Javascript that adds the class.

Tests are fixed. Re-roll and we can include it.

The disadvantage to this approach is that when css is broken you won't get text showing that the link is external.

After looking into aria-label more closely, it's not going to work on a span all by itself. Assistive technologies are going to ignore aria-labels on elements that aren't either natively-interactive or tagged with an ARIA widget role. I'll see if there's something that can be added to the link markup instead of the span.

Would the title attribute make sense?

No. The title attribute has very poor accessibility. It's effectively bonus content for sighted mouse users only.

Well if someone has input on what would be a suitable replace I am eager to hear it.

I don't see anything wrong with the original span.element-invisible, and I'm skeptical about this bug report. Is there any SEO research and/or advice from Google, Bing, etc., which suggests that indexing "link is external" is actually detrimental for SEO? Even if the search engines think that external is a keyword for the site, does that matter, so long as you have the intended keywords too?

SEO represents potential visitors, who may find their way to your website one day. But accessibility is about real visitors who have actually arrived at your website. So I'd prioritize accessibility over SEO.

An approach that you could try is using aria-describedby to indicate an external link:

<a href="http://example.com" aria-describedby="random-id-generated-by-js">Interesting news article</a>
<!-- this span is for the icon via CSS -->
<span class="extlink-icon"></span>
<!-- This span is not displayed, but provides text for the aria-describedby reference -->
<span style="display:none;" id="random-id-generated-by-js"> ' + Drupal.settings.extlink.extLabel + '</span>

In theory this means the OS-level accessibility APIs get a link with an AccessibleName of "Interesting news article" and an AccessibleDescription of "link is external". It will be up to individual screen readers whether to announce the description (e.g. macOS VoiceOver speaks the descriptions after a user-configurable delay).

Thank you so much for your input on this. I have no real knowledge of accessibility standards so I am deferring to other people's knowledge on the subject.

"I'd prioritize accessibility over SEO". I agree.

I think your solution would be workable.

Can we not just add the aria-label to the anchor tag?

@elachlan that would override the actual text of the link, not add to it.

@elachlan:

Can we not just add the aria-label to the anchor tag?

No. Using aria-label (or aria-labelledby) will override the link text completely. It's not a supplement to the link text. In the following example, assistive technology would be told that both links have the name "external link". The GitHub/Twitter link text will not be passed to a screen reader at all.

<a href="https://github.com/drush-ops/drush" aria-label="external link">Fork on GitHub</a>
<a href="https://twitter.com/DrushCli" aria-label="external link">Follow us on Twitter</a>

I tried the latest 8.x-1.x branch with various browser/screen reader combinations (i.e. what we have after the commit in comment #14). I wanted to see if @cboyden's concern from #15 was right. I found that in most cases, the link text and external link warning were both announced. This is the markup of a link:

<a href="https://www.drupal.org/project/modules" data-extlink="">modules<span class="ext" aria-label="(link is external)"></span></a>

External link warning announced OK in:

• Win 7 + JAWS 2018 + Firefox 52 ESR
• Win 7 + NVDA + IE 11
• Win 7 + NVDA + Firefox 52 ESR
• Win 7 + NVDA + Chrome 66
• Win 10 + NVDA + IE 11
• Win 10 + NVDA + Firefox 60
• Win 10 + NVDA + Chrome 66
• in 10 + JAWS 2018 + Edge
• Win 10 + JAWS2018 + Firefox 60
• Win 10 + Narrator + Edge
• iOS 11 + VoiceOver + Safari
• Android 7.0 + Talkback + Chrome 66
• macOS 10.13 + VoiceOver + Safari
• macOS 10.13 + VoiceOver + Chrome

Not working in:

• Win10 + NVDA + Edge
• Win10 + Narrator + IE
• Win 7 + JAWS 2018 + IE11
• Win 7 + JAWS 2018 + Chrome 66

Overall, this is an accessibility regression from what we had before the patch in this issue. Some screen readers don't get the warning which they did before, and the concern in #10 about sighted users when CSS is not applied. The best thing to do for accessibility would be to revert the changes in comments #11-14.

Edit: Sorry, I took a long time to write my comment, an you posted patch #23 while I was testing.
I haven't tested patch #23, to be clear, it was the latest 8.x-1.x branch.

The patch basically implemented what you suggested in #19.

Yes, the patch looks like a correct inplementation of #19. What matters though is whether it improves upon the performance in #24. It will be less reliable I think, because assistive tech isn't always required to convey aria-describedby. Screen readers have verbosity settings which affect it. I'll try it out sometime soon.

what if the span is hidden via screen-reader-accessible css (akin to core's element-invisible class) instead of display:none?

$link[icon_placement]('<span class="element-invisible" id="' + $aria_described_by_id + '"> ' + Drupal.settings.extlink.extLabel + '</span>');

RE: #27 - well this issue started with a span.element-invisible, before this issue was filed - this is going around in circles! The text was available to everyone then, now it's only available to some users, with some assisitive tech.

As far as I'm concerned, there's no substance to the SEO claim that started this issue off. Unless there's some actual evidence that the "link is external" actually causes harm for SEO, then I think the changes here should be reverted.

Also, from the issue summary:

If a client has disabilities, his screen reader should make him aware of the link target.

This is (a) wishful thinking, and worse (b) it just blames the user for being disabled. It's not the screen reader's job to do that. The screen reader's job is to convey what's on screen, using information provided by the browser. Besides, the whole point of this module adding a visible icon is because sighted users without assistive tech don't know that the link is external.

@andrewmacpherson, Are you happy with this patch?

What does it do, does it take us back to the start, before #11?

Correct, but also adds a span for when font awesome is used as well.

Some web accessibility testing tools will flag the <i> with a warning. I know it's what FontAwesome suggests, but <span> works effectively with the fa classes. So my preference would be to use <span> instead of <i>.

I changed the i tag to a span.

Looks good to me @elachlan

I just looked at the patch though.

Would be good to have someone post a screenshot of this patch being applied

Thanks everyone for their input on this issue.

I would love everybody's input in #2811293: Accessibility improvements to icons. It might change this again.