Block Toolbar: Screen readers do not correctly read descriptions of focused elements

[t-hamano]

Description

Issues discovered in this comment: #61168 (comment)

For example, NVDA reads the element's label, role, and description when an element is focused. However, for focusable elements in the block toolbar, it does speak the description of the previously focused element, and in my testing, it appears to speak the focused element's label twice.

Users who use screen readers will be very confused because the read-out label and description do not match.

According to this comment, it seems that this issue first appeared in Gutenberg 19.2. Also, since this issue also occurs in WordPress 6.7 Beta1, it is necessary to identify the commit that caused the problem and fix it within the 6.7 cycle.

Step-by-step reproduction instructions

Below are the test steps when using Windows OS and NVDA.

WordPress 6.6.2

Everything is read correctly. In my testing, the screen reader basically reads out the label, role and description of the focused element.

• Open the post editor.
• Insert two Paragraph blocks.
• Select the First paragraph block.
• Press Shift+Tab to focus the block switcher button in the block toolbar. the screen reader should read:

  Edit Post “Test” ‹ gutenberg — WordPress  document   
  main landmark
  Editor content  region
  Block tools  tool bar
  Paragraph  menu button  collapsed  subMenu  Change block type or style
  

• Press the right arrow key to focus the "Move up" button. The screen reader should read:

  Move up  button  unavailable  Block Paragraph is at the beginning of the content and can’t be moved up
  

• Press the right arrow key to focus the "Move down" button. The screen reader should read:

  Move down  button  Move Paragraph block from position 1 down to position 2
  

• Press the right arrow key to focus the "Align text" button. The screen reader should read:

  Align text  menu button  collapsed  subMenu  Change text alignment
  

WordPres 6.7 Beta1 or Gutenberg 19.4.0-rc.1

All of these are incorrect. In my testing, screen readers read a description of the previously focused element, the label of the focused element, the role of the focused element and the label of the focused element.

• Open the post editor.
• Insert two Paragraph blocks.
• Select the First paragraph block.
• Press Shift+Tab to focus the block switcher button in the block toolbar. the screen reader should read:

  Edit Post “Test” ‹ gutenberg — WordPress  document   
  main landmark
  Editor content  region
  Block tools  tool bar
  Paragraph  menu button  collapsed  subMenu
  Paragraph
  

• Press the right arrow key to focus the "Move up" button. The screen reader should read:

  Change block type or style
  Move up  button  unavailable
  Move up
  

• Press the right arrow key to focus the "Move down" button. The screen reader should read:

  Block Paragraph is at the beginning of the content and can’t be moved up
  Move down  button
  Move down
  

• Press the right arrow key to focus the "Align text" button. The screen reader should read:

  Move Paragraph block from position 1 down to position 2
  Align text  menu button  collapsed  subMenu
  Align text
  

Screenshots, screen recording, code snippet

No response

Environment info

No response

Please confirm that you have searched existing issues in the repo.

• Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

• Yes

[t-hamano]

@talksina, If there are any other issues I've missed, I'd be grateful if you could add them.

@joedolson @afercia, I can't test on MacOS, so I'd appreciate it if you could test if this issue occurs with VoiceOver as well.

[t-hamano]

After investigating using git bisect, I found that this problem was caused by #64066.

@WordPress/gutenberg-components Do you have any good ideas to solve this? I suspect something about this code might be affecting it.

[t-hamano]

I also noticed that even outside of the block toolbar, buttons with tooltips changed what my screen reader announced when they received focus.

For example, if I focus on the "Toggle block inserter", "Tools", "Undo", "Redo", "Document overview" buttons in the post editor header in that order, my screen reader reads them as follows:

Before #64066

Toggle block inserter  toggle button  not pressed  collapsed

Tools  menu button  collapsed  subMenu

Undo  button  unavailable

Redo  button  unavailable

Document Overview  toggle button  not pressed  collapsed

After #64066

Toggle block inserter  toggle button  not pressed  collapsed
Toggle block inserter

Tools  menu button  collapsed  subMenu
Tools

