Improve the link preview accessibility and labels. #60908
Conversation
afercia
added
[Type] Bug
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Size Change: 0 B Total Size: 1.75 MB ℹ️ View Unchanged
|
| __( 'Copy link%s' ), // Ends up looking like "Copy link: https://example.com". | ||
| isEmptyURL || showIconLabels ? '' : ': ' + value.url |
There was a problem hiding this comment.
This was added because users specifically wanted a way to view the full URL without having to edit it.
For visually sighted users the URL is clipped when it overflows and thus users requested a way to see the full URL, even if that requires them hovering over a button.
What's the rationale for removing this? Perhaps it's this:
Tooltips are pointless when their text is already visible within the control.
Which seems true until you realise that the full URL is visually clipped. We don't want to undo that visual clipping (it's been iterated on several times) and so this was a good way to retain UX for sighted users.
There was a problem hiding this comment.
It's not only a visual thing. You are missing the point that before being the tooltip, this is the aria-label of the button.
The aria-label provides the accessible name of a control. A name control is not the place for values or states. It just has to give the control a name that tells users what the control does.
This is basically the equivalent of the button text, would you ever provide users with a HTML button like the following one?
If we want to provide users the ability to see the full URL, then the tooltip is not the right place for that. I also want to mention that these are simple, basic, accessibility principles we've been repeating sinc eyears in this project but it seems designers still don't get the point.
We can't use tooltips this way. Tooltips were introduced in Gutenberg with the only specific purpose to visually expose an icon button name. They must not be used for values, states, or descriptions.
There was a problem hiding this comment.
What I've tried to do is provide you with context you are missing regarding a UX requirement of being able to perceive the entire link. If that context doesn't currently marry with best practices then it would be nice if we could work together collaboratively and constructively to devise a more suitable solution for the benefit of users.
Do you have any suggestions for how we could achieve this?
I also want to mention that these are simple, basic, accessibility principles we've been repeating sinc eyears in this project
I appreciate your frustration regarding accessibility principles. I looked at the component docs and noted the lack of documentation around this component and how it should be used:
https://wordpress.github.io/gutenberg/?path=/docs/components-tooltip--docs
Given that this something you find yourself repeating, I think we could work together to improve the documentation and add specific notes on accessibility linking out to resources where appropriate.
I'll hold my hand up and accept that I could be more informed about the specifics of a11y regarding tooltips, but I strongly believe that if we want consumers (especially 3rd party, non-Core team developers) to follow best practices we should be looking at providing documentation alongside the component.
That requires a collaborative effort and I'd be more than happy to work alongside you on that if you are open to it. If not then I will do it myself.
...but it seems designers still don't get the point.
I'm not a designer but I think it's disingenuous to paint designers in this light. Mistakes happen, it's the will to correct and improve both the code and one's own knowledge that is key. Providing documentation is also a great way to share the knowledge of individual contributors and make it more readily available to others.
There was a problem hiding this comment.
For avoidance of confusion here's an example of where being able to see the entire URL is needed. Is this linking to the correct PDF? How do I know?
Again, I'm not advocating we continue to use a pattern that provides a poor experience for users of assistive tech but I feel we owe it to our users to find a solution that accounts for this requirement before we remove it from the UI.
| aria-label={ __( 'Currently selected' ) } | ||
| role="group" | ||
| aria-label={ __( 'Manage link' ) } |
There was a problem hiding this comment.
Can you help me to understand why you feel the word manage is more appropriate here?
This is a preview of the currently selected link so that's why it's named as such.
There was a problem hiding this comment.
@getdave, did you read the issue before asking your question? I think I already explained there the reasoning.
There was a problem hiding this comment.
Yes I read it.
However I did miss the fact that you'd wrapped the entire LinkPreview component in a Manage link group. That was an oversight on my part which I was happy to correct.
| ref={ ref } | ||
| disabled={ isEmptyURL } | ||
| size="compact" | ||
| showTooltip={ ! showIconLabels } |
There was a problem hiding this comment.
It is extremely annoying that contributors to this project keep missing this. That also proves there is a lack of manual testing and no one tests the UI with the 'Show button text labels' preference enabled, which is less than ideal.
There was a problem hiding this comment.
It is extremely annoying that contributors to this project keep missing this.
Again the documentation makes no mention of this. We don't have to make it wordpress specific but mentioning when this property should / shouldn't be applied would help avoid contributors needing to monitor for further reoccurrences.
https://wordpress.github.io/gutenberg/?path=/docs/components-button--docs
That also proves there is a lack of manual testing
I don't think it's helpful to draw such conclusions from a single PR review instance.
In this case I didn't perform manual testing as it was a first pass review. Happy to do so once the issue of preserving the ability to perceive the full URL from the preview has been addressed.
...and no one tests the UI with the 'Show button text labels' preference enabled
I find it's usually best to avoid generalisations such as this. Indeed, whilst I won't name them, I can think of many contributors (including designers) who do this on a regular basis.
afercia
force-pushed
the
fix/link-control-preview-accessibility
branch
from
fc6d75c
to
b01fcc2
Compare
| aria-label={ __( 'Currently selected' ) } | ||
| role="group" | ||
| aria-label={ __( 'Manage link' ) } |
There was a problem hiding this comment.
Yes I read it.
However I did miss the fact that you'd wrapped the entire LinkPreview component in a Manage link group. That was an oversight on my part which I was happy to correct.
| __( 'Copy link%s' ), // Ends up looking like "Copy link: https://example.com". | ||
| isEmptyURL || showIconLabels ? '' : ': ' + value.url |
There was a problem hiding this comment.
What I've tried to do is provide you with context you are missing regarding a UX requirement of being able to perceive the entire link. If that context doesn't currently marry with best practices then it would be nice if we could work together collaboratively and constructively to devise a more suitable solution for the benefit of users.
Do you have any suggestions for how we could achieve this?
I also want to mention that these are simple, basic, accessibility principles we've been repeating sinc eyears in this project
I appreciate your frustration regarding accessibility principles. I looked at the component docs and noted the lack of documentation around this component and how it should be used:
https://wordpress.github.io/gutenberg/?path=/docs/components-tooltip--docs
Given that this something you find yourself repeating, I think we could work together to improve the documentation and add specific notes on accessibility linking out to resources where appropriate.
I'll hold my hand up and accept that I could be more informed about the specifics of a11y regarding tooltips, but I strongly believe that if we want consumers (especially 3rd party, non-Core team developers) to follow best practices we should be looking at providing documentation alongside the component.
That requires a collaborative effort and I'd be more than happy to work alongside you on that if you are open to it. If not then I will do it myself.
...but it seems designers still don't get the point.
I'm not a designer but I think it's disingenuous to paint designers in this light. Mistakes happen, it's the will to correct and improve both the code and one's own knowledge that is key. Providing documentation is also a great way to share the knowledge of individual contributors and make it more readily available to others.
| ref={ ref } | ||
| disabled={ isEmptyURL } | ||
| size="compact" | ||
| showTooltip={ ! showIconLabels } |
There was a problem hiding this comment.
It is extremely annoying that contributors to this project keep missing this.
Again the documentation makes no mention of this. We don't have to make it wordpress specific but mentioning when this property should / shouldn't be applied would help avoid contributors needing to monitor for further reoccurrences.
https://wordpress.github.io/gutenberg/?path=/docs/components-button--docs
That also proves there is a lack of manual testing
I don't think it's helpful to draw such conclusions from a single PR review instance.
In this case I didn't perform manual testing as it was a first pass review. Happy to do so once the issue of preserving the ability to perceive the full URL from the preview has been addressed.
...and no one tests the UI with the 'Show button text labels' preference enabled
I find it's usually best to avoid generalisations such as this. Indeed, whilst I won't name them, I can think of many contributors (including designers) who do this on a regular basis.
| @@ -365,7 +365,7 @@ describe( 'Basic rendering', () => { | |||
| /> | |||
| ); | |||
|
|
|||
| const linkPreview = screen.getByLabelText( 'Currently selected' ); | |||
| const linkPreview = screen.getByLabelText( 'Manage link' ); | |||
There was a problem hiding this comment.
We should prefer the getByRole selector. This is documented in the guide for Playwright and we should aim to abide by this when writing tests.
| const linkPreview = screen.getByLabelText( 'Manage link' ); | |
| const linkPreview = screen.getByRole( 'group', { | |
| name: 'Manage link', | |
| } ); |
There are other occurrences within this file that also need fixing to utilise the new roles you've added.
There was a problem hiding this comment.
I'd agree usint the role selector is better, but this is already existing code, I was just changing the label value.
| __( 'Copy link%s' ), // Ends up looking like "Copy link: https://example.com". | ||
| isEmptyURL || showIconLabels ? '' : ': ' + value.url |
There was a problem hiding this comment.
For avoidance of confusion here's an example of where being able to see the entire URL is needed. Is this linking to the correct PDF? How do I know?
Again, I'm not advocating we continue to use a pattern that provides a poor experience for users of assistive tech but I feel we owe it to our users to find a solution that accounts for this requirement before we remove it from the UI.
|
Re: Documentation: this is more about the The documentation clearly states the Quick history of Tooltips in GutenbergI'll try to provide some more context about the original implementation of Tooltips, hoping it's a good base for some better documentation. Suggested in the initial accessibility recommendations for Gutenberg on Mar 21, 2017. See point 8. on this issue:
Noting they were specifically requested for icon buttons. Issue regarding implementing Tooltips, created on Apr 27, 2017: Block controls: Show tooltip on hover, keyboard focus First implementation of Tooltips was made in this PR on Aug 8, 2017: Components: Add Tooltip component, display button aria-label tooltips Noting the PR title itself mentions the purpose to 'display button aria-label'. |
I'm sorry if that sounded unfair. However, it's not just a single case. In the last years I've been observing countless changes to the editor UI that didn't take into consideration the 'Show button text labels' preference at all, thuys introducing breakages in the UI. It's clearly an editor feature that is under-tested and often forgot in many cases. I had to submit a few PRs in the past to fix those cases but unfortunately that happens way after the breakage was merged and released. |
|
To better clarify the current situation on trunk. The whole preview popover is wrapped within an element labeled To illustrate visually: all the UI within the highlighted area in the screenshot below is labeled
The UI is actually made of multiple things:
Semantically, this section of content is a
Setting an The solution is to either set an explicit ARIA role or remove the aria-label. However, removing the aria-label would let users without any clue what this popover is about. I tend to think an explicit About the value of the aria-label, I'm not sure 'Manage' is the best term to use. Considering the diverse nature of the popover content I can't think of a better name. As a last note: the 'link preview' may or may not be an actual 'preview'. First of all, it doesn't preview the 'link', rather it previews the 'destination resource' of the link, in most of the cases a web page. However, when the destination can't be
|
|
Re: the design issue and hte need for users to see the full URL. Long URLs have alwasy been a problem. There's no optimal solution for that. However, usitn the tooltip is far from ideal:
Long URL shown within the Copy button tooltip:
Very very long URL shown within the Copy button tooltip:
If the full URL is considered important information to provide to users (and I'd agree it is) then it should be shown with visibel text in the UI. Howeer, very long URLs are a problem. It's not a new problem though. It has been discussed at length and addressed previously in Core, for hte wp-link toolbar for TinyMCE. While truncating text isn't ideal, at some point very long URLs need to be truncated. wplink tries to do that in a smart way and I'd suggest to just copy what WordPress has been doing for years to display long URLs in the link 'preview'. The implementation is here: https://github.com/WordPress/wordpress-develop/blob/d898a25d1a37f493b7050b263cb9c5b29565792a/src/js/_enqueues/vendor/tinymce/plugins/wplink/plugin.js#L11-L51
Besides this JavaScript part, the CSS truncates the URL anyways, this is the CSS used on Classic editor for the Since this hae been the implementation in Core for years and it worked reasonably, I'd suggest to just use a similar implementation. As said, there's no perfect solution to display very long URLs. I will create a separate issue. Screenshots from Classic editor link 'preview':
|
afercia
force-pushed
the
fix/link-control-preview-accessibility
branch
from
b01fcc2
to
7fda3bd
Compare
|
I created a new issue #61158 to explore a new way to display long URLs. I'd appreciate a new review of this PR, thanks everyone. |
afercia
force-pushed
the
fix/link-control-preview-accessibility
branch
from
7fda3bd
to
2593614
Compare
Fixes #60901
What?
Why?
How?
I'd appreciate some specific accessibility feedback about the roles and labels.
Cc @joedolson @alexstine
Testing Instructions
divelement with rolegroupand labelManage link.spanelement with rolefigureand labelLink information.Testing Instructions for Keyboard
Screenshots or screencast