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

Radio group should not listen to nested children under another radio group (aka: allow nesting) #2138

Open
AlexandreBonaventure opened this issue Aug 15, 2024 · 7 comments
Labels
bug Things that aren't working right in the library.

Comments

@AlexandreBonaventure
Copy link

Describe the bug

Hey!

I was trying to implement some kind of form with nested radio choices, with dynamic details depending on parent selection. However I noticed that you can't nest sl-radio-group without having their state "polluted" by inner most component.
To make it clearer here are some screenshots:

First you select between A and B:
Capture d’écran, le 2024-08-15 à 13 10 38
Then you need to select a refinement option between B1 and B2:
What's expected:
Capture d’écran, le 2024-08-15 à 13 10 51 copie
What happens:
Capture d’écran, le 2024-08-15 à 13 10 51

Demo

https://jsfiddle.net/alex_bonaventu/3xokyn0g/3/

To me it would make sense that this component allow nesting to support these use-cases. Not sure if that violates some accessibility principles though....
Let me know if it makes sense to you as well, I can easily PR I think by filtering further radio components in that query:
https://github.com/shoelace-style/shoelace/blob/next/src/components/radio-group/radio-group.component.ts#L127

Thanks!

@AlexandreBonaventure AlexandreBonaventure added the bug Things that aren't working right in the library. label Aug 15, 2024
@KonnorRogers
Copy link
Collaborator

To me it would make sense that this component allow nesting to support these use-cases. Not sure if that violates some accessibility principles though....

Testing it in VoiceOver + Safari, it reads as "1 of 5, 2 of 5" for the outer radios. And for the inner radios, it reads as "1 of 7", "2 of 7".

So at the very least im not sure theres a way to implement this pattern in an accessible way 🤔

I'm also not sure if it has to do with the role="radiogroup" being in the shadow DOM.

Do you have a real world example of what these forms would look like so we can have a better idea if this is something we want to support?

@AlexandreBonaventure
Copy link
Author

Sure!
We have a couple of places where we have "radio detail cards". Ideally, the parent cards are wrapped by a radio group to leverage easy data binding.
eg: sso providers forms, where some have inner radio options
Capture d’écran, le 2024-08-15 à 14 02 53
or more complex configuration forms like this one:
image

@claviska
Copy link
Member

Due to accessibility concerns, I don't think the current radio group component will work well here.

@lindsaym-fa this might be a pattern we can consider supporting in Web Awesome, unless you have an alternative to suggest :)

@lindsaym-fa
Copy link
Collaborator

@claviska Agreed! Nesting radio components doesn't quite seem sound to me. But, in our quest for broader "choice group" capabilities, supporting radio behavior for the details component seems absolutely worthwhile.

@KonnorRogers
Copy link
Collaborator

KonnorRogers commented Aug 19, 2024

@lindsaym-fa I can't find any combo of aria roles to make this work.

A native <input type="radio"> could work here, but only because it gets special screenreader handling by creating implicit groups.

Let me do some research and checkout some prior art and issues to see if anyone else has filed anything around this.

I'm hoping FormAssociated + ElementInternals can solve this, although to my knowledge hasn't been implemented by screen readers (yet)

https://codepen.io/paramagicdev/pen/abgqKjz

Here's a video of how nested radiogroups get read in VoiceOver (haven't tested NVDA yet)

2024-08-19.12-25-00.mp4

@KonnorRogers
Copy link
Collaborator

Also, assuming those radios live inside the summary of the sl-details, it kind of violates the "no nested interactive elements" principle. Hard to tell from the screenshot what the actual UX is like however. But your original example with nested radio choices I have seen before in dynamic forms.

@KonnorRogers
Copy link
Collaborator

So it appears its doable, perhaps with a little hackery we can emulate native radios 🤔

https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/radio_role#associated_wai-aria_roles_states_and_properties

https://codepen.io/paramagicdev/pen/MWMQBdj

Everyone except Safari supports it on VoiceOver. I'll have to check NVDA.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Things that aren't working right in the library.
Projects
None yet
Development

No branches or pull requests

4 participants