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

CustomSelectControl: fix describedBy implementation and docs #63568

Closed
2 tasks done
afercia opened this issue Jul 15, 2024 · 4 comments · Fixed by #63957 · May be fixed by #63884
Closed
2 tasks done

CustomSelectControl: fix describedBy implementation and docs #63568

afercia opened this issue Jul 15, 2024 · 4 comments · Fixed by #63957 · May be fixed by #63884
Assignees
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Package] Components /packages/components [Status] In Progress Tracking issues with work in progress [Type] Regression Related to a regression in the latest release

Comments

@afercia
Copy link
Contributor

afercia commented Jul 15, 2024

Description

Discovered while investigating #63483

The CustomSelectControl component has a describedBy prop that accepts a string.

It is supposed to provide an accessible description for the toggle button that opens the custom select.

Unfortunately, both the implementation and the docs are wrong and the component renders non-accessible markup.

An example can be seen for the font appearance setting. It can be reproduced in the Storybook as well.

Turns out the value passed to the describedBy prop is just rendered as the value of the aria-describedby attribute.

aria-describedby={ describedBy }

Instead, the aria-describedby attribute accepts an ID reference that points to an element containing the description.

For example, the font appearance setting renders this markup:

aria-describedby="Currently selected font appearance: Medium"

The correct usage would be:

... aria-describedby="an-unique-id-here" ...

<div hidden id="an-unique-id-here">Currently selected font appearance: Medium</div>

The component should be refactored to actually render another element that contains the description, referenced by an ID, much like what the Button component does.

At that point, the describedBy prop should be renamed to description.

The readme and types.ts must be fixed as well. For example, this prop description doesn't appear to be correct:

Description for the select trigger button used by assistive technology. If no value is passed, the text "Currently selected: selectedItem.name" will be used fully translated.

Step-by-step reproduction instructions

  • Select a Group block
  • Go to the block Inspector > Styles, and activate the Appearance settings in the Typography options.
  • Set a font appearance value e.g. 'Medium',
  • Inspect the CustomSelectControl toggle button source.
  • Oberve it has an incorrect aria-describedby="Currently selected font appearance: Medium"
  • Alternatively go to the Storybook: https://wordpress.github.io/gutenberg/?path=/docs/components-customselectcontrol--docs
  • Set the describedBy prop to la la la.
  • Inspect the CustomSelectControl toggle button source.
  • Oberve it has an incorrect aria-describedby="la la la"

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
@afercia afercia added [Type] Bug An existing feature does not function as intended [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Package] Components /packages/components labels Jul 15, 2024
@afercia
Copy link
Contributor Author

afercia commented Jul 15, 2024

It is interesting to note the aXe-powered accessibility checks in the Storybook do catch the issue, but only when the describedBy prop is manually set.

Screenshot 2024-07-15 at 16 06 40

@afercia afercia changed the title CustomSelectControl: fix describedBy implementation and cos CustomSelectControl: fix describedBy implementation and docs Jul 25, 2024
@mirka
Copy link
Member

mirka commented Jul 25, 2024

Thanks for catching this. I looked into it and there are a couple of things going on here:

So what we need to first do is reimplement the original behavior. Any deprecation/renaming to description should be a separate PR. (No strong opinion about this, but I'll note that CustomSelectControl v1 itself is on track to be deprecated)

@mirka mirka self-assigned this Jul 25, 2024
@github-actions github-actions bot added the [Status] In Progress Tracking issues with work in progress label Jul 25, 2024
@afercia afercia added [Type] Regression Related to a regression in the latest release and removed [Type] Bug An existing feature does not function as intended labels Jul 26, 2024
@afercia
Copy link
Contributor Author

afercia commented Jul 26, 2024

Thanks for the added context @mirka. Changed the issue label type to regression.
I won't have time to look into this in the next weeks so if anyona is willing to have a look at it, please do feel free to do that.

@tyxla
Copy link
Member

tyxla commented Jul 26, 2024

Good catch @afercia 🙌

Thanks, @mirka, for working on it; I'm taking a look.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Package] Components /packages/components [Status] In Progress Tracking issues with work in progress [Type] Regression Related to a regression in the latest release
Projects
None yet
3 participants