A11y: add screen reader text for external links

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 🐛 Aria-label for external links span throws errors on Accessibility Arc Toolkit Needs work so I copied that commit from https://git.drupalcode.org/project/extlink/-/merge_requests/18 to https://git.drupalcode.org/project/extlink/-/merge_requests/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 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 other people think are helpful, especially when they are already out there in use on production sites.

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 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?

smustgrave → closed merge request !16

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.

Automatically closed - issue fixed for 2 weeks with no activity.

Potentially related: 🐛 `[aria-*]` attributes do not match their roles Needs work