Tooltip: add aria-describedby to anchor only if not redundant

[ciampo]

What?

Related to #65888

This PR makes changes to the Tooltip component so that the aria-describedby attribute gets added more conservatively

Why?

As discussed in #65888, the way Tooltip adds an aria-describedby attribute can disrupt assistive technology users:

• the tooltip is displayed dynamically, which means that the accessible description is only available when the tooltip is visible;
• the tooltip's description can override an existing description;
• the tooltip's description can be unnecessary if it's a copy of the accessible label;
• in general, tooltips should not be a mean of adding an accessible description (although this drawback is not in the scope of this PR)

How?

Implementing @t-hamano 's suggestion: the Tooltip component checks that there isn't a aria-describedby attribute already defined, and that the tooltip text is different from the anchor's accessible label.

Testing Instructions

• Make sure that a tooltip anchor has an explicit aria-describedby attribute. Inspecting the anchor, make sure that the aria-describedby attribute doesn't change when the tooltip is shown.
• Make sure that a tooltip anchor has an explicit aria-label attribute, and that the tooltip text is the same as the anchor's aria-label. Inspecting the anchor, make sure that the aria-describedby attribute is not added to the anchor when the tooltip is shown.

I've added a couple of temporary Storybook examples to help with testing in isolation.

In the editor, you can test this by:

• inserting a block
• moving focus to the block toolbar
• inspect the button responsible for triggering the "options" dropdown (the last one in the toolbar)
• notice how, with the changes from this PR applied, when the button receives focus and the tooltip shows, the button does not get an aria-describedby attribute added for the tooltip

Screenshots or screencast

Kapture.2024-10-09.at.16.31.29.mp4

[tyxla]

Code-wise this looks good to me 👍 I think it's a good compromise, considering we'll seek a better long-term fix.

Leaving the potential approval to @afercia after confirming it resolves the problem.

[afercia]

Thanks for working at this fix. I don't have bandwidth to test it now but as long as:

• It prevents the double announcement of label and description that have the same value.
• It prevents from dynamically updating existing aria-describedby value

then I'd think it's a reasonable short-term solution until the root problem gets solved. Thanks!

[ciampo]

I'll add a couple of unit tests and temporary storybook examples to validate the thesis further

[github-actions]

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 props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: ciampo <[email protected]>
Co-authored-by: tyxla <[email protected]>
Co-authored-by: t-hamano <[email protected]>
Co-authored-by: afercia <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[ciampo]

Added unit tests, a couple of temporary Storybook examples, and updated PR description.

Let's give this PR a round of review and merge if it improves the current situation for the 6.7 release.

[t-hamano]

LGTM!

I tested screen readers primarily on the post editor and site editor, and there were no issues with double reading of labels or inappropriate reading of descriptions.

Here is an example of that test:

Block Toolbar

block-toolbar.mp4

Header and Sidebar

2d9840e9931d4d7b8d9d3cc9fd47f0e7.mp4

[ciampo]

Thank you for the reviews!

Enabled auto-merging + backporting to WordPress 6.7 release

[github-actions]

I just cherry-picked this PR to the wp/6.7 branch to get it included in the next release: 1fdd5d9

[DaniGuardiola]

Thanks @ciampo! We should look into removing the patch next, which I believe you created a draft PR for somewhere else. I will look at everything once I'm back from sick leave.

[ciampo]

We should look into removing the patch next, which I believe you created a draft PR for somewhere else. I will look at everything once I'm back from sick leave.

Yup, see #66021