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

Adding label/description to BlockEditor/DuotoneControl #54473

Conversation

andrewhayward
Copy link
Contributor

What?

This PR ensures that the main duotone picker found in the image duotone filter panel has an identifying label and relevant description.

Screenshot of image duotone filter panel, with hand-drawn arrows connecting a description and the label 'Apply duotone filter' with a duotone picker

Why?

As per #54055, every DuotonePicker needs to have an appropriate contextual label.

How?

An ID is generated and assigned to the node containing the popup description. This is linked to the DuotonePicker with an aria-describedby props. Additionally, an aria-label is applied to the DuotonePicker matching the tooltip display value.

The DuotonePicker component is updated to accept the additional aria-describedby prop. (And, as a bonus, the 'unset' option gets a label.)

Testing Instructions

  • Add an image in the editor
  • Open the 'Apply duotone filter' popup
  • Confirm that the duotone picker has an aria-describedby attribute, and that this points to the description
  • Confirm that the duotone picker has an aria-label attribute

@ciampo ciampo requested review from chad1008 and brookewp September 14, 2023 19:50
@ciampo ciampo self-requested a review September 14, 2023 19:50
@ciampo ciampo added [Package] Components /packages/components [Type] Enhancement A suggestion for improvement. labels Sep 14, 2023
Copy link
Contributor

@ciampo ciampo left a comment

Choose a reason for hiding this comment

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

This is testing well at a first glance!

@andrewhayward could you merge/rebase in order to solve conflicts and include changes from #54468 ? That way we'll be able to take a final look at this. Thank you!

Also, it looks like similar improvements could be made to the DuotonePicker in the global styles filters panel and gradient palette's panel

@alexstine alexstine self-requested a review September 19, 2023 02:16
Copy link
Contributor

@brookewp brookewp left a comment

Choose a reason for hiding this comment

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

I only get the label and unset button via screenreader when opening the duotone dropdown. Is it expected, or should the description be included as well?

duotone

Copy link
Contributor

@chad1008 chad1008 left a comment

Choose a reason for hiding this comment

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

This looks good to me as well, though I do second @brookewp's question regarding screen readers. I also jump directly to Unset and see that label when navigating into the Duotone picker.

If I then navigate out of the current group (ctrl + option + shift + uparrow in VoiceOver, iirc) the actual ColorPicker is focused by the virtual cursor, and that does read out the correct description. So I suppose for me question would be do we want the virtual cursor to jump right to the first option (i.e. Unset)? Or to the ColorPicker itself, to highlight that description initially?

@andrewhayward
Copy link
Contributor Author

Thanks for the feedback, @brookewp @chad1008.

...should the description be included as well?

As @chad1008 pointed out, the description applies to the listbox control, rather than the individual options, so won't show up when navigating through them. If you focus the listbox, it does then get announced.

Screenshot showing listbox with focus, and VoiceOver caption panel reading

So...

So I suppose for me question would be do we want the virtual cursor to jump right to the first option (i.e. Unset)? Or to the ColorPicker itself, to highlight that description initially?

Good question. Currently, Popover (used by Dropdown), which renders this, defaults to focusing the first relevant element when it opens. Because the CircularOptionPicker itself isn't currently focusable, this means that focus goes to the first/selected option within it. We could change that, but probably not in this PR.

@ciampo
Copy link
Contributor

ciampo commented Sep 28, 2023

Good question. Currently, Popover (used by Dropdown), which renders this, defaults to focusing the first relevant element when it opens. Because the CircularOptionPicker itself isn't currently focusable, this means that focus goes to the first/selected option within it. We could change that, but probably not in this PR.

Probably a good task to work on as a follow-up (although not urgent).

Let's merge this PR in the meantime

@andrewhayward andrewhayward merged commit 15c2dd9 into WordPress:trunk Sep 28, 2023
49 checks passed
@github-actions github-actions bot added this to the Gutenberg 16.8 milestone Sep 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components [Type] Enhancement A suggestion for improvement.
Projects
Status: Done 🎉
Development

Successfully merging this pull request may close these issues.

4 participants