[Link] Warn users when links open in a new window #1247

ry5n · 2019-03-25T23:41:36Z

WHY are these changes introduced?

Fixes Shopify/polaris-ux#146

WHAT is this pull request doing?

This PR takes the position that we should automatically do the right thing for accessibility whenever possible. It automatically adds screen reader text and an icon when the external prop is true.

Unfortunately, implementation is complicated by CSS things, historical things, and international things (see comments below).

I’m looking for comments on this approach before going any further.

How to 🎩

The icon will be blank when tophatting—we’re waiting on the correct icon to be available (should be in the next release of Polaris Icons).
Change the viewport width to make sure the icon wraps with the last word in the link, never on its own.

🖥 Local development instructions
🗒 General tophatting guidelines
📄 Changelog guidelines

Copy-paste this code in playground/Playground.tsx:

import * as React from 'react';
import {Page, Link, TextContainer} from '../src';

interface State {}

export default class Playground extends React.Component<{}, State> {
  render() {
    return (
      <Page title="Playground">
        <TextContainer>
          <p>
            This is some sample text to make the paragraph longer, in order to
            test text wrapping. Links to external sites, like the{' '}
            <Link url="https://help.shopify.com/manual" external>
              Shopify Help Center
            </Link>
            , have a trailing icon and open in a new browser tab.
          </p>
          <p>
            これはテキストの折り返しをテストするために段落を長くするためのサンプルテキストです。{' '}
            <Link url="https://help.shopify.com/manual" external>
              Andersonヘルプセンター
            </Link>
            などの外部サイトへのリンクには末尾のアイコンがあり、新しいブラウザタブで開きます。
          </p>
        </TextContainer>
      </Page>
    );
  }
}

🎩 checklist

Tested on mobile
Tested on multiple browsers
Tested for accessibility

src/components/Link/Link.tsx

  const className = classNames(styles.Link, monochrome && styles.monochrome);
