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

fix: 🐛 make validation more accessible in all form elements #1619

Merged
merged 8 commits into from
Nov 15, 2024

Conversation

Vahid1919
Copy link
Contributor

@Vahid1919 Vahid1919 commented Nov 6, 2024

Description:

Closes #1483

@solid-design-system/core-development There is a disclaimer that changes made here will affect all other form-related components. In code, the fieldset element is defined inside sd-radio-group. I do not see how it has an effect on the other components since the only relation to the formController is referencing the id to link the labels.
Please let me know, if I am incorrect here.

Definition of Reviewable:

  • relevant tickets are linked

@Vahid1919 Vahid1919 linked an issue Nov 6, 2024 that may be closed by this pull request
15 tasks
Copy link

github-actions bot commented Nov 6, 2024

🚀 Storybook has been deployed for branch fix_sd-radio-group-a11y

Copy link
Contributor

@karlbaumhauer karlbaumhauer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Vahid1919 the issues mentions a problem about missing labels on icon only templates. Did you check that (as I am missing changes on this topic)?

I also would like to clarify in the daily, if someone remembers what exactly we wanted to accomplish with the mentioned note/caution hint in the ticket. I second that I don't see a problem with the changes in any other component but we added it for some reason...;)

@karlbaumhauer karlbaumhauer removed their assignment Nov 7, 2024
@paulovareiro29 paulovareiro29 self-assigned this Nov 11, 2024
@Vahid1919
Copy link
Contributor Author

Vahid1919 commented Nov 13, 2024

@solid-design-system/core-development I have addressed the recommendations regarding reporting errors on screen readers.

I managed to get error messages to be communicated in almost all form elements except sd-radio-group (does not work on Chrome but works fine on Safari -- both tested with VoiceOver).

I found some forum discussions regarding what to do in case of screen-reader differences while testing: https://club.ministryoftesting.com/t/why-do-screen-readers-behave-so-differently-from-one-another/69298

How would you suggest we handle these differences right now? @mariohamann @smfonseca @karlbaumhauer

PS: I tried some "hacks" using aria-labelledby that allow screen reader error reporting in Chrome, but those cause duplicate readings on Safari.

@mariohamann mariohamann changed the title fix: sd-radio-group a11y fix: 🐛 validation a11y in all form elements Nov 15, 2024
@mariohamann mariohamann changed the title fix: 🐛 validation a11y in all form elements fix: 🐛 make validation more accessible in all form elements Nov 15, 2024
@mariohamann
Copy link
Contributor

Apparently there seems to be an issue with how radio-button then handles some stuff in comparison to radio? Did you make a comparison on what's the difference? I would be fine if you would extract that issue in a new ticket, if you could properly document, what's happening and what you did.

@mariohamann mariohamann removed their assignment Nov 15, 2024
@Vahid1919
Copy link
Contributor Author

VoiceOver issues for sd-radio-group will now be handled here: #1648

@karlbaumhauer karlbaumhauer removed their assignment Nov 15, 2024
@Vahid1919 Vahid1919 merged commit 7a2ca8d into main Nov 15, 2024
13 of 15 checks passed
@Vahid1919 Vahid1919 deleted the fix/sd-radio-group-a11y branch November 15, 2024 13:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

feat[dev]: ✨ implement A11y improvements to sd-radio-group
7 participants