fix(foundation): remove readonly from radio button

[yinonov]

readonly attribute on a radio button violates a WCAG rule. see #6437

there's no evidence readonly should be applied on a radio button. it does on radio group however

• aria-readonly associated roles on MDN doesn’t list radio
• https://stackoverflow.com/questions/1953017/why-cant-radio-buttons-be-readonly
• https://www.w3schools.com/jsref/dom_obj_radio.asp

as a matter of fact, native radio ignores readonly on interaction

Screen.Recording.2022-10-09.at.9.22.14.mov

[yinonov]

closes #6437

[nicholasrice]

Thanks for putting this together! I think there is still a little more that needs to be done here. Previously, the radio itself would prevent changing its checked value when readonly. That behavior essentially needs to be moved instead of removed; the checkbox radio shouldn't get checked when that radiogroup is readonly.

[yinonov]

the checkbox shouldn't get checked when that radiogroup is readonly.

you mean radio shouldn't get checked?

[nicholasrice]

the checkbox shouldn't get checked when that radiogroup is readonly.

you mean radio shouldn't get checked?

Yes, sorry - radio

[yinonov]

I ran the site locally on this branch and it seems to already be covered.

correct me if I'm missing anything here @nicholasrice but this is the expected behavior when radio group is set to readonly correct? preventing any change to radio-buttons on user interaction

Screen.Recording.2022-10-11.at.10.17.53.mov

[nicholasrice]

I ran the site locally on this branch and it seems to already be covered.

correct me if I'm missing anything here @nicholasrice but this is the expected behavior when radio group is set to readonly correct? preventing any change to radio-buttons on user interaction

Screen.Recording.2022-10-11.at.10.17.53.mov

Odd, yes that is my expectation. When I set readonly in storybook on your branch it didn't function correctly, but I'll look again; maybe I messed something up.

[nicholasrice]

I just tried again - setting the readonly attribute on the radiogroup custom element does not prevent the user from changing radio checked values in fast-foundation's storybook app. Try to repro there.

[yinonov]

I just tried again - setting the readonly attribute on the radiogroup custom element does not prevent the user from changing radio checked values in fast-foundation's storybook app. Try to repro there.

you're right. now covered click, keypress and arrow navigation and tabbing into group.

took the liberty to refactor a const to class getter as it repeated in more than 2 places.

https://github.com/yinonov/fast/blob/11ee0292de3667eeadb164ac3a3210694716301c/packages/web-components/fast-foundation/src/radio/radio.ts#L56-L60

[nicholasrice]

Looks good, with one suggestion. Thanks for making this update!

[yinonov]

Can this be merged?

[EisenbergEffect]

Since we removed the tests for the radio itself...do we have an equivalent test for the group?

[yinonov]

Since we removed the tests for the radio itself...do we have an equivalent test for the group?

radio-group had already featured its own readonly member.

anything you think should be added considering this change?

[EisenbergEffect]

Since we removed the tests for the radio itself...do we have an equivalent test for the group?

radio-group had already featured its own readonly member.

anything you think should be added considering this change?

Do we have a test or some other way to ensure that when setting the group to readonly that the individual radios will be readonly? Does the platform handle that for us?

[yinonov]

Since we removed the tests for the radio itself...do we have an equivalent test for the group?

radio-group had already featured its own readonly member.
anything you think should be added considering this change?

Do we have a test or some other way to ensure that when setting the group to readonly that the individual radios will be readonly? Does the platform handle that for us?

How about testing if setting checked property to true on radio actually functions (when radio-group is set to readonly).
Thing is, it's not hardened in the current implementation. setting a readonly radio-group AND radio can be bypassed programmatically.

I guess this should be hardened? in the scope of this PR?

Screen.Recording.2022-10-26.at.22.03.25.mov

[chrisdholt]

@yinonov @olaf-k Given the refactor for "disabled" behavior in PR, etc and the new tests added, I'd like to rebase this and test once those changes are in and follow-up on the tests/behavior at that point. I think the changes made in #6601 will lend themself more towards ensuring that radios are treated as "readonly" from an interactive standpoint. My only other thought there is ensuring that they're communicated in the a11y tree as readonly...but that we can validate at the same time. Thoughts?

[chrisdholt]

@yinonov - with the radio/radio group refactor in to solve the state issue it looks like we have some conflicts here. I'd love to get this in. Do we want to resolve conflicts on this branch or just kick off a new one to remove readonly? Happy to move it through quickly regardless of the approach :)

[yinonov]

@yinonov - with the radio/radio group refactor in to solve the state issue it looks like we have some conflicts here. I'd love to get this in. Do we want to resolve conflicts on this branch or just kick off a new one to remove readonly? Happy to move it through quickly regardless of the approach :)

no sweat, resolved the conflicts

[yinonov]

Can this be merged?

[chrisdholt]

Can this be merged?

It looks like there are some build failures directly related to the PR. Once we get a passing build I'll pull this down, validate the behavior and merge. Thanks!

[yinonov]

soz. build works now (locally)

[chrisdholt]

@yinonov looks like there are still a couple failures - I created a PR against your branch to address these (or you can run prettier and yarn locally to address). yinonov#1

Once this is in/updated I'd like to get this in!

[yinonov]

Thanks @chrisdholt. Merged