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

[EuiLink] Adding new tab/window help text for screen readers #3659

Closed
myasonik opened this issue Jun 25, 2020 · 6 comments · Fixed by #4172
Closed

[EuiLink] Adding new tab/window help text for screen readers #3659

myasonik opened this issue Jun 25, 2020 · 6 comments · Fixed by #4172

Comments

@myasonik
Copy link
Contributor

Whenever a link is going to open in a new tab or window, we should warn screen readers ahead of time.

In an ideal world, they could do this for us but support is abysmal right now so we're going to have to help out.

The easiest way we can do this is adding some hidden text to the link text that reads "opens in a new window/tab" at the end of the link if the user passes in a target attribute.

Related Kibana issue: elastic/kibana#43020

@chandlerprall
Copy link
Contributor

I feel this falls under the bucket "should be fixed by the browser platform", similar to recent discussions around tabbing to scrollable div containers. I suppose the simple fix of adding an EuiScreenReaderOnly in affected EuiLinks makes the surface area for this very small, but could become complicated if/when individual browsers fix this on their end; wouldn't want the message duplicated.

@snide
Copy link
Contributor

snide commented Jun 29, 2020

I also feel this is something that is so simplistic on the browser side that I don't want to create something on our side to cover. It seems more like a bug with them, and while I know we tend to fix browser oversights for this in more complicated components, I feel the line where we do that starts is when we're making non-semantic UI like toasts.

I don't think this is something we should take on, but will leave it up for discussion for a bit.

@cchaos cchaos changed the title [EuiLink] adding new tab/window help text for screen readers [EuiLink] Adding new tab/window help text for screen readers Sep 20, 2020
@myasonik
Copy link
Contributor Author

I needed a short distraction and this seemed sufficiently small. I threw together a PR to show just how small the surface area for this is: #4172

could become complicated if/when individual browsers fix this on their end

Theoretically, I guess. But this is also an issue as old as dirt so there's no reason to assume this will come in any sort of immediate future and worst case we could revert this fix if it ever becomes to difficult to maintain what is currently a single conditional.

wouldn't want the message duplicated

It could be annoying to have a message be duplicate but I'd rather get the same message twice than 0 times and be left without context.

I also feel this is something that is so simplistic on the browser side that I don't want to create something on our side to cover. It seems more like a bug with them, and while I know we tend to fix browser oversights for this in more complicated components, I feel the line where we do that starts is when we're making non-semantic UI like toasts.

Am I understanding your point correctly that we'd fix this if it was more complicated but because it's so easy to fix we shouldn't? It'd seem to me that we should squash the easy bugs first while egging on browsers to solve the harder problems.

Even in this same file, we do a similar patch for what is effectively a browser/networking bug where we add noreferrer and noopener to rel if the link points to an external URL and the logic for that is twice as long as this fix.

@chandlerprall
Copy link
Contributor

Given that visual&audio warning when a link will open in a new window is a recommended technique (https://www.w3.org/TR/WCAG20-TECHS/G201.html), and that we do provide the visual indication through EuiLink's existing external prop, I think we should find a way to match with screen reader support.

Which opens up a new question: EuiLink's external prop is an indication that the link goes to another app/site rather than opens in a new window, but in practice (I presume) it is intended to be used along with target="_blank". So I'm not sure how we'd want that existing prop to interact (or not) with this screen reader text.

@myasonik
Copy link
Contributor Author

I think we leave them entirely separate (as it is now) and leave it up to consumers to ask for the icon or setup the target if they want it.

On the flip side, we could wrap up some more magic into this and auto add external if the href doesn't match the current URL and add target="_blank" and the screen reader text as well. But I'm not sure we really gain all that much for this added magic and it limits EUI's flexibility.

@chandlerprall
Copy link
Contributor

Talked with @cchaos and I'm good with her proposal of how target&external should interact:

  • target=_blank should add a11y warning and the external icon but allow it to be hidden with external={false} vs undefined
  • external: Just adds the icon with no other assumptions made

If we get that into your PR I think we'll be good

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants