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

The RadioControl component renders invalid and not accessible markup #52890

Closed
afercia opened this issue Jul 24, 2023 · 1 comment · Fixed by #64582
Closed

The RadioControl component renders invalid and not accessible markup #52890

afercia opened this issue Jul 24, 2023 · 1 comment · Fixed by #64582
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Package] Components /packages/components [Package] Edit Site /packages/edit-site [Type] Bug An existing feature does not function as intended

Comments

@afercia
Copy link
Contributor

afercia commented Jul 24, 2023

Description

While investigating the Post Status UI in teh Site editor in #52885 and #52634 I noticed the RadioControl component is open to misuse by developers and has some limitations that cause accessibility problems. Both the component and its implementations should be reviewed thoroughly.

At a first check:

  • The label prop renders a stray label element that is not associated with any input.
  • No fieldset and legend to group the radio boxes.
  • No ability to set a description for each radio button (associated with aria-describedby).
  • As such, in the PageStatus component the description has been added within the label, thus making the accessible name of the inputs very confusing: name and description should be separated.
  • The help prop renders some additional text at the end of all radio buttons. This text is associated with all the radio buttons via aria-describedby. This may be useful in some cases but it's likely to be redundant and confusing. In most cases, each radio button will need its won description. Worth reminding aria-describedby may accept multiple ID refs.

In the screenshot below I made some visually hidden elements visible to better illustrate, and set some options:

In all three case the HTML is invalid. The labelling isn't ideal, given the stray label.

Given the component limitations, maybe it's not a case that in the Post editor, in the very similar Post visibility UI, an ad-hoc implementation has been made, see the PostVisibilityChoice component.

Ideally, the RadioControl component should behave more like the PostVisibilityChoice one, or at least produce valid markup, render fieldset+legend, and be more flexible to be able to replace PostVisibilityChoice.

Cc @ciampo @mirka

Step-by-step reproduction instructions

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 [Package] Edit Site /packages/edit-site labels Jul 24, 2023
@ciampo ciampo moved this to Todo in Andrew's onboarding Sep 1, 2023
@mirka
Copy link
Member

mirka commented Dec 22, 2023

For quick reference, here are the radio UI patterns used in the Post Visibility and Page Status components:

Post Visibility radio Page Status radio

RadioControl does not support this pattern yet, and the above usages work around that in different ways. (Post Visibility doesn't even use RadioControl and does a custom implementation.)

I classify this as a similar problem to what we've seen with CheckboxControl, where our components aren't flexible enough therefore it's very easy for consumers to overlook certain accessibility considerations in their workarounds.

I think we should look into providing an easier, "officially sanctioned" way to accomplish these kinds of custom layouts with the (currently monolithic) CheckboxControl/RadioControl components so we can provide better documentation and examples on how to do it accessibly. It might look similar to how we're designing the new Tabs and DropdownMenu components, where we export all the subcomponents to support customization.

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 [Package] Edit Site /packages/edit-site [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants