-
-
Notifications
You must be signed in to change notification settings - Fork 504
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: accessibility for filter labels #4046
Conversation
Before labels were using the wrong id parameter and were not correctly labeling select elements.
d54f140
to
7457aa8
Compare
In the off chance that there are two filters with the same scope on a page they would both have the same id. This adds a unique uuid value to the filters.
Previously systems tests would select filters by the id attribute, since ids are now followed by a uuid this is not possible. All the selections are now made using the name attribute.
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.
Had one question.
<%= label_tag "by Source" %> | ||
<%= select_tag "filters[by_source]", options_for_select(@sources, @selected_source), {include_blank: true, class: "form-control"} %> | ||
<% id = "filter_#{SecureRandom.uuid}" %> | ||
<%= label_tag id, "Filter by Source" %> |
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 think this changes the visible text? Was that on purpose?
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.
Looks good, thanks! |
@awwaiid Please note this for the dashboard? Thanks. (Edit. Never mind -- it doesn't apply after all.) |
@elasticspoon: Your PR |
Filters on various pages were not labeled correctly. Though they had labels
had the wrong
for=
attributes making them not functional for screen readers.This is an accessibility concern. This PR fixes the helper function to have the
correct id.
Note: my linter did some stuff but I only touched
app/helpers/filter_helper.rb
and a label tag in the donations index.
Before per lighthouse:
label for=
andselect id=
do not match, thus labels do not provide a11y correct function.After:
Type of change
How Has This Been Tested?
It passed all tests but I did not write any specifically for it.