A11y: add screen reader text for external links

Hello,

+++ b/extlink.js
@@ -203,6 +203,18 @@
+        if(old_title.indexOf('New window') === -1){

The translated string needs to be tested.

Also whitespace before and after parenthesis.

+++ b/extlink.js
@@ -203,6 +203,18 @@
+      // Add to the title attr of ext link a 'New window' value for screen reader.

In comment, use the whole word "attribute" and not "attr".

Tagging the issue.

Good for me except checkstyles.

The patch from #5 doesn't appear to check against the ExtLink module's 'Open external links in a new window.' setting before applying title="(New window)" to the link <a> tag. It should probably check for that since if that option isn't enabled, the link doesn't open in a new window.

Queeing tests on a more recent PHP version.

@Piegefull: Patch needs to be updated. I can help if you need it :).

I feel like this should be an optional setting -- personally. Like, there are other ways to provide this info for WA purposes -- WCAG recommends putting it in the link text, which I realize is closely related to the title attribute, BUT, I really think this should be an optional setting :-/

(...but I don't have time to help with the code! Just sharing my two cents, I appreciate y'all's efforts.)

One of my colleagues on our WA team said that some screen readers ignore the title attribute -- and even if the software doesn't ignore it, screen reader users often do, because it's read after the link text (so by the time it gets to the title attribute, they've already "clicked" (or not clicked)).

**Either way,** as I'm thinking/typing, I think that whatever is implemented, there should be a reference to the relevant WA guidelines/rules/etc. -- not necessarily the WCAG link I shared, someone else might have a better reference, but it should be in the issue description and release notes/changelog. (Do citation-ish links like that go into code comments, too, or not usually? Idk what the normal practice is.)

Hello,

Here is an updated version of patch from comment 8.

Now there is a dedicated option, an update hook and a dedicated test.

Unfortunately, my environment does not currently allows me to have Javascript tests working.

So we will see with testbot.

I'm picking this up at DrupalCon Amsterdam.

Helping with this issue as mentor on Drupalcon Amsterdam 2019

YAY! First time mentoring at DrupalCon Amsterdam 2019.

Helping with this issue at DrupalCon Amsterdam 2019

Patch applied ok but found an issue when trying to change the configuration of the module. When you check 'Open external links in a new window.' another option appears 'Add "(New window)" in the title of links that should open in new window.'. After checking this and saving the 'Add (New Window)' option unchecks. So the save does not persist.

I'm presuming this is the required actions to test this properly? Apologies if I've picked this up wrong?

Also when viewing the page we then got a JS error in Drupal.js:

drupal.js?v=8.7.8:13 Uncaught TypeError: $link[icon_placement] is not a function
    at Object.Drupal.extlink.applyClassAndSpan (extlink.js?v=8.7.8:220)
    at Object.Drupal.extlink.attach (extlink.js?v=8.7.8:107)
    at Object.Drupal.behaviors.extlink.attach (extlink.js?v=8.7.8:249)
    at drupal.js?v=8.7.8:25
    at Array.forEach (<anonymous>)
    at Object.Drupal.attachBehaviors (drupal.js?v=8.7.8:22)
    at drupal.init.js?v=8.7.8:16
    at HTMLDocument.t (ready.min.js?v=1.0.8:4)

retagging

I fixed the testing issue. But now the tests aren't passing.

Could someone please reroll this patch?

Rerolled the patch.

Removing the failing line $this->assertSession()->statusCodeEquals(200);. I don't think it's really needed in this test.

Patch got failed, Needs a reroll

Please review the patch.

Please review!

Please review!

Please review.

please review the patch.

Please review the patch.

Thanks

I've applied the patch from #41 but external links still do not have a title attribute.

Thank you for the effort to make this more accessible!

I've rerolled #41 for 8.x-1.7.

#43 adds the title string twice (title="(New window) (New window)").
This should fix it.

Updating issue description to be more comprehensive.

The attached patch :

1. makes the "(new window)" label configurable
2. compares the title and the "(new window)" label in a case-insensitive manner without the parenthesis. See Drupal.extlink.compareLabels().
3. combines the two titles in a more readable manner, see Drupal.extlink.combineLabels().

It does not update the tests.

I created a MR, so I'm hiding all of the patches attached to this ticket, to avoid confusion.

There is some overlap (and conflicts) with #3182802: Aria-label for external links span throws errors on Accessibility Arc Toolkit so I copied that commit from MR!18 to MR!16 and resolved the conflicts.

Accessibility isn't optional. Why is there an option for "Add "(New window)" in the title of links that should open in new window."? The answer to that checkbox needs to be identical to the answer to "Open external links in a new window or tab." So we don't need both.

Either the links are opened in new windows (AND they have a warning) or they are not (and there is no warning). There is not a use-case for having links open in new windows without a warning. That's not accessible.

While I dislike removing options that other people think are helpful, I agree with @jenlampton that the new option extlink_target_append_new_window should not be an option but just always be true.

I'm a little less clear on whether the other new option extlink_target_append_new_window_label should be configurable or some set text. The fact that extlink_target_label is configurable makes me think that extlink_target_append_new_window_label should also be an option.

Also a little unclear to me is if the new JS I wrote should even exist. If both labels are configurable, and if the defaults are as we currently have them, this JS will never be used. And since the site builder has full control over this, is this new JS a waste of time to run in the browser and maintain in code?

This was made an option because of comments starting in #11 above without any discussion until #51. Also, #51 suggested linking to WCAG somewhere, but that was never done. And IMO that is a better thing to have done than making the accessible feature optional. What should we link to?

> #51 suggested linking to WCAG somewhere, but that was never done.

We could link to https://www.w3.org/TR/2012/NOTE-WCAG20-TECHS-20120103/G201.html

The text they include in that example is (opens in new window) so I'm tempted to always use that exact text and not allow for it to be modified. The only use-case for modification of the (opens in new window) text would be if someone had already entered the missing warning into the existing link is external text.

Making neither configurable would remove that use-case, but I also dislike the idea of removing options that already in use on production sites. If the link is external text changed suddenly it could cause problems for production sites with translations. (Though I wonder about the use-case for that as well. Was it only so that people could add the missing warning?)

In a perfect world I'd like the screen-reader text to always be the same, either (link is external) for opening links in the same window, or (link is external, opens in new window) for opening links in a new window. That would make the same translations work for everyone, and for a consistent screen-reader experience across sites.

To remove the link is external text override, however, we'd need to write a BC layer that would check for an existing value that's different from the default, and if that existed, use that override instead of the non-configurable text. We could even have the admin UI hide the existing field for the override when it's not already in use.

It's more work, but would make for a better user experience as well as more consistent accessible output.

I like the idea of removing the config option extlink_target_append_new_window_label which defines the label text and is new to this patch, always using the recommended text, and linking to the external resource (from above). This isn't removing an option in production. This is removing an option that someone in this thread thought helpful, and replacing it with a standardized (maybe industry standard) a11y text.

The current patch checks for "new window" in the existing label (not "opens in a new window") so I think that the existing patch is already backwards compatible with production systems.

If we agree, then what's left is to remove the config option extlink_target_append_new_window_label, link to the external resource, and make sure the tests work. If I understand correctly, we're pretty much at the place that you started with this ticket, it just took me a while to get there (sorry). What do you think?

Tests are all green and all eslint issues fixed for the pipeline. Thoughts? Would like to include with 2.0.x release.

Going to get a little ahead of myself and merge as this is the last one on the radar for a beta1 release.

Potentially related: #3275748: `[aria-*]` attributes do not match their roles when using FA icons