-
-
Notifications
You must be signed in to change notification settings - Fork 389
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
Small improvements on forms accessibility #1536
base: main
Are you sure you want to change the base?
Conversation
It's a small issue, but it's more straightforward for screen reader users to have headings hierarchy starting at first lvl instead of arbitrary 2nd one. No risk of stumbling upon the h2 then asking ourselves if we didn't miss anything before it (like, where is the h1?)
not a big issue, but making the distinction between the main content and the footer with the correct html tags can help people navigate better in the form
…sers this makes the "grist" logo announced by screen readers. it's not the ideal way to handle that, but it's a bit tricky to handle this better in order to support rtl languages correctly. This will do for now.
this will mostly help screen reader users: - have a page heading before showing the success text. it's not shown, we just wrap the decorative image that we add an alt text to, - use main/footer tags - wrap the text in a <p> tag for better screen reader navigation support
5d61501
to
858a159
Compare
- make every input correctly be tied to their labels by adding `id` attributes. This helps screen reader users a lot - for radio groups and checkbox groups, simulate fieldset/legend with `role=group` + `aria-labelledby` attributes on the containers*. We simulate with aria roles to prevent css issues and having to handle those changes in current code. The result is the same for screen readers. - markup is not the cleanest as is, because now radio/checkbox groups still use a `label` tag for their heading, that has an `id` attr pointing to nowhere. It's not an issue in real use though. It makes the js code cleaner to not handle the switch to another dom el/removing the for attr.
858a159
to
8d61626
Compare
The last commit of this PR is a bit unrelated to this but I had trouble running local tests without those small changes. Figured I'd left the changes here. @berhalak it might interest you, it's kind of the same logic that you included recently to fix tests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds great to me! Few comments on my side.
@@ -1452,6 +1452,7 @@ | |||
}, | |||
"FormContainer": { | |||
"Powered by": "Задвижвано от", | |||
"Powered by Grist": "Задвижвано от Grist", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't these localisation strings be handled through Weblate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does weblate have issues when updating strings in the repo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, could you be more specific ? I don't understand 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean: does that cause an issue to update locales json files directly here in the repo? Like, weblate doesn't sync or something?
Because otherwise there is no value here to force weblate usage. The strings are not new, it's just a concatenation of the already-there strings "Powered by" and "Grist" (in the good order for rtl languages).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC, yes, it may cause synchronisation issues in Weblate.
c4816cc
to
df1ad79
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Except my comment about the changes in localization files, it sounds good to me. Thank you @manuhabitela!
Context
Here are small HTML changes on the user-generated forms, mostly made to improve the life of screen reader users.
Proposed solution
Each fix has his own contained commit with its description.
The most important fix is the one making sure form labels are tied to their inputs, as this makes screen reader navigation in forms much easier.
Other fixes are arguably not that important, but each improves the UX a tiny bit for screen reader users, making this PR as a whole a noticeable change.
Related issues
This doesn't fix everything. As forms are created by users, what someone does when authoring the form can have much impact on its accessibility. For example, the form widget name or the headings hierarchy need to be authored with care. We could imagine improving UX when creating forms to make people more aware of that.
One thing that would be great in the future would also to define the
lang
attribute on the html tag depending on the form locale. This would make sure that screen readers spell words in the correct language. This one thing is easy to code if relying on the document locale, but I fear that applying that as of right now might do more harm than good, because of how document locales are currently handled in Grist. So I'm more inclined to not change anything right now on this, this will require some specific work.Has this been tested?