Undo  button  unavailable UndoCtrl+Z

Redo  button  unavailable  RedoCtrl+Y

Document Overview  toggle button  not pressed  collapsed  Document OverviewShift+Alt+O

In addition to the aria-label text, the tooltip text also appears to be spoken.

[ciampo]

I haven't look too much in details, but the code that you highlighted seems to be doing the right thing — ie. adding a aria-describedby attribute, so that the anchor of the tooltip is described by the tooltip's text.

Also, for additonal context, this change on our side was necessary because arikit stopped adding the aria-describedby attribute to its tooltips — the code changes that you highlighted are a patch on our side to preserve that same behaviour while we decided if the resons why ariakit removed this feature should be considered in our component as well.

cc @DaniGuardiola who worked on this change, and @diegohaz in case he can spot anything missing from our patch which could cause this change in what the screen reader announces.

[DaniGuardiola]

That's correct @ciampo, and I think we more or less agreed to remove it but patch it to keep the impact of the upgrade relatively small. I think we could take a similar route to what we did with button sizes and introduce a __next prop, then progressively opt into the new behavior making any appropriate changes that retain accessibility.

[t-hamano]

Thank you for your feedback.

However, it is true that there is an issue with screen readers not reading element labels and descriptions correctly, at least with NVDA.

For example, I added the following story about the Button component:

Details

export const MultipleIcons = () => {
	return (
		<>
			<Button
				icon={ formatBold }
				label="Bold"
				description="Bold icon description"
			/>
			<Button
				icon={ formatItalic }
				label="Italic"
				description="Italic icon description"
			/>
			<Button
				icon={ link }
				label="Link"
				description="Link icon description"
			/>
		</>
	);
};

When I use the tab key to focus through the buttons, I hear:

(Focus the Bold button)

Bold button

(Focus the Italic button)

Bold icon description
Italic button

(Focus the Link button)

Italic icon description

Link button

Video recording of the test:

actual.mp4

I can see that the description, i.e. the aria-describedby text, is not being read out correctly.

Before #64066, it was read out as follows, which I believe is the correct reading:

(Focus the Bold button)

Bold  button  Bold icon description

(Focus the Italic button)

Italic  button  Italic icon description

(Focus the Link button)

Link  button  Link icon description

Video recording of the test:

expected.mp4

I would appreciate it if you could confirm if this issue occurs with VoiceOver as well.

[talksina]

with JAWS it's the same. I would expect it to read command, description and keyboard shortcut - maybe description and keyboard shortcut should appear in the control's "title" attribute? I'm not a coder unfortunately but what I know is that before Gutenberg 19.2 the issue wasn't there. Elena Brescacin - Accessibility, WordPress, HIV awareness E-Mail: ***@***.*** MOBILE: +39 328 7483161 LINK: ***@***.*** <https://linktr.ee/ilmondopositivo> Il giorno sab 5 ott 2024 alle 16:38 Aki Hamano ***@***.***> ha scritto:

…

Thank you for your feedback. However, it is true that there is an issue with screen readers not reading element labels and descriptions correctly, at least with NVDA. For example, I added the following story about the Button component: Details export const MultipleIcons = () => { return ( <> <Button icon={ formatBold } label="Bold" description="Bold icon description" /> <Button icon={ formatItalic } label="Italic" description="Italic icon description" /> <Button icon={ link } label="Link" description="Link icon description" /> </> );}; When I use the tab key to focus through the buttons, I hear: (Focus the Bold button) Bold button (Focus the Italic button) Bold icon description Italic button (Focus the Link button) Italic icon description Link button Video recording of the test: https://github.com/user-attachments/assets/e47492c7-7e4e-47e7-ac94-82ec8e7cd192 I can see that the description, i.e. the aria-describedby text, is not being read out correctly. Before #64066 <#64066>, it was read out as follows, which I believe is the correct reading: (Focus the Bold button) Bold button Bold icon description (Focus the Italic button) Italic button Italic icon description (Focus the Link button) Link button Link icon description Video recording of the test: https://github.com/user-attachments/assets/793f650c-4f8b-4786-9ae5-2fe13e4ee1de I would appreciate it if you could confirm if this issue occurs with VoiceOver as well. — Reply to this email directly, view it on GitHub <#65888 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADT5KDURNPZA4PX3WSUTTO3ZZ72VTAVCNFSM6AAAAABPM6ZHJWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGOJVGA3TSMJVHE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[joedolson]

