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 wcag search filters navigation #1240

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

BarbaraOliveira13
Copy link

@BarbaraOliveira13 BarbaraOliveira13 commented Dec 20, 2024

🎩 What?

Updated the search filters container to improve accessibility compliance. Changes include:

  1. Add a <nav> element with role="navigation" to define the navigation region.
  2. Add an aria-labelledby attribute referencing a hidden <h1> to programmatically name the navigation area for screen readers.

🎩 Why?

These changes address accessibility concerns highlighted during the city of Lyon rgaa audit, and align with WCAG 2.1 standards:

🔗 WCAG References:

  1. 1.3.1 - Info and Relationships: Ensures information and relationships are perceivable for assistive technologies.
  2. 4.1.2 - Name, Role, Value: Provides programmatically determinable names and roles for interactive elements.

🎩 Related to

In grist, array Diagnostique Decidim, line 31

📋 Subtasks

  • Add CHANGELOG entry.
  • If there's a new public field, add it to GraphQL API.
  • Add documentation regarding the feature.
  • Add/modify seeds.
  • Add tests.
  • Verify compliance with WCAG and visually validate changes in a local environment.

📷 Screenshots (optional)

Capture d’écran 2024-12-23 à 15 59 00

@BarbaraOliveira13 BarbaraOliveira13 changed the title fix wcag search filters navigation Fix wcag search filters navigation Dec 22, 2024
@BarbaraOliveira13 BarbaraOliveira13 marked this pull request as ready for review December 23, 2024 15:07
Copy link

@AyakorK AyakorK left a comment

Choose a reason for hiding this comment

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

The changes look great to me, well done!

I do have a small point of consideration regarding the third line of _filters.html.erb, specifically the default value. It may not be strictly necessary, but it seems like a good safeguard against potential errors.

@BarbaraOliveira13
Copy link
Author

The changes look great to me, well done!

I do have a small point of consideration regarding the third line of _filters.html.erb, specifically the default value. It may not be strictly necessary, but it seems like a good safeguard against potential errors.

Thanks for the feedback! I agree that the default: "Search filters" parameter might not always be necessary if the key decidim.searches.filters.title is always present. Do you think it’s better to keep it as a safeguard or remove it for simplicity, @Quentinchampenois ?

Copy link

@Quentinchampenois Quentinchampenois left a comment

Choose a reason for hiding this comment

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

It's hard for me to understand changes,

The nav tag seems good with the right role and the aria-labelledby. However the value of aria-labelledby should match the ID of the currently existing H1 title (cf: 32 Results for the search: "" )

We shall not create a new h1 tag but add an ID to the existing H1 to match it

<span><%= searchable_resource_human_name(type) %></span>
<nav role="navigation" aria-labelledby="filters-title">
<h1 id="filters-title" class="sr-only">
<%= t("decidim.searches.filters.title", default: "Search filters") %>

Choose a reason for hiding this comment

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

I agree, the default parameter is not necessary in this case

Suggested change
<%= t("decidim.searches.filters.title", default: "Search filters") %>
<%= t("decidim.searches.filters.title") %>

Copy link
Author

Choose a reason for hiding this comment

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

Good point! The changes have been implemented for better dynamic approach

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants