Problem/Motivation

There are 2 issues with the Font Awesome option:

1. CSS: dot missing, was probably meant to be .extlink i
2. Markup: Accessibility issue.
An aria-label on <i> is not valid with AXE for example: https://jsfiddle.net/8wvby7pt/.
Same on <span> it seems: https://jsfiddle.net/8wvby7pt/1/
PS: It seems this was changed from span-i to span-span in #2467383-35: Accessibility Enhancements.

Proposed resolution

1. Update the CSS to .extlink i
2. And the markup to use FA's sr-only class + use aria-hidden="true" on the icon: 

<span class="fa-ext extlink">
  <i class="fa fa-external-link" aria-hidden="true"></i>
  <span class="sr-only">(link is external)</span>
</span>

https://jsfiddle.net/s23w8q4j/

CommentFileSizeAuthor
#9 fontawesome_fixes-3378557-9.patch2.31 KBherved
#7 fontawesome_fixes-3378557-7.patch1.54 KBherved
#4 fontawesome_css_fix-3378557-4.patch276 bytesherved

Issue fork extlink-3378557

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

herved created an issue. See original summary.

herved’s picture

herved commented
Issue summary: View changes
herved’s picture

herved commented
Issue summary: View changes
herved’s picture

herved commented
StatusFileSize
new fontawesome_css_fix-3378557-4.patch276 bytes

Edit: ignore this, it only contains the CSS fix but there is also a problem with the markup (see updated IS).

herved’s picture

herved commented
Status: Active » Needs review
herved’s picture

herved commented
Issue summary: View changes
herved’s picture

herved commented
StatusFileSize
new fontawesome_fixes-3378557-7.patch1.54 KB

Here is the patch with both CSS and markup fixes.
AXE has no issues with that: https://jsfiddle.net/s23w8q4j/

herved’s picture

herved commented
Related issues: +#3182802: Aria-label for external links span throws errors on Accessibility Arc Toolkit
herved’s picture

herved commented
Issue summary: View changes
StatusFileSize
new fontawesome_fixes-3378557-9.patch2.31 KB

Fixing tests...

jbaum_13’s picture

jbaum_13 commented

This patch #9 worked for me! Thank you so much!

baysaa’s picture

baysaa commented

Is `sr-only` a native Drupal CSS class? I don't know so asking. I think traditionally `visually-hidden` may be the more appropriate class to use here. So I'd take the patch in the related issue over this one.

herved’s picture

herved commented

Hello @Baysaa,

sr-only is a fontawesome-specific class, added since version 4.6.0 (commit)
I'm not a maintainer but I'm not against changing it to visually-hidden which would make it independent from any FA version.
Feel free to post a patch.

clarksl@gmail.com’s picture

clarksl@gmail.com commented

Path #9 worked for me as well. Thanks!

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

Thanks, could this please be turned into a MR for further review and merge?

herved opened merge request !13

herved’s picture

herved commented

Hi @Anybody, sure, done.

baysaa’s picture

baysaa commented

@herved if the icon's using FA then it makes sense that sr-only should also work, thanks!

smustgrave’s picture

smustgrave commented
Version: 8.x-1.x-dev » 2.0.x-dev
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Think visually hidden would be better as even though we have font awesome icons the library might not be present (so the checkbox setting is unchecked) so not sure sr-only would cover all cases.

smustgrave closed merge request !13

smustgrave opened merge request !45

  • smustgrave committed b9c5a204 on 2.0.x 
    Issue #3378557 by herved, smustgrave, baysaa: Fix Font Awesome markup...
smustgrave’s picture

smustgrave commented
Status: Needs work » Fixed
cboyden’s picture

cboyden commented

I'm testing the latest dev of the 2.0.x branch, including this commit, and it's still outputting aria-label inappropriately.

To reproduce:

  1. Install Drupal core 10.3.6 using the standard profile.
  2. Require extlink at 2.0.x-dev via composer.
  3. Log in as an administrator.
  4. Enable the extlink module.
  5. In the extlink settings, check the checkbox for "Use Font Awesome icons instead of images" and save the configuration.
  6. Rebuild the site cache.
  7. View the automatically-generated home page content of the standard profile, which includes external links.
  8. Using the browser code inspector, view the code for the "Drupal community" link:
<a href="https://www.drupal.org/community" class="ext" data-extlink="" rel="noreferrer">Drupal 
  <span class="extlink-nobreak" aria-label="(link is external)">community
    <span class="fa-ext extlink" role="img" aria-hidden="false">
      <i class="fa fa-external-link" data-extlink-placement="append">
      </i>
      <span class="visually-hidden"></span>
    </span>
  </span>
</a>

First, the aria-label attribute is being output on a nested span tag. The attribute isn't intended for use on elements with a generic role, including the HTML div and span tags. Most assistive technologies will ignore aria-label on generic tags.

Second, the aria-hidden attribute doesn't need to be added if the value is "false." See aria-hidden where the Note says "At the time of this writing, aria-hidden="false" is known to work inconsistently in browsers. As future implementations improve, use caution and test thoroughly before relying on this approach." In the proposed draft for ARIA 1.3, the "false" value is supposed to be handled the same as if it's undefined (i.e. attribute is not present).

Third, the span containing the FA icon markup has a role of "img" and is not hidden from assistive technology, which means it should have something in the way of alt text, but the markup inside that span doesn't contain any text.

herved’s picture

herved commented

Indeed this doesn't seem valid: https://jsfiddle.net/cafsjowu/1/
It looks like #3172378: Prevent extlink icons from breaking into new lines recently committed added an extra span wrapper in the mean time?
Can you please create a new issue @cboyden? thanks
PS: The added code there looks convoluted. I wonder, wouldn't simply &nbsp; work?

Edit: Ok, we can fix it here I hope.

smustgrave’s picture

smustgrave commented

I'll revert this when I'm back in front of my computer.

smustgrave’s picture

smustgrave commented
Status: Fixed » Needs work

Reverted

  • smustgrave committed 6bc2a773 on 2.0.x 
    Revert "Issue #3378557 by herved, smustgrave, baysaa: Fix Font Awesome...
smustgrave’s picture

smustgrave commented

So what exactly is needed for this one? 1 of 2 bugs left open.

smustgrave’s picture

smustgrave commented
Status: Needs work » Postponed (maintainer needs more info)

Does https://www.drupal.org/project/extlink/issues/3492645 fix the issue here?

smustgrave’s picture

smustgrave commented
Status: Postponed (maintainer needs more info) » Closed (outdated)

Since there's been no follow up

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.