Confirmed this is also happening on VoiceOver.

I looked at what's happening, and the aria-describedby value is changing dynamically when the tooltip is present. If the tooltip is present, the aria-describedby refers to the tooltip; if it isn't, it refers to the description. Since the tooltip is triggered when the component receives focus, the description used is not the right one.

The tooltip is usually the accessible name of the control; as such, it should be in the aria-label (which it is), and not also duplicated by the aria-description. There may be cases where we're using tooltips for additional information, but in most cases it's just the name of the control. I'd only render an aria-description if the text used is not the same as the value used in the aria-label.

[DaniGuardiola]

@joedolson that could be a way to improve the patch while we figure out how to transition away from it.

[t-hamano]

The important thing to note is that this issue also occurs in WP 6.7.

[diegohaz]

@diegohaz in case he can spot anything missing from our patch which could cause this change in what the screen reader announces.

How can I test WordPres 6.7 Beta1 or Gutenberg 19.4.0-rc.1? I tried https://wordpress.github.io/gutenberg and https://gutenberg.run, but I couldn't reproduce the issue using Safari and VoiceOver. I can try NVDA on Windows later, but I'm afraid I'm not using the correct version.

[t-hamano]

@diegohaz You can use the Playground to test WP 6.7 Beta1: https://playground.wordpress.net/?php=8.0&wp=beta

[diegohaz]

@diegohaz You can use the Playground to test WP 6.7 Beta1: https://playground.wordpress.net/?php=8.0&wp=beta

Thanks! I still can't get VoiceOver/Safari 17.2 to announce the description of the previously focused button. However, there's a flash of text on the caption panel just before I move the focus, which might refer to the previous description, but I'm not sure. I can reproduce the issue using NVDA, though.

I suspect it's caused by dynamically changing the aria-describedby attribute. I didn't notice any item referring to the wrong description. They all seem to point to the correct elements, but updating the attribute's value might be notifying the screen reader and triggering the announcement just before the newly focused element is announced.

I suggest not relying on tooltips for labels or descriptions, especially since Gutenberg removes tooltips from the DOM when they're not displayed. Treat them as purely visual elements. Labels and descriptions should be rendered statically in the DOM (using the aria-label attribute or as visible/visually hidden text, for example). This is the safest option, and by avoiding tooltips on the accessibility tree, you also prevent duplicated labels.

[t-hamano]

What do you think about making the following changes to prevent the aria-describedby value from changing dynamically and to prevent labels from being read twice?

In other words, only when the child does not have the aria-describedby attribute and the tooltip text and aria-label are different, add aria-describedby to the tooltip anchor and reference the tooltip text as the description. The unit tests seem to pass.

