michaelbromley / ngx-pagination

Pagination for Angular
http://michaelbromley.github.io/ngx-pagination/
MIT License
1.21k stars 244 forks source link

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

Closed jamesuhl closed 3 years ago

jamesuhl commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

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

https://github.com/GoogleChrome/lighthouse/issues/10997#issuecomment-646630956 https://github.com/GoogleChrome/lighthouse/issues/9643

alexkushnarov commented 3 years ago

@michaelbromley any update on this issue ?

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

michaelbromley commented 3 years ago

@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: https://github.com/foundation/foundation-sites/issues/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 commented 3 years ago

@michaelbromley Please review my PR.

jamesuhl commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

OK, available now in v5.1.0. 👍