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.

Sign up for GitHub

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

Improve the link preview accessibility and labels. #60908

Merged
merged 3 commits into from
Oct 28, 2024
Merged

Improve the link preview accessibility and labels. #60908

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
28a0da9
Improve the link preview accessibility and labels.
afercia Apr 19, 2024
a0ce5cd
Adjust test.
afercia Apr 22, 2024
c737c13
Update test by using ByRole queries.
afercia Apr 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 15 additions & 8 deletions packages/block-editor/src/components/link-control/link-preview.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import clsx from 'clsx';
/**
* WordPress dependencies
*/
import { __, sprintf } from '@wordpress/i18n';
import { __ } from '@wordpress/i18n';
import {
Button,
ExternalLink,
Expand Down Expand Up @@ -96,7 +96,8 @@ export default function LinkPreview( {

return (
<div
aria-label={ __( 'Currently selected' ) }
role="group"
aria-label={ __( 'Manage link' ) }
getdave marked this conversation as resolved.
Show resolved Hide resolved
Comment on lines +99 to +100
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Currently selected is quite old code now and I'm really pleased to get input on improving this 👍

className={ clsx( 'block-editor-link-control__search-item', {
'is-current': true,
'is-rich': hasRichData,
Expand All @@ -107,7 +108,14 @@ export default function LinkPreview( {
} ) }
>
<div className="block-editor-link-control__search-item-top">
<span className="block-editor-link-control__search-item-header">
<span
className="block-editor-link-control__search-item-header"
role="figure"
aria-label={
/* translators: Accessibility text for the link preview when editing a link. */
__( 'Link information' )
}
Comment on lines +114 to +117
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a good option 👍

>
<span
className={ clsx(
'block-editor-link-control__search-item-icon',
Expand Down Expand Up @@ -149,26 +157,25 @@ export default function LinkPreview( {
label={ __( 'Edit link' ) }
onClick={ onEditClick }
size="compact"
showTooltip={ ! showIconLabels }
/>
{ hasUnlinkControl && (
<Button
icon={ linkOff }
label={ __( 'Remove link' ) }
onClick={ onRemove }
size="compact"
showTooltip={ ! showIconLabels }
/>
) }
<Button
icon={ copySmall }
label={ sprintf(
// Translators: %s is a placeholder for the link URL and an optional colon, (if a Link URL is present).
__( 'Copy link%s' ), // Ends up looking like "Copy link: https://example.com".
isEmptyURL || showIconLabels ? '' : ': ' + value.url
Comment on lines -165 to -166
Copy link
Contributor

@getdave getdave Apr 22, 2024 •
edited by draganescu
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was added because users specifically wanted a way to view the full URL without having to edit it.

For 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Screenshot 2024-04-23 at 09 26 51

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Screen Shot 2024-04-23 at 10 56 40

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to reiterate my concerns above regarding the tooltip providing important UX for sighted users.

I've re-read the thread and acknowledhge that Copy link: {{FULL URL HERE}} is not helpful as an accessible name.

Screenshot of the long name for context Screen Shot 2024-10-24 at 12 32 43

That said whilst fixing this for one set of users, we should avoid introducing a regression for another set.

Therefore I wonder if it's possible to have a tooltip attached to the button which isn't used as the name for the button but which is perceivable by all users if they choose? What a11y roles or semantics could be used to achieve this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see you are exploring in #61158. I'll check that too.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've re-tested the implementation on this PR and I've changed my opinion. Having a very long URL in a tooltip doesn't offer a huge amount of value. It is also detrimental to the UX for a large number of users.

With the advancements to the Link UI in WordPress 6.5 it's now much easier to just click "Edit" and see the full URL in the form input field. Prior to those changes it was necessary to click several buttons to achieve the same thing.

So overall I'd be in favour of this change.

) }
label={ __( 'Copy link' ) }
ref={ ref }
accessibleWhenDisabled
disabled={ isEmptyURL }
size="compact"
showTooltip={ ! showIconLabels }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for spotting this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd recommend we be proactive here and raise another PR to update the documentation for this prop to add some guidance about it's effective usage. We could explain the a11y best practices that surround this and help to guide future contributors.

Any other components that make use of this prop could also be covered.

I wonder what @ciampo thinks about improving the guidance around relying on icons/tooltips in the component library?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better documentation is always a win!

I should also mention that it was recently agreed (see #65989 and #66021) that Tooltips should be purely visual/decorative, and therefore should not be intended as a tool to add accessible descriptions to their anchors.

The anchor should be self-sufficient when it comes to their accessible label / description — ie. they should either have accessible content, or use the aria-label / aria-labelledby / aria-describedby attributes accordingly.

I can look into updating Button's and Tooltip's documentation accordingly.

/>
<ViewerSlot fillProps={ value } />
</div>
Expand Down
87 changes: 63 additions & 24 deletions packages/block-editor/src/components/link-control/test/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -373,7 +373,9 @@ describe( 'Basic rendering', () => {
/>
);

const linkPreview = screen.getByLabelText( 'Currently selected' );
const linkPreview = screen.getByRole( 'group', {
name: 'Manage link',
} );

const isPreviewError = linkPreview.classList.contains( 'is-error' );
expect( isPreviewError ).toBe( true );
Expand Down Expand Up @@ -834,7 +836,9 @@ describe( 'Manual link entry', () => {

render( <LinkControlConsumer /> );

let linkPreview = screen.getByLabelText( 'Currently selected' );
let linkPreview = screen.getByRole( 'group', {
name: 'Manage link',
} );

expect( linkPreview ).toBeInTheDocument();

Expand Down Expand Up @@ -868,7 +872,9 @@ describe( 'Manual link entry', () => {
// Cancel the editing process.
await user.click( cancelButton );

linkPreview = screen.getByLabelText( 'Currently selected' );
linkPreview = screen.getByRole( 'group', {
name: 'Manage link',
} );

expect( linkPreview ).toBeInTheDocument();

Expand Down Expand Up @@ -1076,7 +1082,9 @@ describe( 'Default search suggestions', () => {

// Click the "Edit/Change" button and check initial suggestions are not
// shown.
const currentLinkUI = screen.getByLabelText( 'Currently selected' );
const currentLinkUI = screen.getByRole( 'group', {
name: 'Manage link',
} );
const currentLinkBtn = within( currentLinkUI ).getByRole( 'button', {
name: 'Edit link',
} );
Expand Down Expand Up @@ -1230,8 +1238,9 @@ describe( 'Creating Entities (eg: Posts, Pages)', () => {

// Check for loading indicator.
const loadingIndicator = screen.getByText( 'Creating…' );
const currentLinkLabel =
screen.queryByLabelText( 'Currently selected' );
const currentLinkLabel = screen.queryByRole( 'group', {
name: 'Manage link',
} );

expect( currentLinkLabel ).not.toBeInTheDocument();
expect( loadingIndicator ).toBeVisible();
Expand All @@ -1242,8 +1251,9 @@ describe( 'Creating Entities (eg: Posts, Pages)', () => {
// Resolve the `createSuggestion` promise.
resolver();

const currentLink =
await screen.findByLabelText( 'Currently selected' );
const currentLink = await screen.findByRole( 'group', {
name: 'Manage link',
} );

expect( currentLink ).toHaveTextContent( entityNameText );
expect( currentLink ).toHaveTextContent( '/?p=123' );
Expand Down Expand Up @@ -1291,7 +1301,9 @@ describe( 'Creating Entities (eg: Posts, Pages)', () => {

await user.click( createButton );

const currentLink = screen.getByLabelText( 'Currently selected' );
const currentLink = screen.getByRole( 'group', {
name: 'Manage link',
} );

expect( currentLink ).toHaveTextContent( 'Some new page to create' );
expect( currentLink ).toHaveTextContent( '/?p=123' );
Expand Down Expand Up @@ -1350,7 +1362,9 @@ describe( 'Creating Entities (eg: Posts, Pages)', () => {
triggerEnter( searchInput );

expect(
await screen.findByLabelText( 'Currently selected' )
await screen.findByRole( 'group', {
name: 'Manage link',
} )
).toHaveTextContent( entityNameText );
} );

Expand Down Expand Up @@ -1529,7 +1543,9 @@ describe( 'Selecting links', () => {

render( <LinkControlConsumer /> );

const currentLink = screen.getByLabelText( 'Currently selected' );
const currentLink = screen.getByRole( 'group', {
name: 'Manage link',
} );
const currentLinkAnchor = screen.getByRole( 'link', {
name: `${ selectedLink.title } (opens in a new tab)`,
} );
Expand Down Expand Up @@ -1559,7 +1575,9 @@ describe( 'Selecting links', () => {
render( <LinkControlConsumer /> );

// Required in order to select the button below.
let currentLinkUI = screen.getByLabelText( 'Currently selected' );
let currentLinkUI = screen.getByRole( 'group', {
name: 'Manage link',
} );
const currentLinkBtn = within( currentLinkUI ).getByRole( 'button', {
name: 'Edit link',
} );
Expand All @@ -1570,7 +1588,9 @@ describe( 'Selecting links', () => {
const searchInput = screen.getByRole( 'combobox', {
name: 'Search or type URL',
} );
currentLinkUI = screen.queryByLabelText( 'Currently selected' );
currentLinkUI = screen.queryByRole( 'group', {
name: 'Manage link',
} );

// We should be back to showing the search input.
expect( searchInput ).toBeVisible();
Expand Down Expand Up @@ -1733,8 +1753,9 @@ describe( 'Selecting links', () => {
triggerEnter( searchInput );

// Check that the suggestion selected via is now shown as selected.
const currentLink =
screen.getByLabelText( 'Currently selected' );
const currentLink = screen.getByRole( 'group', {
name: 'Manage link',
} );
const currentLinkAnchor = screen.getByRole( 'link', {
name: `${ selectedLink.title } (opens in a new tab)`,
} );
Expand Down Expand Up @@ -2127,7 +2148,9 @@ describe( 'Rich link previews', () => {

render( <LinkControl value={ selectedLink } /> );

const linkPreview = screen.getByLabelText( 'Currently selected' );
const linkPreview = screen.getByRole( 'group', {
name: 'Manage link',
} );

const isRichLinkPreview = linkPreview.classList.contains( 'is-rich' );

Expand All @@ -2148,7 +2171,9 @@ describe( 'Rich link previews', () => {

render( <LinkControl value={ selectedLink } hasRichPreviews /> );

const linkPreview = screen.getByLabelText( 'Currently selected' );
const linkPreview = screen.getByRole( 'group', {
name: 'Manage link',
} );

await waitFor( () => expect( linkPreview ).toHaveClass( 'is-rich' ) );
} );
Expand All @@ -2165,7 +2190,9 @@ describe( 'Rich link previews', () => {

render( <LinkControl value={ selectedLink } hasRichPreviews /> );

const linkPreview = screen.getByLabelText( 'Currently selected' );
const linkPreview = screen.getByRole( 'group', {
name: 'Manage link',
} );

await waitFor( () => expect( linkPreview ).toHaveClass( 'is-rich' ) );

Expand Down Expand Up @@ -2197,7 +2224,9 @@ describe( 'Rich link previews', () => {

render( <LinkControl value={ selectedLink } hasRichPreviews /> );

const linkPreview = screen.getByLabelText( 'Currently selected' );
const linkPreview = screen.getByRole( 'group', {
name: 'Manage link',
} );

await waitFor( () => expect( linkPreview ).toHaveClass( 'is-rich' ) );

Expand All @@ -2221,7 +2250,9 @@ describe( 'Rich link previews', () => {

render( <LinkControl value={ selectedLink } hasRichPreviews /> );

const linkPreview = screen.getByLabelText( 'Currently selected' );
const linkPreview = screen.getByRole( 'group', {
name: 'Manage link',
} );

await waitFor( () => expect( linkPreview ).toHaveClass( 'is-rich' ) );

Expand Down Expand Up @@ -2256,7 +2287,9 @@ describe( 'Rich link previews', () => {

render( <LinkControl value={ selectedLink } hasRichPreviews /> );

const linkPreview = screen.getByLabelText( 'Currently selected' );
const linkPreview = screen.getByRole( 'group', {
name: 'Manage link',
} );

await waitFor( () =>
expect( linkPreview ).toHaveClass( 'is-rich' )
Expand All @@ -2281,7 +2314,9 @@ describe( 'Rich link previews', () => {

render( <LinkControl value={ selectedLink } hasRichPreviews /> );

const linkPreview = screen.getByLabelText( 'Currently selected' );
const linkPreview = screen.getByRole( 'group', {
name: 'Manage link',
} );

expect( linkPreview ).toHaveClass( 'is-fetching' );

Expand All @@ -2300,7 +2335,9 @@ describe( 'Rich link previews', () => {

render( <LinkControl value={ selectedLink } hasRichPreviews /> );

const linkPreview = screen.getByLabelText( 'Currently selected' );
const linkPreview = screen.getByRole( 'group', {
name: 'Manage link',
} );

expect( linkPreview ).toHaveClass( 'is-fetching' );
expect( linkPreview ).not.toHaveClass( 'is-rich' );
Expand All @@ -2313,7 +2350,9 @@ describe( 'Rich link previews', () => {

render( <LinkControl value={ selectedLink } hasRichPreviews /> );

const linkPreview = screen.getByLabelText( 'Currently selected' );
const linkPreview = screen.getByRole( 'group', {
name: 'Manage link',
} );

expect( linkPreview ).toHaveClass( 'is-fetching' );

Expand Down
Loading