+  let childrenMarkup = children;
+
+  if (external && typeof children === 'string') {


ry5n · 2019-03-25T23:48:02Z

src/components/Link/Link.tsx

+        <span className={styles.IconLockup}>
+          {lastWord}
+          <Icon accessibilityLabel={iconLabel} source={ExternalSmallMinor} />
+        </span>


In order to center the icon with the text, and to prevent the icon from wrapping by itself to the next line, the best solution I found is to wrap the last word and the icon in an inline-flex container.

This way of preventing unwanted wrapping won’t work with languages like Japanese because those languages don’t use spaces between words. I’m not sure what to do about this.

For languages that does not use space between words, you can grab the last character + punctuation as a word. Since each character is consider a word, a single character on the second line is perfectly fine. But punctuation should never be wrap onto the next line by itself.

✅
你好
嗎。

❎
你好嗎
。

Thanks @michelleyschen! Would we want an icon at the end of a text link to behave the same way as punctuation?

yes, I would consider icon a punctuation

Thanks. I have a new implementation that doesn’t manipulate the children string. Should work for all languages now 😄

yourpalsonja · 2019-03-27T22:04:06Z

What if we ignore the content of Link and just rely on the external prop to add a class that:

Adds a(n) :before/:after element that contains the icon (since we know it won't change) and is positioned absolutely, so that it won't wrap to the next line but also
Adds padding or a margin to the end of the element so that the entire link will wrap to the next line if things get too jammed up.
This would work for both RTL languages, and languages like Japanese.

Caveats: Could be weird if someone decides to use a :pseudo element for something else, but I don't think that can happen by default based on my reading of the code.

ry5n · 2019-03-27T23:18:23Z

@yourpalsonja Thanks for looking at this. That’s an interesting idea! I tried it a couple ways, and I think something like it could work.

Polaris React uses inline SVGs, relying on them for being able to recolor icons for interaction effects like hover. I am not sure, but I don’t think inline SVGs play well with CSS generated content. BUT, if we made the link inline-flex, we would achieve the same overall effect (without needing absolute positioning).

The downside is that as you pointed out, the whole link wraps as a unit, which will cause some disruption to the flow of the text on narrower columns when links are long. Example:

Both approaches have tradeoffs. Are there other approaches?

dleroux · 2019-03-28T12:17:00Z

I tried inserting a   at the end of the string and then concatenating the Icon hoping it wouldn't wrap but it did. Guessing because the Icon is html and not just text.

Have we considered something like a Tooltip? In the end visually impaired users are warned of the new window via the rel attribute. A simple tooltip would keep the ui clean and meet the requirement of warning sighted users. It's one of the examples provided by the w3c: https://www.w3.org/TR/WCAG20-TECHS/G201.html

1 caveat with our Tooltip implementation is that the link would not wrap.

dpersing · 2019-03-28T14:34:17Z

Have we considered something like a Tooltip?

@dleroux We currently have a few logged a11y issues around tooltips (https://github.com/Shopify/polaris-ux/issues/154, #793), so I'd like to avoid adding more complexity for the link.

The rel attribute isn't actually conveyed to screen reader users via the accessibility API, which is why we've had to implement workarounds like this. There's also concern for sighted keyboard-only users, who will want to be able to fully control their experience, as well as low vision users relying on tools like magnification, and users with cognitive issues, who may not be expecting or notice a new tab/window.

Adds a(n) :before/:after element that contains the icon (since we know it won't change) and is positioned absolutely, so that it won't wrap to the next line but also

@yourpalsonja If we add the icon as a pseudo-element, we'll need a way to provide the text equivalent for the icon as part of the link content. It doesn't appear that accessibilityLabel can be used on the Link component, so I recommended including the icon inline. If we could add that prop, that would be helpful for AT users in general, provided we add some guidance around its use like we have on the Button component.

yourpalsonja · 2019-03-29T00:25:51Z

@ry5n I was thinking the link wrapping in this case would be a good thing, so the text doesn't look too crowded with the link beside it? I could be wrong though.

@dpersing would we need to provide accessible text for the icon, or could we just add text that is read by the screen reader in addition to the icon? (like, "Visit our docs" [icon]), and a screenreader would read "Visit our docs, external link".

ry5n · 2019-03-29T01:29:09Z

Here’s a side-by-side of how the 2 wrapping options behave:

Link itself can wrap	Link wraps as a whole

Here’s what the the current PR does. Screen readers will see “(opens a new window)” appended to the link content. (This follows Devon’s recommendation in the original issue):

dpersing · 2019-03-29T15:15:49Z

would we need to provide accessible text for the icon, or could we just add text that is read by the screen reader in addition to the icon? (like, "Visit our docs" [icon]), and a screenreader would read "Visit our docs, external link".

@yourpalsonja I think it's 6 of 1 and half a dozen of the other!

I also want to clarify that it's less that it's an external link and more that it's a link that opens a new tab/window. The issue isn't with the content, but with the browser behavior, if that makes sense. @ry5n's "(opens a new window)" text does the trick!

ry5n · 2019-03-29T16:05:44Z

OK, I think I found another way. This is close to what Dan was thinking. I’ve tested this with different font sizes, and in Chrome, Firefox, Safari and Edge (CodeSandbox doesn’t work in IE11, so I couldn’t test there). I’ve also tested it with our different system fonts (San Francisco, Roboto, and Segoe UI).

https://codesandbox.io/s/9wymvnyrw

Here’s how this works:

It uses a non-breaking space between the Link text and the icon (not sure why this works when it didn’t for Dan).
It wraps the Icon component in a span set to inline-flex so that the icon can sit inline with the text.
A zero-width space character is included in the span along with the Icon. Because the space is an actual character with font metrics (while the icon isn’t), this ensures that the span has the same height as the rest of the text. Because it has the right height, flexbox centers the icon correctly.
The span is pulled backwards with negative margin to negate the width of the non-breaking space. I wanted to use a zero-width non-breaking space, but browsers don’t actually enforce the no-break behavior for this character.

dpersing · 2019-03-29T16:29:20Z

Looks great when text is resized too, @ry5n ! ❤️

ry5n · 2019-03-29T16:46:18Z

……except it actually doesn’t work in Polaris React, trying it for real. This is probably what Dan ran into. I’m stumped.

dleroux · 2019-03-29T16:52:02Z

Looks like the span around the last word might be it. https://snook.ca/archives/html_and_css/icon-wrap-after-text

ry5n · 2019-03-29T19:52:52Z

Looks like the span around the last word might be it.

Yeah. That brings us back to the code in this PR. I pushed an update that fixes cases where there are no spaces. It seems to work OK with Japanese at least.

I also reached out to the international team to see if there are any more gotchas with this approach. Once I’m confident in internationalization things, I can write some tests and will ask for final code review. Thanks all!

paulstairmand

From an internationalization perspective, the obvious issue I see is with languages that have no spaces between words. But it seems you have a solution to that @ry5n

The only other thing is choosing an icon that is internationally understandable, especially in our major markets. Once we have a suggested icon we can run it past folks in our in-country teams to check this.

ry5n · 2019-04-02T22:35:35Z

src/components/Link/Link.tsx

@@ -1,8 +1,11 @@
 import * as React from 'react';

 import {classNames} from '@shopify/react-utilities';
+import {ChevronLeftMinor} from '@shopify/polaris-icons';


This needs to be changed to ExternalSmallMinor once this icon is included in Polaris Icons. I believe this is the last blocker for this PR.

ry5n · 2019-04-02T23:35:50Z

OK folks, I think this is almost there. @dleroux I went back to your approach again, really dug into Snook’s article and codepen, and found a way to make it work with inline SVGs. I’m confident now this will work for multiple languages.

The only real thing still blocking this is the correct icon isn’t available in Polaris Icons yet.

dleroux · 2019-04-08T13:16:20Z

Nice work! 🎩 looks good.

Is the plan to roll this in now or wait until version 4?

dleroux · 2019-04-08T13:17:57Z

src/components/Link/Link.scss

@@ -18,6 +19,26 @@
  }
 }

+// Prevents the icon from wrapping on its own to the next line


I think these comments can go, it's pretty self explanatory. Could even call this ExternalIconLockUp if we want more clarity.

ry5n · 2019-04-08T19:53:48Z

The external link icon merged this morning, so this is ready for final review 😀

I’d like to get this into the current version—@dleroux do you think it’s a breaking change?

dleroux · 2019-04-08T20:39:03Z

I guess breaking would mean an api change. I just question the importance of a change when it could visually impact a design. Since we are checking for a ‘string’ the risk is minimal, so either way it should be ok.

dleroux

👍

BPScott temporarily deployed to polaris-react-pr-1247 March 25, 2019 23:41 Inactive

ry5n commented Mar 25, 2019

View reviewed changes

src/components/Link/Link.tsx

const className = classNames(styles.Link, monochrome && styles.monochrome);

let childrenMarkup = children;

if (external && typeof children === 'string') {

This comment was marked as outdated.

Sign in to view

ry5n commented Mar 25, 2019

View reviewed changes

ry5n requested review from dleroux, dpersing, yourpalsonja and danrosenthal March 25, 2019 23:49

ry5n self-assigned this Mar 25, 2019

BPScott temporarily deployed to polaris-react-pr-1247 March 29, 2019 19:22 Inactive

ry5n requested review from paulstairmand and removed request for yourpalsonja, dleroux, danrosenthal and dpersing March 29, 2019 19:53

paulstairmand reviewed Apr 2, 2019

View reviewed changes

ry5n force-pushed the external-link branch from e6c8eea to b295254 Compare April 2, 2019 22:33

BPScott temporarily deployed to polaris-react-pr-1247 April 2, 2019 22:34 Inactive

ry5n commented Apr 2, 2019

View reviewed changes

BPScott temporarily deployed to polaris-react-pr-1247 April 2, 2019 22:43 Inactive

BPScott temporarily deployed to polaris-react-pr-1247 April 2, 2019 23:20 Inactive

BPScott temporarily deployed to polaris-react-pr-1247 April 3, 2019 15:50 Inactive

dleroux reviewed Apr 8, 2019

View reviewed changes

BPScott temporarily deployed to polaris-react-pr-1247 April 8, 2019 17:42 Inactive

ry5n mentioned this pull request Apr 8, 2019

Update polaris-icons to include the full icon set #1284

Merged

ry5n force-pushed the external-link branch from db52dba to 773fe7a Compare April 8, 2019 19:44

BPScott temporarily deployed to polaris-react-pr-1247 April 8, 2019 19:44 Inactive

ry5n requested review from dleroux, paulstairmand and yourpalsonja April 8, 2019 19:46

ry5n changed the title ~~[WIP][Link] Warn users when links open in a new window~~ [Link] Warn users when links open in a new window Apr 8, 2019

dleroux approved these changes Apr 9, 2019

View reviewed changes

Add an icon and screen reader hint for external links

c4006b3

ry5n force-pushed the external-link branch from 773fe7a to c4006b3 Compare April 9, 2019 20:17

BPScott deployed to polaris-react-pr-1247 April 9, 2019 20:17 View deployment

ry5n merged commit bf2738a into master Apr 9, 2019

ry5n deleted the external-link branch April 9, 2019 22:15

alex-page temporarily deployed to production April 22, 2019 19:26 Inactive

dpersing mentioned this pull request Apr 24, 2019

[a11y] Update accessibility documentation for the external prop #1347

Merged

ry5n mentioned this pull request May 13, 2019

[List filters] [a11y] Add external link icon to opt-in card #1211

Closed

BPScott mentioned this pull request Jun 21, 2019

Link modifies color of SVG child #1724

Closed

ry5n mentioned this pull request Jun 24, 2019

Avoid recoloring any old icon inside Link #1729

Merged

1 task

BPScott mentioned this pull request Jul 9, 2019

Don't pass polaris from Link into UnstyledLink #1805

Merged

mzlock mentioned this pull request Oct 13, 2020

[a11y] External unstyled link causes change of context without warning #3476

Closed

BPScott mentioned this pull request Oct 22, 2020

[UnstyledLink] Add new window warning to unstyled link, pass translation to custom Link #3529

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Link] Warn users when links open in a new window #1247

[Link] Warn users when links open in a new window #1247

ry5n commented Mar 25, 2019 •

edited

Loading

This comment was marked as outdated.

ry5n Mar 25, 2019

michenly Mar 29, 2019

ry5n Mar 29, 2019

michenly Mar 29, 2019

ry5n Apr 2, 2019

yourpalsonja commented Mar 27, 2019

ry5n commented Mar 27, 2019 •

edited

Loading

dleroux commented Mar 28, 2019

dpersing commented Mar 28, 2019 •

edited by dleroux

Loading

yourpalsonja commented Mar 29, 2019

ry5n commented Mar 29, 2019

dpersing commented Mar 29, 2019

ry5n commented Mar 29, 2019

dpersing commented Mar 29, 2019

ry5n commented Mar 29, 2019

dleroux commented Mar 29, 2019

ry5n commented Mar 29, 2019

paulstairmand left a comment •

edited

Loading

ry5n Apr 2, 2019

ry5n commented Apr 2, 2019 •

edited

Loading

dleroux commented Apr 8, 2019

dleroux Apr 8, 2019

ry5n commented Apr 8, 2019 •

edited

Loading

dleroux commented Apr 8, 2019 •

edited

Loading

dleroux left a comment

[Link] Warn users when links open in a new window #1247

[Link] Warn users when links open in a new window #1247

Conversation

ry5n commented Mar 25, 2019 • edited Loading

WHY are these changes introduced?

WHAT is this pull request doing?

How to 🎩

🎩 checklist

This comment was marked as outdated.

ry5n Mar 25, 2019

Choose a reason for hiding this comment

michenly Mar 29, 2019

Choose a reason for hiding this comment

ry5n Mar 29, 2019

Choose a reason for hiding this comment

michenly Mar 29, 2019

Choose a reason for hiding this comment

ry5n Apr 2, 2019

Choose a reason for hiding this comment

yourpalsonja commented Mar 27, 2019

ry5n commented Mar 27, 2019 • edited Loading

dleroux commented Mar 28, 2019

dpersing commented Mar 28, 2019 • edited by dleroux Loading

yourpalsonja commented Mar 29, 2019

ry5n commented Mar 29, 2019

dpersing commented Mar 29, 2019

ry5n commented Mar 29, 2019

dpersing commented Mar 29, 2019

ry5n commented Mar 29, 2019

dleroux commented Mar 29, 2019

ry5n commented Mar 29, 2019

paulstairmand left a comment • edited Loading

Choose a reason for hiding this comment

ry5n Apr 2, 2019

Choose a reason for hiding this comment

ry5n commented Apr 2, 2019 • edited Loading

dleroux commented Apr 8, 2019

dleroux Apr 8, 2019

Choose a reason for hiding this comment

ry5n commented Apr 8, 2019 • edited Loading

dleroux commented Apr 8, 2019 • edited Loading

dleroux left a comment

Choose a reason for hiding this comment

ry5n commented Mar 25, 2019 •

edited

Loading

ry5n commented Mar 27, 2019 •

edited

Loading

dpersing commented Mar 28, 2019 •

edited by dleroux

Loading

paulstairmand left a comment •

edited

Loading

ry5n commented Apr 2, 2019 •

edited

Loading

ry5n commented Apr 8, 2019 •

edited

Loading

dleroux commented Apr 8, 2019 •

edited

Loading