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

a11y: role=navigation is not permitted on UL tag - causes failure on Section 508 scans #372

Closed
jamesuhl opened this issue Mar 4, 2021 · 9 comments

Comments

@jamesuhl
Copy link

jamesuhl commented Mar 4, 2021

Hi, thanks for contributing!

This project is maintained in my spare time, so in order to help me address your issue as quickly as
possible, please provide as much of the following information as you can.

-- Michael

[^ delete this message]

=======

Angular version:

ngx-pagination version:

Description of issue:

Steps to reproduce:

Expected result:

Actual result:

Demo: (if possible, edit this StackBlitz demo and paste the link to your fork)

Any relevant code:


@michaelbromley
Copy link
Owner

Hi,

Thanks for the report. Can you provide some more context?

What's a Section 508 scan? Is there an online tool that can reproduce the failure?

What are the implications of failing this scan?

Do you have a suggestion about a way to remedy the issue?

Thanks!

@alexkushnarov
Copy link
Contributor

alexkushnarov commented Mar 4, 2021

@michaelbromley I see Error in Lighthouse because of role=navigation

List items (<li>) are not contained within <ul> or <ol> parent elements.

GoogleChrome/lighthouse#10997 (comment)
GoogleChrome/lighthouse#9643

@alexkushnarov
Copy link
Contributor

@michaelbromley any update on this issue ?

to reproduce the issue just run lighthouse test on the page with pagination.

@michaelbromley
Copy link
Owner

@alexkushnarov I originally took the markup for the default template from Zurb Foundation. I just checked and it seems they changed their pagination to resolve this issue some years ago: foundation/foundation-sites#9334

So there are 2 options right now:

  1. I will happily accept a pull request which rectifies this
  2. You can use a custom template with valid markup

unfortunately I am not able to prioritise this work myself right now due to other commitments. If nobody PRs a fix, I will get around to it in time.

@alexkushnarov
Copy link
Contributor

@michaelbromley Please review my PR.

@jamesuhl
Copy link
Author

jamesuhl commented May 8, 2021

Hi. Sorry for such a late response, I'm not on GitHub a lot. I am a contractor for a government agency that is upgrading its website forms into Angular applications. All the web content we create has to be accessible to people with disabilities based on U.S. law, Section 508. The agency uses software called AMP (Accessibility Management Platform [ref: www.levelaccess.com]) which generates accessibility reports. We have to submit these reports to the government. AMP is aggressively thorough and flags any instance of invalid HTML or missing/improper use of role and aria- attributes in code. We use the ngx-pagination extensively on most datatables. So every single LI tag in every "view" that uses pagination gets flagged as an accessibility error (even though JAWS screen reader has no problem with ngx-pagination). The invalid HTML comes from the node_modules folder itself so I can't fix it at it's source. Since I'm new to Angular, I ended up with a brute force solution of involving querySelectorAll('.ngx-pagination') followed a forEach method to removeAttribute("role") and called this inside ngAfterViewInit(). But it's an ugly solution and I was hoping for an update so I can have the developers pull the latest version of ngx with the fix so I can remove my ugly solution. The culprit is inside the node_modules/ngx-pagination/ngx-pagination.umd.js, where the variable DEFAULT_TEMPLATE creates the UL here: "<ul class="ngx-pagination" \n role="navigation"..."
The role="navigation" should go on the outer container of the UL, or you can make this container a NAV tag with an aria-label="pagination", or something similar.

It looks like alexkushnarov jumped on this, so I would be so grateful if a newer version will let me remove my cumbersome solution and let your fantastic component stand on its own.

@michaelbromley
Copy link
Owner

@jamesuhl thanks for the extra info, that's what I was missing originally. Makes sense.

@alexkushnarov thank you for the PR, I'll merge and do a new release after the weekend.

@jamesuhl
Copy link
Author

jamesuhl commented May 9, 2021

Also, if role="navigation" is used anywhere, it's always supposed to be accompanied by an aria-label (the only exception is when there is only one role="navigation" on the entire page. Thank you again, we love this component.

@michaelbromley
Copy link
Owner

OK, available now in v5.1.0. 👍

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

No branches or pull requests

3 participants