Add an icon and visually hidden a11y text to links that open in a new tab #7883

talldan · 2018-07-11T04:35:32Z

Description

Adds an icon and the visually hidden text '(Opens in a new window)' for improved a11y to two links that open in a new tab.

Use ExternalLink component for the link editor toolbar option on the Paragraph block. It replaces an a tag. This adds an additional icon and the visibly hidden text to the url.
Use the ExternalLink component for the permalink url when editing the post header

Furthermore a change has been made to the preview button in the top toolbar so that is a button element instead of an anchor.

How has this been tested?

Manually tested visual appearance of icon
Manually tested 'Opens in a new window' a11y text using VoiceOver
Ensured automated tests pass

Screenshots

Permalink:

Preview (visually unchanged, but is now a button element):

Paragraph Block Link:

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.

talldan · 2018-07-11T04:44:55Z

@karmatosed Would be great if you could just double-check the screenshots look ok. The only thing I spotted is that the link text color isn't the same for the permalink and the rich text link. I can make a separate issue if you think that's worth making consistent.

talldan · 2018-07-11T04:57:34Z

components/button/style.scss

@@ -181,6 +181,15 @@
 		padding: 0 8px 1px;
 		font-size: 11px;
 	}
+
+	.components-external-link__icon {


I wasn't entirely sure that these are the right selectors to use, or whether I should somehow specify a class of components-button__icon on the icon.

The class should begin with components-button__ since the element is in the Button component.

talldan · 2018-07-11T05:17:52Z

ah - I need to add some tests, particularly on the button element. Will add some asap.

noisysocks · 2018-07-11T06:50:17Z

Very supportive of the changes to the link formatting tool and permalink editor here. It looks great 👍

I'm not so sure about changing the Preview button, though.

In my mind¹, the point of the external icon is to shape the user's expectation so that they're not surprised when a new tab opens. This makes total sense since the expectation when clicking a hyperlink is that the browser navigates to a new page in-place.

But I don't think users have any such expectations when they click on a button. A button doesn't imply that clicking it will navigate to a new page. It's a button—buttons can do anything!

Could an alternative to this be to make the Preview button a <button> instead of an <a>? Or, could we add we add aria-role="button" and have it open when a user selects it and presses Space?

¹ Read: I am not a UX designer! 🙂

karmatosed

@noisysocks is hereby granted temporary 'UX designer' status as he was on point with suggesting this doesn't get done to the preview button :) I would say apart from that so long as things are just links (not buttons hiding in link clothing) this is a great improvement.

noisysocks · 2018-07-16T01:22:32Z

editor/components/post-permalink/index.js

@@ -83,6 +83,7 @@ class PostPermalink extends Component {
 						href={ ! isPublished ? postLink : samplePermalink }
 						target="_blank"
 						ref={ ( permalinkButton ) => this.permalinkButton = permalinkButton }
+						isExternalLink


Rather than modifying Button, could we just make PostPermalink use an ExternalLink component? It doesn't look like we need any Button functionality in this case, and I'm wary of Button becoming a component that has too many responsibilities.

<ExternalLink className="editor-post-permalink__link" href={ ! isPublished ? postLink : samplePermalink } ref={ this.permalinkRef } />

(To make the above work, we'll likely need to make ExternalLink forward refs.)

afercia · 2018-07-16T07:13:09Z

and I'm wary of Button becoming a component that has too many responsibilities.

This is the main reason why a previous attempt in #6786 was abandoned. It ended up in adding a lot of stuff to the Button component. Currently, the Button component is named based on its visual appearance. This is far from ideal and introduces some potential confusion. Components should be named based on what they do. In the discussion on #6786 (comment) at some point it was agreed to split the Button component in two separate components, in short:

Link: it's an <a> element, used to navigate to a web resource (i.e. no href="#" links used to "do something") and visually styled as a link or a button
Button: it's a <button> element, used to "do something" and visually styled as a button or link

Based on that #7534 was created to separate the two components and eliminate ExternalLink. I know that takes a different path from the work done here, sorry for that, but I'd strongly encourage to focus on the effort proposed on #7534.

talldan · 2018-07-16T10:26:29Z

Agree that separate components would be nice, I've addressed most of the feedback now.

Will take a look at this failing test tomorrow.

noisysocks

Aside from the failing test this looks great! 🙂

noisysocks · 2018-07-17T01:17:25Z

It looks like the failing test was just an intermittent E2E test failure. I restarted the job for you.

talldan · 2018-07-17T03:18:04Z

Great - thanks @noisysocks.

@karmatosed - I've updated the screenshots at the top, are you happy to approve this now?

talldan · 2018-07-17T03:26:53Z

Btw, I spotted a bug - the edit link popover in paragraph blocks doesn't always open in the right place (It's ok the first time you select a link, but the second time it goes wandering off). That bug is present in master so I'll make a separate issue for it (#7992).

karmatosed · 2018-07-17T08:53:36Z

@talldan do we need to have the link icon in the actual link interface? That feels over the top to me.

talldan · 2018-07-18T03:17:34Z

@karmatosed - do you mean this icon?

I thought that was one of the main points we were trying to address from #1105. My opinion is that we should use the icon consistently on anything that looks like and interacts like a link.

I think in any case where a user might be concerned about losing their state or unsaved changes it's good to indicate the link will launch in a new tab.

karmatosed · 2018-07-18T13:13:04Z

@talldan my understanding was that it was for interface links not links in the drop down. It's this bit I am questioning:

If everyone wants that's ok it just feels a little odd seeing beside something like this.

Links added in RichText are opened in a new tab when using the editor toolbar. This commit switches the anchor tag with an ExternalLink component. This makes the link more accessible (it adds some hidden text) and provides an icon indicating that the link is external.

…s selectors

For accessibility purposes the post preview button is now rendered as a button, meaning it can be interacted with using the keyboard (space bar to open a preview page).

… for scss selectors" This reverts commit 77422fce000a2b75788b5fadaa1cd26e770799e3. Revert "Add tests for isExternalLink prop in Button component" This reverts commit 5102826e2c22c7ae59c8d21a3b1fb44781babe5f. Revert "Use flex to center icon in button and add a tiny bit of margin to adjust" This reverts commit 0f3c5fa792dc3386e05acd3821ea32b245b42377. Revert "Set isExternalLink true for post permalink and adjust styling" This reverts commit 402ecd3ff93f3b945072124f4bd47e15fb3470ce. Revert "Position dashicon for external links when rendered via Button" This reverts commit 0930efac6c9eb709b5af7e0e28e9d05d3346bebd. Revert "Allow Button component to render an ExternalLink" This reverts commit 1b8e9ca5c5fe67aab5d08d2ffc0798fb7edfaa8d. Revert "Add prop for icon class name to ExternalLink" This reverts commit ddac73dd0f09a1076f842fdb89ece370e274ac82.

…button

talldan · 2018-07-23T02:24:42Z

@karmatosed My feeling is that it should be consistent across links. I'm happy for you to make a call and I'll make the necessary changes. Here's a gif so you can see what it looks like in action:

karmatosed · 2018-07-24T01:31:48Z

Let's add it in @talldan and iterate.

talldan self-assigned this Jul 11, 2018

talldan requested review from karmatosed and afercia July 11, 2018 04:41

talldan requested a review from noisysocks July 11, 2018 04:53

talldan commented Jul 11, 2018

View reviewed changes

noisysocks added the Needs Design Feedback Needs general design feedback. label Jul 11, 2018

karmatosed requested changes Jul 11, 2018

View reviewed changes

karmatosed removed the Needs Design Feedback Needs general design feedback. label Jul 11, 2018

talldan force-pushed the fix/external-links branch from e241a6a to 77422fc Compare July 13, 2018 02:51

talldan mentioned this pull request Jul 13, 2018

Saving: Autosaveable does not account for autosave existing at start of editing #7416

Closed

noisysocks requested changes Jul 16, 2018

View reviewed changes

talldan force-pushed the fix/external-links branch from 9d284b3 to 39f82fe Compare July 16, 2018 09:46

noisysocks approved these changes Jul 17, 2018

View reviewed changes

talldan added 16 commits July 23, 2018 09:55

Add prop for icon class name to ExternalLink

d49339e

Allow Button component to render an ExternalLink

7b1b42d

Specify PostPreviewButton as opening external links

2938366

Fix overly specific selector preventing override of icon styles

e9686f6

Position dashicon for external links when rendered via Button

90e7e57

Set isExternalLink true for post permalink and adjust styling

1d61f51

Fix incorrect default display property given to button

0c01440

Use flex to center icon in button and add a tiny bit of margin to adjust

80496ab

Add tests for isExternalLink prop in Button component

33cd704

Pass component-button__icon class through to icon, and use it for scs…

241a84f

…s selectors

Make post preview button render as a button instead of a link

953beb1

For accessibility purposes the post preview button is now rendered as a button, meaning it can be interacted with using the keyboard (space bar to open a preview page).

Make the post permalink link an ExternalLink instead of a Button

b7dc451

Rename member variable used as ref to reflect that it's not longer a …

fdfcc2b

…button

Modify ExternalLink component to forwards its ref

3eeb9f2

Adjust styling to work in a wider range of contexts

e1a63e8

talldan force-pushed the fix/external-links branch from 6d47458 to e1a63e8 Compare July 23, 2018 01:56

karmatosed approved these changes Jul 24, 2018

View reviewed changes

talldan merged commit 10e87a9 into master Jul 24, 2018

talldan deleted the fix/external-links branch July 24, 2018 01:56

talldan added this to the 3.4 milestone Jul 24, 2018

mmtr mentioned this pull request Jul 28, 2018

Fix components styles import #8264

Merged

afercia mentioned this pull request Aug 29, 2018

The Preview button should be a link #9446

Closed

afercia mentioned this pull request Jul 24, 2024

When adding a link that opens in a new tab, the screen reader text does not honor the site language #18727

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add an icon and visually hidden a11y text to links that open in a new tab #7883

Add an icon and visually hidden a11y text to links that open in a new tab #7883

talldan commented Jul 11, 2018 •

edited

Loading

talldan commented Jul 11, 2018

talldan Jul 11, 2018

noisysocks Jul 11, 2018 •

edited

Loading

talldan commented Jul 11, 2018

noisysocks commented Jul 11, 2018

karmatosed left a comment

noisysocks Jul 16, 2018

afercia commented Jul 16, 2018

talldan commented Jul 16, 2018

noisysocks left a comment

noisysocks commented Jul 17, 2018

talldan commented Jul 17, 2018

talldan commented Jul 17, 2018 •

edited

Loading

karmatosed commented Jul 17, 2018

talldan commented Jul 18, 2018 •

edited

Loading

karmatosed commented Jul 18, 2018

talldan commented Jul 23, 2018

karmatosed commented Jul 24, 2018

Add an icon and visually hidden a11y text to links that open in a new tab #7883

Add an icon and visually hidden a11y text to links that open in a new tab #7883

Conversation

talldan commented Jul 11, 2018 • edited Loading

Description

How has this been tested?

Screenshots

Types of changes

Checklist:

talldan commented Jul 11, 2018

talldan Jul 11, 2018

Choose a reason for hiding this comment

noisysocks Jul 11, 2018 • edited Loading

Choose a reason for hiding this comment

talldan commented Jul 11, 2018

noisysocks commented Jul 11, 2018

karmatosed left a comment

Choose a reason for hiding this comment

noisysocks Jul 16, 2018

Choose a reason for hiding this comment

afercia commented Jul 16, 2018

talldan commented Jul 16, 2018

noisysocks left a comment

Choose a reason for hiding this comment

noisysocks commented Jul 17, 2018

talldan commented Jul 17, 2018

talldan commented Jul 17, 2018 • edited Loading

karmatosed commented Jul 17, 2018

talldan commented Jul 18, 2018 • edited Loading

karmatosed commented Jul 18, 2018

talldan commented Jul 23, 2018

karmatosed commented Jul 24, 2018

talldan commented Jul 11, 2018 •

edited

Loading

noisysocks Jul 11, 2018 •

edited

Loading

talldan commented Jul 17, 2018 •

edited

Loading

talldan commented Jul 18, 2018 •

edited

Loading