Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add aria-label="link text - new window" attribute in <a> tag when "open in new window" is checked #4450

Closed
cbizingre opened this issue Jan 13, 2018 · 12 comments
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes).

Comments

@cbizingre
Copy link

cbizingre commented Jan 13, 2018

Issue Overview

When a user checks "open in a new window" in link settings, add automatically aria-label attribute in the generated link tag :
<a href="..." target="_blank" aria-label="link text - new window">link text</a>

@youknowriad youknowriad added the [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). label Jan 13, 2018
@audrasjb
Copy link
Contributor

Hi,

If guess we have to be consistent with the rest of WordPress core. This issue seems to be related to https://core.trac.wordpress.org/ticket/23432#comment:53 where we can see the current pattern is something like:

<a href="https://example.com" target="_blank" class="external-link">Good link text <span class="screen-reader-text">(link opens in new tab/window)</span></a>

In addition, a good practice should be to add rel="external noopener noreferrer" attribute to target blank links (see https://blog.dareboost.com/en/2017/03/target-blank-links-rel-noopener-performance-security/ for further informations).

Moreover, it is more or less what is used in Gutenberg external link component:

function ExternalLink( { href, children, className, rel = '', ...additionalProps } ) {
	rel = uniq( compact( [
		...rel.split( ' ' ),
		'external',
		'noreferrer',
		'noopener',
	] ) ).join( ' ' );
	const classes = classnames( 'components-external-link', className );
	return (
		<a { ...additionalProps } className={ classes } href={ href } target="_blank" rel={ rel }>
			{ children }
			<span className="screen-reader-text">
				{
					/* translators: accessibility text */
					__( '(opens in a new window)' )
				}
			</span>
			<Dashicon icon="external" />
		</a>
	);
}

😊

@afercia
Copy link
Contributor

afercia commented Jan 14, 2018

Yep, it would be great to help produce more accessible links in the content. Couple things about the text to use:

  • it should avoid to repeat the word "link"
  • the text used in WordPress is (opens in a new window) because the parenthesis make screen readers do a short pause, which is a good thing

@cbizingre
Copy link
Author

Sorry, <a href="...">text link</a> is better.
Yes, a screen reader says "link" when it finds a <a> tag.
"-" is also a separator and make screen readers do a short pause.
"(opens in a new window)" or "- opens in a new window", perhaps the same result ?

@audrasjb
Copy link
Contributor

audrasjb commented Jan 15, 2018

Ok. Here is an example of HTML markup for target blank links:

<a href="https://example.com" target="_blank" rel="external noopener noreferrer">My text anchor<span class="screen-reader-text">(opens in new window)</span></a>

I kicked out the external-link class as we can always target it with CSS with other ways than classes.

Are we ok to say this is a clean way to manage these links?

Is so, I'll work on an integration in G. this week 😃

@afercia
Copy link
Contributor

afercia commented Jan 15, 2018

If possible, a space before the opening parenthesis would be 👍
<a href="https://example.org" target="_blank" rel="external noopener noreferrer">My text anchor<span class="screen-reader-text"> (opens in a new window)</span></a>

Also, a while ago someone pointed out that "in a new window" isn't 100% correct, as many browsers open a new tab by default. It also depends on the browser settings, could be a new tab or new window. I guess consistency with the wording already used in core is more important though.

@youknowriad
Copy link
Contributor

Closing since the option has been removed

@afercia
Copy link
Contributor

afercia commented Sep 10, 2018

Reopening since the option has been restored in #6028

@afercia afercia reopened this Sep 10, 2018
@afercia afercia added this to the WordPress 5.0 milestone Sep 10, 2018
@ryanwelcher
Copy link
Contributor

@afercia I have been looking into this and it seems that inserting the span into the markup is not ( as far as I can tell ) possible using the API that is handling turning the text into the link
However, is possible to add some attributes. Would adding aria-label=" (opens in a new window)" ( or something similar )be an acceptable solution?

@ryanwelcher
Copy link
Contributor

@afercia as a followup, I have created a PR as a starting point that adds an aria-label to the link if set to open in a new window with the following format:

aria-label="{SELECTED TEXT} (opens in a new window)"

@afercia
Copy link
Contributor

afercia commented Oct 25, 2018

@ryanwelcher thanks! Worth noting the standard text in core is:

(opens in a new tab)

See also #10697

As long as the link content is exclusively text, an aria-label attribute should be fine. Not sure it the link contains images (with an alt attribute) or other things because aria-label overrides any content of the link.

@timwright12
Copy link
Contributor

This is great! A quick note: I would remove rel="external" as it is reserved for external links only and someone could certain add an internal link into as a URL that they want opening in a new tab/window. Might be a good item to open up in another ticket to check if the URL is internal or external and conditionally add the attribute. ( @ryanwelcher @afercia )

@ryanwelcher
Copy link
Contributor

@afercia @timwright12 I've pushed a change that updates the text and removes external from the rel

@afercia from what I can see in the block, I can't insert an img so I think we're OK.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes).
Projects
None yet
Development

No branches or pull requests

6 participants