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

[EuiPagination] Accessibility issues #2196

Closed
myasonik opened this issue Aug 2, 2019 · 6 comments · Fixed by #3294
Closed

[EuiPagination] Accessibility issues #2196

myasonik opened this issue Aug 2, 2019 · 6 comments · Fixed by #3294

Comments

@myasonik
Copy link
Contributor

myasonik commented Aug 2, 2019

I think our current pagination component could be improved. Going to dump several ideas here, many of which can be mix-and-matched with what currently exists and a lot of it is up for debate.

Draft markup:

<nav aria-lablledby={uniqueId} aria-controls={tableId}>
  <h# id={uniqueId} class="visually-hidden">Pagination for table</h#>
  <button aria-label="Go to previous page, 2"> « </button>
  <ul>
    <li><button><span class="visually-hidden">Page</span> 1</button></li>
    <li><button><span class="visually-hidden">Page</span> 2</button></li>
    <li>
		<button aria-current="page" disabled>
			<span class="visually-hidden">Page</span> 3
		</button>
	</li>
    <li><button><span class="visually-hidden">Page</span> 4</button></li>
	<li><button><span class="visually-hidden">Page</span> 5</button></li>
    <li><button><span class="visually-hidden">Page</span> 6</button></li>
  </ul>
	<button aria-label="Go to next page, 4"> » </button>
</nav>

Benefits:

  1. <nav> landmark makes it easier to navigate to. Though not well supported, some screen readers will let users easily navigate back to the table thanks to the addition of aria-controls. aria-labelledby will make it easy to find in rotor-like functionality.
  2. <h#> makes it easier to navigate to as well. Not all screen reader users use landmarks to navigate. The addition of the heading makes it easier for those who don't. The big question here is what level heading to make it.*
  3. "Previous" and "Next" buttons call out what page they will navigate to.
  4. Putting the pages in a list means we don't have to maintain text like "Page 1 of 5" ourselves.
  5. Using aria-current tell the screen reader what page it's currently on. Using aria-current="page" or aria-current="location" is up for debate but either is probably fine. Also marking that button as disabled sends a hit to users with a screen reader that doesn't support aria-current. Could also add more hidden text to fill that gap.

* Maybe implementing developer can configure this? Or, it can try to intelligently guess based off reading the DOM from the table maybe? Though that probably becomes much slower. Also, in a real dream world of accessible content, we could parse the the table's header (because every table will have one) and use "Pagination for {tableHeader} table" instead of the more generic heading.

@snide
Copy link
Contributor

snide commented Aug 2, 2019

<h#> makes it easier to navigate to as well. Not all screen reader users use landmarks to navigate. The addition of the heading makes it easier for those who don't. The big question here is what level heading to make it.*

My suggestion here would be to start with H6 as default and have make a prop to define it. ariaHeading="h2" or something.

This is tangential, but the bigger issue is we need a landmark / hidden system in general for EUI. It will require a lot of thought, because I could see it requiring context or something in Kibana.

@barlowm
Copy link

barlowm commented Sep 17, 2019

Only problem I have with this concept is the arbitrary assignment of a heading tag to the controls.
The control is not a "structural" element which is what the heading tags are used for. And if a screenreader user were to try navigating through the heading tags they would come up with a list of "heading level X Pagination for table" (though if you added the table name as identified by the caption that would give them a better idea of where they were at. But then they'd still be at a control to navigate to a page in a table that they would then have to "find" on the page.

aria-controls is a nice concept but implementation leaves much to be desired.

@myasonik
Copy link
Contributor Author

Yeah, aria-controls is there mostly as a "doesn't hurt".

Will circle back tomorrow on where I saw the guidance to include a heading in something like this but at a glance I'd be ok ditching it, I think.

@myasonik
Copy link
Contributor Author

Couldn't find a source so I think I just made it up. Reading through the Providing heading elements at the beginning of each section of content page though makes me think it's still a good idea. The closes it gets to explicitly mentioning this case is mentioning including headings for "secondary navigation".

If someone is navigating a page using headings instead of landmarks, I think it makes sense to include a heading for each (or, at least, most?) landmarks on the page. In this case, a user navigating by landmarks can go to the nav element whenever they want to; and, a user navigating by headings can go the h# within it.

@anishagg17
Copy link
Contributor

@myasonik is it still open?

@myasonik
Copy link
Contributor Author

myasonik commented Mar 9, 2020

Yup! Though I'll warn you my proposal up there was a bit of a draft so there may be some rounds of revisions it has to go through when it's up in PR and I test it more fully.

Also, I'll give you a tip, that it'll be much easier to merge this if you can avoid making any breaking changes to the component API.

Thanks!

@cchaos cchaos added this to Not started in Accessibility Mar 18, 2020
@cchaos cchaos changed the title Pagination accessibility [EuiPagination] Accessibility issues Mar 18, 2020
@cchaos cchaos added the meta label Mar 18, 2020
Accessibility automation moved this from Not started to Closed Jun 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Accessibility
  
Closed
Development

Successfully merging a pull request may close this issue.

5 participants