diff --git a/packages/components/src/tooltip/index.tsx b/packages/components/src/tooltip/index.tsx
index 7ce9311fc9..1b61062180 100644
--- a/packages/components/src/tooltip/index.tsx
+++ b/packages/components/src/tooltip/index.tsx
@@ -109,7 +109,10 @@ function UnforwardedTooltip(
        // the tooltip anchor anymore since 0.4.0, so we need to add it manually.
        // See: https://github.com/WordPress/gutenberg/pull/64066
        function addDescribedById( element: React.ReactElement ) {
-               return describedById && mounted
+               return describedById &&
+                       mounted &&
+                       element.props[ 'aria-describedby' ] === undefined &&
+                       element.props[ 'aria-label' ] !== text
                        ? cloneElement( element, { 'aria-describedby': describedById } )
                        : element;
        }

[ciampo]

That may work, although this could be the right moment to move away from relying on tooltips to add meaningful descriptive text:

• as Diego mentioned above, "I suggest not relying on tooltips for labels or descriptions [...] Treat them as purely visual elements. Labels and descriptions should be rendered statically in the DOM (using the aria-label attribute or as visible/visually hidden text, for example). This is the safest option, and by avoiding tooltips on the accessibility tree, you also prevent duplicated labels."
• this change in ariakit was actually informed by some feedback shared from experience in Gutenberg — see Tooltip should not use role and aria-describedby ariakit/ariakit#3242
• as Dani mentioned above, the current patch in our Tooltip component was meant to be temporary while we transition to purely visual tooltips

Given all of the above, I wonder if, instead of tweaking the aria-describedby behaviour, we should directly refactor the usages in the repo so that they don't rely on Tooltip at all for descriptive text. It feels like the correct long term solution to me.

[ciampo]

Related: #61168 (comment)

[afercia]

I suggest not relying on tooltips for labels or descriptions, especially since Gutenberg removes tooltips from the DOM when they're not displayed. Treat them as purely visual elements.

I totally agree. Tooltips in Gutenberg have been originally implemented only to visually expose the accessible name (the aria-label) of icon buttons. Usin them for descriptions or other information is far from ideal. This was reported in other issues as well but still, there are occurrences in the editor where tooltips are used for instructions, descriptions and such. All those cases should be audited.

the code changes that you highlighted are a patch on our side to preserve that same behaviour while we decided if the resons why ariakit removed this feature should be considered in our component as well.

The previous ariakit implementation was less than ideal and I'm glad it has been changed upstream. Re-implementing it in the editor is less than ideal as well and dismisses the recommendation made from accessibility specialists in this project. Can you point me please to the change that patched the implementation in the editor?

From a design, usability, UI, perspective, placing essential information in a tooltip is less than ideal in the first place. Some users may have not accesso the the tooltip referenced by aria-describedby. Announcement of description may also be disabled in the screen readers user preferences. Also, the aria-describedby value isn't supposed to be dynamically updated. Screen readers don't expect this value to change dynamically so that isn't guaranteed to work.

move away from relying on tooltips to add meaningful descriptive text:

Yes please. This should be documented in the components and in the design guidelines: tooltips must not be used for descriptions. Thanks.

[DaniGuardiola]

For context, the only reason we added the patch was to keep the impact of the Ariakit upgrade work as minimal as possible, but we were aligned on removing the behavior down the line, to align with the Ariakit change. We didn't give it any further thought and we also didn't see a problem with the patch back then, it seemed to correctly mimic the previous Ariakit behavior but it's possible that we missed some nuance in the implementation.

I'm glad this came up because it's a sign that we should probably do the work to remove the patch and handle accessible labels in all impacted usages of Tooltip.

I see two paths:

• A single PR that adds any aria-labels, hidden text, etc necessary and removed the patch.
• A gradual approach with a temporary prop to enable/disable the patch so we can transition in batches.

I think the single PR is probably fine, as it's not a big change and it can save us a lot of iteration and review time. Any mistakes can be fixed afterwards with small PRs.

What do y'all think?

[afercia]

Can you point me please to the change that patched the implementation in the editor?

I' dlike to have a look at the reasoning and process there, thanks,

[t-hamano]

• A single PR that adds any aria-labels, hidden text, etc necessary and removed the patch.
• A gradual approach with a temporary prop to enable/disable the patch so we can transition in batches.

I think either path makes sense, but I think this needs to be fixed in the WP 6.7 cycle.

Also, do we need a dev note? I mean, something like "Don't include important text in the tooltip text because screen readers will no longer read it."

[ciampo]

• A single PR that adds any aria-labels, hidden text, etc necessary and removed the patch.
• A gradual approach with a temporary prop to enable/disable the patch so we can transition in batches.

I think either path makes sense, but I think this needs to be fixed in the WP 6.7 cycle.

Also, do we need a dev note? I mean, something like "Don't include important text in the tooltip text because screen readers will no longer read it."

We can start making all the changes in one PR, and decide whether we want to extract them to several PRs before merging.

Can you point me please to the change that patched the implementation in the editor?

I' dlike to have a look at the reasoning and process there, thanks,

@afercia this is the PR. As mentioned by Dani, the rationale for applying the patch on our end was to minimize the amount of changes to test / introduce at once with that ariakit update, since there were many already. To reiterate, we are all aligned in removing this behavior from <Tooltip />, we just didn't prioritize it that moment.

[afercia]

@afercia this is the PR. As mentioned by Dani, the rationale for applying the patch on our end was to minimize the amount of changes to test / introduce at once with that ariakit update, since there were many already

Thanks @ciampo. My point is more about process. I don't see that PR marked with the accessibility label even though it touched an important accessibility issue and the original ariakit behavior was already reported as problematic. I'm not sure that PR was only about technical implementation and for better collaboration in this project I'd like to see accessibility specialists involved in the discussion of important accessibility issues. Thanks,

[ciampo]

My point is more about process. I don't see that PR marked with the accessibility label even though it touched an important accessibility issue and the original ariakit behavior was already reported as problematic. I'm not sure that PR was only about technical implementation and for better collaboration in this project I'd like to see accessibility specialists involved in the discussion of important accessibility issues. Thanks,

Got it. Since we decided to add that patch to keep things unchanged, we didn't think there was a need for the accessibility label. Basically, we updated a bunch of stuff and the tooltip's behaviour was untouched. We would have definitely tagged any issues/PRs touching that aspect of Tooltip with the accessibility label.

[afercia]

nd the tooltip's behaviour was untouched

I'm not sure why but it appears there is a change somewhere as the double announcement (accessible name + accessible description) of the toolbar buttons wasn't happening with the previous ariakit version and before #64066

The behavior changed somewhere ant this is actually a regression, I'm going to label this issue accordingly.

[afercia]

How can I test WordPres 6.7 Beta1 or Gutenberg 19.4.0-rc.1?

@diegohaz you may want to clone the Gutenberg plugin repo in the WordPress wp-content/plugins directory, build it and activate it. However, this has been tested enough and I can reproduce it with Safari and VoiceOver as well. Not surprising because, when the toolbar buttons are hovered or focused, the value of aria-describedby changes or gets added dynamically and points to the tooltip.

At that point, both the accessible name and the accessible description have the same text and both get announced, with default screen reader user preferences (some screen readers allow to disable the announcement of descriptions).

Screenshots:

[t-hamano]

We can start making all the changes in one PR, and decide whether we want to extract them to several PRs before merging.

Is there anything I can do to move this forward? Sorry, I may not yet have the full picture of what tasks should be handled.

[ciampo]

Potentially the quickest, short term change could be to add a check in Tooltip so that the aria-describedby attribute is added only if there isn't one already?

After that, the long term fix would be to remove the code adding the aria-describedby attribute from Tooltip, and manually review each usage (with the help of the accessibility team)

[afercia]

so that the aria-describedby attribute is added only if there isn't one already?

Not sure that would solve the double announcement issue (name and description with the same text). Buttons that don't have an aria-describedby would get one that just repeats the name. GIF:

[ciampo]

@afercia I put together a tentative short-term improvement, following @t-hamano 's earlier suggestion:

#65989

Could you give it a quick check? Hopefully it's acceptable as a short-term fix for the 6.7 release. And then we can immediately start working on a long-term fix (ie. remove the aria-describedby attribute addition from the Tooltip component altogether).

[ciampo]

Update:

• short-term fix merged and backported: Tooltip: add aria-describedby to anchor only if not redundant #65989
• long-term effort kickstarted, and in need of feedback: Tooltip: do not add the aria-describedby attribute to the anchor #66021

[getdave]

Thanks for all the work that went into discussing this and finding a good solution.

As #65989 is now going to be in WordPress 6.7 I will remove this Issue from the board.

The long term effort can continue during the 6.8 cycle.

cc @kevin940726 @ndiego @colorful-tones