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

Format library: link: remove aria-label #18742

Merged
merged 2 commits into from
Feb 13, 2020
Merged

Conversation

ellatrix
Copy link
Member

@ellatrix ellatrix commented Nov 26, 2019

Description

Fixes #12325.

I'm removing the aria-label from user created links as the current implementation is broken. The label text can go out of sync, it doesn't apply if you don't select any text, the language may be incorrect, and it doesn't apply consistently for all links in the post content.

The fact that the text may go out of sync is the most serious issue as this enhancement can do more damage than good, incorrectly announcing link content, so I think it's better to remove it until we find a better solution.

Generally, the whole things seems strange to me. This text should be something for the user agent to add, not the content creator. Ideally it shouldn't be inserted in the source at all. It should be added to every web page viewed in the browser. We also can't make it perfect. The user agent will know the language the user has set, which is not necessarily the language of the web page. (I may open a page in a language I don't understand.) The user agent will also know if the link opens in a new tab or window, and adjust the text accordingly.

Safari does this well it seems:

Screenshot 2019-11-26 at 10 06 49

If browsers are reluctant to fix the issue, a better place to fix the behaviour would be in a browser extension or screen reader software.

If we really, really want to do this, we should append screen reader text in PHP display filters, even though we lack context (browser language and tab/window setting).

The last place it should be added is at link creation with the link UI, if we want to label consistently.

How has this been tested?

Insert a link that opens in a new window. Make sure the source code doesn't have an aria-label.

Screenshots

Types of changes

Bug fix

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR. .

@ellatrix ellatrix added the [Package] Format library /packages/format-library label Nov 26, 2019
@youknowriad youknowriad added the Needs Accessibility Feedback Need input from accessibility label Nov 26, 2019
@ellatrix ellatrix added the [Type] Bug An existing feature does not function as intended label Nov 26, 2019
@afercia
Copy link
Contributor

afercia commented Nov 26, 2019

Not opposed to remove the current implementation but I'd like to know what are the plans for a new, better, implementation (maybe PHP side) before removing it. Is there any plan for that?

Also, I'm not sure user agents can reliable for this purpose, as language identification is subject to too many factors (including wrong page headers or lang attributes) or assumptions.

@ellatrix
Copy link
Member Author

Also, I'm not sure user agents can reliable for this purpose, as language identification is subject to too many factors (including wrong page headers or lang attributes) or assumptions.

@afercia What do you mean? The language should not be that of the web page, but that of the OS, which only the browser can know. In other words, the browser, or whatever user agent, or an extension is the best place to fix it.

I don't have any plans. I'd rather see this addressed in browser or screen readers, or an accessibility browser extension so it's consistent for all links on all web pages.

@ellatrix
Copy link
Member Author

If we insist on fixing this on WordPress pages (which, again, is not a full fix for all the pages a user is browsing...), I would recommend adding a small script on the front end to inject screen reader text in the DOM instead of trying to inject in in HTML.

@ellatrix
Copy link
Member Author

@afercia Do you have any resources stating that this is a good practice?

@afercia
Copy link
Contributor

afercia commented Nov 26, 2019

I proposed to discuss this issue / PR in the next accessibility meeting so I'd like to ask to wait a bit before merging it.

I'm not sure I understand the operating system thing.

The "opens in a new browser tab" text needs to be in the same language of the post content. Browsers or operating systems don't analyze the page content, not to my knowledge at least. They assume a certain language based on the page headers or lang attribute. Delegating to them seems highly unreliable to me.

Also, WordPress discourages the usage of target="_blank" at the point there's a blessed task on the core Trac to remove as many occurrences as possible from the admin pages. Same applies to content produced by users. It is discouraged, though still allowed. As WordPress allows target="_blank" it should also make sure users are informed properly and the post content is accessible. As I see it, producing accessible content is a WordPress responsibility.

@afercia
Copy link
Contributor

afercia commented Nov 26, 2019

@ellatrix when you have a chance, can you please clarify the steps to get that "open in a new tab" text displayed in Safari?

Screenshot 2019-11-26 at 10 06 49

@MarcoZehe
Copy link
Contributor

MarcoZehe commented Nov 26, 2019

I agree with @afercia on this. I work for a browser vendor, Firefox, to be exact, and the problem I have with this proposal is that the way language detection works is via the lang or xml-lang attributes either in the page header or overridden somewhere in the DOM. However, that is on an element level, not an attribute level one. Browsers expose that information to screen readers, and they can switch the synthesizer language or voice accordingly if the user has set this up. In that sense, content produced trough ARIA should be treated as if it was visual content for sighted folks, in the sense that it has the same effect. You would probably not want an English text to suddenly show " (öffnet in neuem Tab)" (the German equivalent) written somewhere on the screen. This is what is currently happening with that aria-label to the readers of my English blog. But because aria-label is an attribute whose language cannot be set individually, much less mid-phrase, users will hear this in their English synthesizer voice. I imagine this mix of languages would be just as ugly visually.

The same would be true if browsers suddenly started implementing this for the content authors somehow. I am not a Mac user, so don't know if that tool tip the screenshot shows would be spoken by VoiceOver while reading the page contents. But the fact remains that, even when I am normally reading something in German, if I read English text, I would expect something like "opens in a new tab" to be given to me in the same language, so my brain doesn't have to context-switch.

And as Andrea said, content accessibility is the content producer's responsibility, in this case WordPress. Browsers in this contract are making sure to translate everything in a way screen readers can understand, and screen readers then convey that information to the users who must of course know their part to get at all possible information.

I am OK with removing this particular broken implementation, but a replacement must come from WordPress, not user agents. What Apple are doing here is non-standard. And besides, it is annoying to hear that whole URL spoken for something like the notification that this will open in a new tab, so to me as a user, that particular announcement, if VoiceOver reads it, is more an annoyance than actually helpful.

As for WCAG standards: Informing users about the fact that a link will open in a new tab falls under success criterion 3.2, the web site must operate in predictable ways. So yes, this is WordPress responsibility.

@enriquesanchez
Copy link
Contributor

Will add this issue to the accessibility meeting agenda for this Friday.

@ellatrix
Copy link
Member Author

The "opens in a new browser tab" text needs to be in the same language of the post content. Browsers or operating systems don't analyze the page content, not to my knowledge at least.

@afercia I'm not saying that the browser should analyse the post content. I'm saying that the browser should use the language of the OS and browser. For example, my OS and browser language is English. If I open a web page in Japanese, the "open ... in a new tab" text will still be in English, because that's the language I set for the browser and it's a language I understand as a user of the internet. I'm also not the user agent should look at any lang attribute or page headers, which also changes from web page to web page.

The screenshot is a label that appears in Safari when you hover over a link and press Cmd or Shift+Cmd. I could imagine something like that being announced as well by a screen reader or properly labelled internally by the browser.

Screenshot 2019-11-26 at 20 13 36

In the screenshot above the pointer is over the link and Shift+Cmd is pressed.

@ellatrix
Copy link
Member Author

As I see it, producing accessible content is a WordPress responsibility.

As I asked before, do you have any resources stating this is the recommended practice for accessible content?

@ellatrix
Copy link
Member Author

You would probably not want an English text to suddenly show " (öffnet in neuem Tab)" (the German equivalent) written somewhere on the screen.

@MarcoZehe I would if the browser language is German. Again, I'm saying we should not look at the web page language at all.

Screenshot 2019-11-26 at 20 20 19

Here you can see that the directions in Safari are in English, even though the page is in German, because my browser and OS language is English.

And as Andrea said, content accessibility is the content producer's responsibility, in this case WordPress.

I don't fully agree with this. Browsers and screen readers provide a lot of built-in logic to enhance accessibility.

@MarcoZehe
Copy link
Contributor

You would probably not want an English text to suddenly show " (öffnet in neuem Tab)" (the German equivalent) written somewhere on the screen.

@MarcoZehe I would if the browser language is German. Again, I'm saying we should not look at the web page language at all.

inside the web content page? After all that is what we're talking about here.

Screenshot 2019-11-26 at 20 20 19

Here you can see that the directions in Safari are in English, even though the page is in German, because my browser and OS language is English.

I actually cannot, because I am blind, but I assume from your previous description that this is outside the actual web page content, somewhere in the browser chrome. Which means that it is indeed not part of the content area. And, if I might add, it is not very discoverable, IMO, if you have to press two modifier keys on hover to get this tooltip to appear.
Again, I think because the content provider, in this case WordPress on behalf of its author(s), is responsible for providing the content, and that should be language-consistent. So if the site language is set to English, that text inlined in the web page content should be in the same language.

And besides, WCAG criterium 3.2 (predictability) seems to agree with me with regards to the indicator for opening in a new tab.

@ellatrix
Copy link
Member Author

ellatrix commented Nov 26, 2019

@MarcoZehe The Safari behaviour is just something to illustrate what I have in mind for a screen reader to announce. I'm not saying it should use that exact same text, it could be be just that the link would open in a new tab or window if the the target attribute is set to _blank.

Let me rephrase why I think it would be better to have it built-in a browser or screen reader.

Even if we add "opens in a new tab" to every link on the page for WordPress pages, the rest of the pages that the user visits won't have this text. So while it might be of help for some pages, it's not a consistent experience at all.

Something at would make more sense to me is the screen reading detecting the target attribute and announcing that the link would open in a new language if it would be pressed. And what language is that? The language that the screen reader is set to, not the language of the web page. The language of the screen reader is not dependent on the language of the web page. No text would be added inside the web page.

For example, in VoiceOver, it could be announced something like "You are currently on a link that opens in a new tab".

@MarcoZehe
Copy link
Contributor

@ellatrix The question is really one of "What's good for one group is most likely good for many more". So if we go with a new tab indicator for all sites that have the target attribute on links, it has to come through the user agent, and be more than just a screen reader thing. It needs to be a clear visual indicator for everybody. So the last thing we want is for the screen reader to start interpreting the HTML. That would take us back to the dark ages of the early 2000s and IE only, where screen readers all had to interpret HTML their own way, and interop was a nightmare.

Having said that, even the current approach in Gutenberg, which this pull request will indeed remedy, is not a good solution because aria-label only is for assistive technologies, and mostly screen readers. Sighted people are left out. But I know many to whom this would mean a serious cognitive challenge without prior knowledge. That's why many web authors have so far adopted putting that text right into the link as well so everybody benefits. And that would, of course, be in the language of the other text around it, not the language the CMS was set to by chance when the content was created. Which takes us back to my original issue in #18727 .

So while I agree in principle that it should be a default indicator for everyone, and I'll actually propose this to Firefox product, it would still mean an inconsistent behavior if Chromium (and that also means Edge) decide to not follow suit. On the other hand, if WordPress would start indicating this clearly and visually by default, it would mean that at least a third of websites on the net today would get this indicator in all user agents.

@ellatrix
Copy link
Member Author

Right, adding the text is a fairly easy solution with some immediate benefits. User agents adopting it would be good as well, but as you say, neither solution will be complete across the web and all browsers immediately. Either it's fixed for all WP sites, or it's fixed for one browser. I still believe it would be good to address something like this on the browser level. I see this a bit as a browser user interface, giving directions to the user about the page and navigation. But I can also see how we can make a dent with WordPress.

So while I agree in principle that it should be a default indicator for everyone, and I'll actually propose this to Firefox product

That's great to hear!

So the last thing we want is for the screen reader to start interpreting the HTML.

Yeah, without a specification or best practise it's hard. That's why I'm asking for a resource specifically saying that appending the text "opens in a new tab" is the best practice. It's worth noting that this is something fairly new that we're doing. We never added this text in the classic editor, or on the front end.

@afercia
Copy link
Contributor

afercia commented Nov 27, 2019

As I see it, producing accessible content is a WordPress responsibility.

As I asked before, do you have any resources stating this is the recommended practice for accessible content?

There are a number of suggested techniques published by the W3C that everyone can easily find by googling a bit. For example:

G201: Giving users advanced warning when opening a new window
https://www.w3.org/TR/WCAG20-TECHS/G201.html

H83: Using the target attribute to open a new window on user request and indicating this in link text
https://www.w3.org/TR/WCAG20-TECHS/H83.html

Overall, I'm not sure I understand your objections. WordPress should aim to produce accessible, semantic, content regardless of any W3C recommendation or legal requirement. WordPress can do that by providing tools and educating users. Relying on non-existing-yet browsers or assistive technologies features isn't a solution.

Historically, WordPress also aims to educate users to best practices to produce semantic, accessible, content. For a number of years WordPress has been discouraging the use of target="_blank" and should educating users that they should stop taking control of the users browser. We're doing it in the admin since a few years. Gutenberg aims to improve content production thus it would be great to see the best practices we adopted for the admin be implemented also for the front end. Gutenberg offers a good opportunity to do that, and that's the reason why this feature was originally implemented.

That said, the accessibility team expressed some concerns about the implementation details. Not sure an aria-label is the best option. As pointed out in other comments, some plain text would be preferable. Also, sighted users would need a visual indication.

Regarding language identification, I'm really not sure I understand. Why this text should be based on the operating system language? That seems to me a big assumption. Seems pretty obvious to me that it should use the post content language instead. Also, it should be editable as any other content thus Gutenberg should provide a UI to edit this text.

@enriquesanchez
Copy link
Contributor

View #4583 for historical context on why this setting exists: (thanks @karmatosed for uncovering this!).

@karmatosed
Copy link
Member

I have gone on a bit of a dive back through the issue linked be @enriquesanchez and thought about this. I want to propose a few optional options and then say which I think could work here, finding a balance.

Before I do that, it's important to note the cognitive load of visually adding a repeating piece of text to a page. This can cause visual rivers along with just being a confusing visual if someone has a lot of links. I understand why suggested, but it's worth pointing out the idea initially was to have it as text which could be spoken, not visually on all the time.

That said, here are my suggestions:

  • Remove the option to open in a new window. This feels potentially something people won't want; personally, I would be up for that. It's worth considering the number of comments and requests, though for it.
  • Add an option to 'turn on text'.
  • Add an option to 'turn on open in new window' that then notes it will turn on text.

I like the last option. This would work by under options have a setting that would both add the toggle today and also automatically the text if the toggle is used. In clicking this on you would know what doing and it also encourages conscious thinking.

Why turn on? Ideally, this option isn't something encouraged; however, some want to use it. By having off by default, it would be there, but you have to decide to use it.

@enriquesanchez
Copy link
Contributor

Thanks for your feedback @karmatosed! You make great points and suggestions.

After going over the historical context for this setting and reviewing the WCAG recommendations, I don't think it should be removed.

I believe there's middle ground to be found in which:

  • all users (not just screen reader users) are alerted when a link will open in a new tab
  • this alert is non-obtrusive to users who are sensitive to content overload and repetition

I wonder if there's a way we can improve upon the already common practice of adding an icon + tooltip to the link that opens in a new tab. I found this example on Inclusive Components that looks promising.

I think that an approach like the one described on Inclusive Components will check both requirements: it'll alert all users (not just screen reader users) while the visual cue is subtle. The tooltip that appears on focus and hover rounds it all up and makes it more clear and accessible.

@MarcoZehe @afercia what do you think about the Inclusive Components example?

Also @ellatrix, apologies for diverging this issue from its original intent, which was to remove the broken aria-label implementation. Would you prefer if I create another issue to discuss a new implementation proposal?

@MarcoZehe
Copy link
Contributor

I am all behind the Inclusive Components example. Heydon's book should be a must-read at any web development job. :-) So yes, I would vote for this.

@melchoyce
Copy link
Contributor

melchoyce commented Dec 13, 2019

Could we use an inline SVG of https://developer.wordpress.org/resource/dashicons/#external instead of text?

Edit: Haha I see @enriquesanchez already suggested this. It's a +1 for me.

@enriquesanchez
Copy link
Contributor

Thanks for the feedback @MarcoZehe!

And looks like we are already use the inline SVG that @melchoyce suggests, in the URL popover:

Screen Shot 2019-12-13 at 18 51 11

(Image above shows a screenshot of the URL popover of a link in a paragraph block)

If we were to follow the Inclusive Components example, we would just need to add the accessible tooltip.

@mapk
Copy link
Contributor

mapk commented Dec 17, 2019

Would you prefer if I create another issue to discuss a new implementation proposal?

Sounds like a good idea, @enriquesanchez, if this isn't being worked on as part of this PR.

After going over the historical context for this setting and reviewing the WCAG recommendations, I don't think it should be removed.

I so happy about this! +1 to NOT removing it. Sounds like this is getting wrapped up with a good direction forward. 👍

@ellatrix
Copy link
Member Author

I'm confused about what the next steps are here.

@enriquesanchez
Copy link
Contributor

enriquesanchez commented Jan 28, 2020

Hi @ellatrix!

I think that the original intent of removing the broken aria-label implementation should move forward, as everyone agreed.

There's also a proposal for a new implementation (#18742 (comment)) that follows best practices, and a few of us gave it a thumbs up.

Would you like me to create a new issue to address this new implementation proposal?

@ellatrix
Copy link
Member Author

@enriquesanchez Yes please! 😄 It's not exactly clear to me what should be done regarding the UI. In any case, I think we should remove the label before the 5.4 release.

@youknowriad
Copy link
Contributor

Let's update this PR to remove the label for now then.

@enriquesanchez
Copy link
Contributor

@ellatrix @mapk

I created #20194 to address the new implementation.

@ellatrix ellatrix force-pushed the remove/link-aria-label branch from 5b7b28f to f562ab2 Compare February 13, 2020 11:32
@ellatrix
Copy link
Member Author

I rebased this PR and simplified it to only remove the aria-label attribute. Let's try to get this in for WordPress 5.4 to avoid further damage.

@ellatrix ellatrix added this to the Gutenberg 7.6 milestone Feb 13, 2020
@youknowriad youknowriad added the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Feb 13, 2020
@ellatrix
Copy link
Member Author

Thanks @youknowriad!

@ellatrix ellatrix merged commit c84784f into master Feb 13, 2020
@ellatrix ellatrix deleted the remove/link-aria-label branch February 13, 2020 12:32
jorgefilipecosta pushed a commit that referenced this pull request Feb 17, 2020
* Format library: link: remove aria-label

* Adjust new e2e snap
jorgefilipecosta pushed a commit that referenced this pull request Feb 17, 2020
* Format library: link: remove aria-label

* Adjust new e2e snap
@jorgefilipecosta jorgefilipecosta removed the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Feb 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Accessibility Feedback Need input from accessibility [Package] Format library /packages/format-library [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Format Library: Link aria-label is assigned wrong text
10 participants