AdeleD / react-paginate

A ReactJS component that creates a pagination
MIT License
2.75k stars 625 forks source link

Addition of role='navigation' to the <ul> element causes a Lighthouse failure #459

Open stevejay opened 1 year ago

stevejay commented 1 year ago

If I run Lighthouse on our pagination component, Lighthouse complains that List items (<li>) are not contained within <ul> or <ol> parent elements.:

Screenshot 2022-11-30 at 19 12 09

AFAIK the 'navigation' role overrides the natural 'list' role of the <ul> element.

The addition of this role was a recent change, and the release notes link to this page to explain its addition. However, that page shows the use of the navigation role as wrapping the <ul> element, not added to it:

<div role="navigation" aria-label="Customer service">
  <ul>
    <li><a href="#">Help</a></li>
    <li><a href="#">Order tracking</a></li>
    <li><a href="#">Shipping &amp; Delivery</a></li>
    <li><a href="#">Returns</a></li>
    <li><a href="#">Contact us</a></li>
    <li><a href="#">Find a store</a></li>
  </ul>
</div>

Is there some way to avoid this role being added? It would be preferable if it was configurable.

devinschulz commented 1 year ago

Getting a similar error when using axe:

 "<li> elements must be contained in a <ul> or <ol> (listitem)"

    Fix any of the following:
      List item does not have a <ul>, <ol> parent element without a role, or a role="list"

    You can find more information on this issue here:
    https://dequeuniversity.com/rules/axe/4.5/listitem?application=axeAPI

The solution @stevejay provided would fix this accessibility issue. Another option would be to wrap the unordered list in a nav tag and remove the role attribute altogether.

racingfoxs commented 1 year ago

Same problem facing!

How to I change/assign

  • role?

    <ReactPaginate nextLabel=">" onPageChange={handlePageClick} pageRangeDisplayed={3} marginPagesDisplayed={2} pageCount={pageCount} previousLabel="<" pageClassName="inline-flex items-center px-4 py-2 text-sm font-bold border dark:border-gray-700" pageLinkClassName="" previousClassName="inline-flex items-center px-6 py-2 text-sm font-bold border rounded-l-md dark:border-gray-700" previousLinkClassName="" nextClassName="inline-flex items-center px-6 py-2 text-sm font-bold border rounded-r-md dark:border-gray-700" nextLinkClassName="" breakLabel="..." breakClassName="" breakLinkClassName="" containerClassName="" activeClassName="inline-flex items-center px-4 py-2 text-sm font-semibold border dark:bg-violet-400 dark:text-gray-900 dark:border-gray-700" renderOnZeroPageCount={null} />

  • racingfoxs commented 1 year ago

    Update:- I was trying to fix myself and found a solution. Solution:- Wrap under <nav> Tag Having aria-label="Pagination". Example:-

    <nav aria-label="Pagination">
        <ReactPaginate/>
    </nav>
    amitt2202 commented 1 year ago

    The solution mentioned by @racingfoxs didn't work. the <ul> element still comes with role="navigation" which is complainaed by the axe tool. I did make the changes to the file "PaginationBoxView.js" present in .....\node_modules\react-paginate\react_components folder line number 566 where <ul> tag is added with role="navigation" is also not working. Can i request the developers to provide alternative release without role="navigation" tag for <ul> element? this makes it fail the accessibility test

    SLain123 commented 1 year ago

    The solution mentioned by @racingfoxs didn't work. the <ul> element still comes with role="navigation" which is complainaed by the axe tool. I did make the changes to the file "PaginationBoxView.js" present in .....\node_modules\react-paginate\react_components folder line number 566 where <ul> tag is added with role="navigation" is also not working. Can i request the developers to provide alternative release without role="navigation" tag for <ul> element? this makes it fail the accessibility test

    +1. I tried to added "nav" wrapper with arai-label, but problem still actual.

    TravelPaul commented 1 year ago

    Having same issue, looks like the role for a ul needs to be list. Also tried wrapping in nav but doesn't change that the role "navigation" is being applied to the ul, which is the issue according to axe.

    Axe test is saying:

        Fix any of the following:
          List item does not have a <ul>, <ol> parent element without a role, or a role="list"

    using "jest-axe": "^5.0.1", and "react-paginate": "^8.1.4",

    So the fix as @stevejay would solve this.

    mynamesleon commented 1 year ago

    Further to this, the aria-label on the generated <ul> isn't configurable. So the change that added this also leaves us with a hardcoded English "pagination" announcement, and without the ability to add additional attributes to that navigation landmark (such as aria-controls, if using the pagination for something that is dynamically updated).

    So I'm going to stick with 8.1.3 for now and wrap it in my own <nav> element that I can add aria-controls and a translated aria-label to.

    kubaf13 commented 1 year ago

    Hey Hi Hello, any updates how to fix this issue?

    amitt2202 commented 1 year ago

    Hey Hi Hello, any updates how to fix this issue?

    Please use the fork : https://github.com/amitt2202/react-paginate-fts.git For my project i have made the fix and updated!

    mynamesleon commented 1 year ago

    Hey Hi Hello, any updates how to fix this issue?

    @kubaf13 The simplest option right now is to stick with v8.1.3 and avoid any newer versions.

    Despite the above comment, although @amitt2202's fork has removed the role attribute that was causing the issue, they've also added a hardcoded aria-label="Pagination123" onto the generated <ul> which can't be easily overriden by a consuming app.

    quicksketch commented 1 year ago

    +1 to the PR at https://github.com/AdeleD/react-paginate/pull/494 that fixes this issue. Thanks @kroney for making it.

    For others stuck in the same situation, our project decided to use patch-package to modify the dist/react-paginate.js and react_components/PaginationBoxView.js files while we wait for this fix to be applied.

    thomcrielaard commented 11 months ago

    +1 to PR at #494