-
Notifications
You must be signed in to change notification settings - Fork 644
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
[A11y] Correct role attribute for package lists #9303
Conversation
This view is shared by the main search page. Could you run FastPass on that page too? Looks good by the way. It sort of makes sense. It's a list of package results :) |
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.
Check FastPass on other pages that use this shared view then
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.
Same as what @joelverhagen said. Let's run a pass over the other places that share this view, and we are good to go. Looks great!
@@ -41,7 +41,7 @@ | |||
<section role="main" class="container main-container page-list-packages"> | |||
<div class="row clearfix no-margin"> | |||
<div class="col-md-10 no-padding"> | |||
<h1 role="alert"> | |||
<h1 role="heading"> |
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.
Is role="heading"
necessary for h1 tags?
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.
You're right, it wasn't necessary, the screen reader announces h1 tags as headings anyway. I removed the header role from this element. I also added tabindex=0 to this element so that screen readers can read the heading, which they were skipping previously.
Addresses #9301
Problem:
On the Profiles page and the List Packages page, the lists of packages were flagged for not having the appropriate role on the individual list items. These were already marked with role="listitem" and the enclosing div was marked with role="list", so I'm unsure why this was flagged. However, we can bypass the need for these manual role attributes by using
<ul>
and<li>
tags directly instead.Previously,
Fix:
These package lists now use
<ul>
and<li>
tags, which do not require additional role attributes. I also had to remove the bullet and left padding from these package list<ul>
s.After the changes,