shoelace-style / shoelace

A collection of professionally designed, every day UI components built on Web standards. SHOELACE IS BECOMING WEB AWESOME 👇👇👇
https://webawesome.com
MIT License
12.66k stars 804 forks source link

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

Open AlexandreBonaventure opened 1 month ago

AlexandreBonaventure commented 1 month ago

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!

KonnorRogers commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 4 weeks ago

@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 commented 4 weeks ago

@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)

https://github.com/user-attachments/assets/a90282f4-f1c1-4964-9a34-30057fb25786

KonnorRogers commented 4 weeks ago

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 commented 4 weeks ago

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.