a11y rework of [EuiPagination] and friends #3294

myasonik · 2020-04-10T00:15:29Z

Summary

Closes EuiPagination needs to better manage focus when clicking on numbers #3244
- "Fixed" by moving focus away from the pagination after clicking
Closes [EuiPagination] Accessibility issues #2196
- Largely followed my initial draft structure
- Will make a new issue for adding a header element instead of relying on aria-* attributes
- May make a new issue for compressed pagination which I entirely avoided in this PR

EuiPagination now sports the following changes:

Everything is wrapped in a <nav> and the pages are in a <ul>
Renders as <a> (instead of <button>) if there is an id to point to (similar idea to a skip link using <a>)
- If this is the case, will move focus to the id
- Also adds aria-controls={id}
If the ellipses would only replace 1 page, renders the page number
Adds more context to prev and next pagination buttons of which page they will be going to for screen readers
Adds more context to the ellipses for screen readers
Adjusts spacing of ellipses to match page numbers

EuiBasicTable, EuiInMemoryTable and EuiDataGrid pass in aria-controls to their pagination to take full advantage of the new structure.

Breaking change

EuiPaginationButton now requires a pageIndex
- Shouldn't be a huge deal because I don't think there's a real use case to use it outside of EUI

Checklist

Check against all themes for compatibility in both light and dark modes
Checked in mobile
Checked in IE11 and Firefox
Props have proper autodocs
~~- [ ] Added documentation examples~~
Added or updated jest tests
Checked for breaking changes and labeled appropriately
Checked for accessibility including keyboard-only and screenreader modes
A changelog entry exists and is marked appropriately

kibanamachine · 2020-04-10T00:36:23Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_3294/

kibanamachine · 2020-04-10T00:59:58Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_3294/

kibanamachine · 2020-04-10T01:40:36Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_3294/

cchaos

Mostly doing a quick functionality review:

Focus state

Do we really need a focus ring on the entire table after each page page selection? It seems excessive visually as users page through.

Perhaps you can link to some guidelines?

Compressed version

These numbers shouldn't be buttons.

Truncating / Number display

I don't think this is the fault of this PR, but the physical changes of this thing's width and position of the back button really concerns me. It shifts a lot and if the user is trying to use the previous arrow continuously, they have continuously move their mouse.

cchaos · 2020-04-10T13:01:25Z

src/components/pagination/_pagination_button.scss

  font-weight: $euiFontWeightBold;
+
+  &#{&} {


Why this crazy selector? It just concatenates the original selector to itself like: .euiPaginationButton-isActive.euiPaginationButton-isActive. It seems like a hacky way to get specificity.

It seems like a hacky way to get specificity.

That's exactly what it is 😆 First saw it many years ago on CSS Wizardry

Sounds like it's not a pattern in EUI so I can switch it to something else

I wasn't sure how to best fix this without hacks... Do you have a suggestion?

src/components/pagination/_pagination_button.scss

src/components/datagrid/data_grid.tsx

snide · 2020-04-10T15:33:08Z

Do we really need a focus ring on the entire table after each page page selection? It seems excessive visually as users page through.

This is excessive, at the point of distraction, which is its own top level concern. I know I say this a lot, but there is a balance. This is a pattern I do not see regularly on even high accessibility websites.

I think ultimately only sometimes do you want to jump focus to the table itself. Sometimes you just want to continue tabbing through pagination.

chandlerprall

Reviewed the top-level usages of these changes (e.g. tables code), will wait on resolutions to existing conversations before really looking at pagination bar/button changes.

src/components/basic_table/basic_table.tsx

src/components/basic_table/in_memory_table.test.tsx

src/components/basic_table/pagination_bar.tsx

src/components/common.ts

chandlerprall · 2020-04-14T18:20:50Z

Totally agree with Dave's assessment of that focus jumping

myasonik · 2020-04-14T23:52:53Z

All the comments here seem reasonable and it's good to move forward but moving this PR back to a draft as I need to step away from it for an indefinite while to focus on some Kibana work.

myasonik · 2020-05-07T18:38:54Z

Don't think I can fully get back to this PR right now but wanted to merge master into it so it doesn't fall too far behind.

I addressed just the easiest PR feedback while I had it open.

Big change (that was easy to do) is I also removed focus ring from tables. Turns out, visible focus is only required on controls (things you can interact with) which is why things like skip links don't trigger giant visible focus rings around the entire main contents of the page.

How do folks feel about moving focus to the table with the giant focus ring gone?

kibanamachine · 2020-05-07T18:59:16Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_3294/

myasonik · 2020-05-28T21:32:48Z

Have more time now so picking this back up.

To reiterate my previous question:

I also removed focus ring from tables. Turns out, visible focus is only required on controls (things you can interact with) which is why things like skip links don't trigger giant visible focus rings around the entire main contents of the page.

How do folks feel about moving focus to the table with the giant focus ring gone?

kibanamachine · 2020-05-28T21:52:07Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_3294/

snide

Talked over slack, but posting here so others can read. Functionally this works great.

How do folks feel about moving focus to the table with the giant focus ring gone?

Yes. That's fine. It works well in practice from what I see.

I made a quick video to go over some finer detail stuff with the table / data grid specifically in concert with the pagination controls. @myasonik and I talked about changing the caption and aria-label on the table/data-grid to better announce the pagination state. He's gonna try that out, otherwise from a functional perspective this looks 👍 to me.

http://snid.es/fb2bc2eb5db4

src/components/basic_table/basic_table.tsx

src/components/pagination/pagination.tsx

myasonik · 2020-06-01T23:16:23Z

Added page state info to the table <caption> and grid aria-label/aria-labelledby.

If you watch Dave's video above, you'll see the table gets reread linearly when focus is moved to it. Did some testing and research and that seems to be expected.

Also tested in VO+Safari and it seems to read the caption like 50% of the time and move focus to the wrong element the other 50% so I think we're into screen reader/browser discrepancies at this point.

Windows screen reader testing for grid is on my to-do list (not planned for this PR, like within the next ~6 months) so I'll review this then as well.

chandlerprall

Changes so far LGTM; latest build failure was a known flaky unit test

myasonik · 2020-06-02T15:11:50Z

retest

snide

@myasonik and I noticed some browser inconsistencies, but they aren't the end of the world. I'm ok with this as it stands for merging.

kibanamachine · 2020-06-02T22:28:02Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_3294/

Mistakenly put pagination changes (#3294) under wrong version

myasonik added breaking change accessibility labels Apr 10, 2020

a11y rework of pagination

d9d8fa3

adding changelong entry

abf23a9

myasonik force-pushed the a11y/pagination branch from d9b81f3 to abf23a9 Compare April 10, 2020 00:38

Fix axe tests

2df9979

cchaos reviewed Apr 10, 2020

View reviewed changes

This comment has been minimized.

Sign in to view

myasonik commented Apr 10, 2020

View reviewed changes

src/components/datagrid/data_grid.tsx Show resolved Hide resolved

chandlerprall suggested changes Apr 14, 2020

View reviewed changes

myasonik marked this pull request as draft April 14, 2020 23:52

Michail Yasonik added 2 commits May 7, 2020 12:57

Merge remote-tracking branch 'upstream/master' into a11y/pagination

4cce167

fixing simple PR feedback

68fbd79

Michail Yasonik added 3 commits May 28, 2020 14:32

Merge remote-tracking branch 'upstream/master' into a11y/pagination

c419e59

test snapshot updates

1a39b16

addressing PR feedback

2c9e325

myasonik marked this pull request as ready for review May 28, 2020 21:31

myasonik requested review from chandlerprall, cchaos and snide May 29, 2020 21:24

snide reviewed May 29, 2020

View reviewed changes

adding paging info

8aa3c85

chandlerprall suggested changes Jun 1, 2020

View reviewed changes

src/components/basic_table/basic_table.tsx Outdated Show resolved Hide resolved

src/components/pagination/pagination.tsx Outdated Show resolved Hide resolved

adding paging info to data grid

8962ba4

myasonik requested review from chandlerprall and snide June 1, 2020 23:16

chandlerprall reviewed Jun 2, 2020

View reviewed changes

chandlerprall approved these changes Jun 2, 2020

View reviewed changes

deduping i18n keys

ac31c50

snide approved these changes Jun 2, 2020

View reviewed changes

deduping i18n keys

638ba52

myasonik merged commit 2859b52 into elastic:master Jun 3, 2020

myasonik deleted the a11y/pagination branch June 3, 2020 19:56

myasonik pushed a commit that referenced this pull request Jun 3, 2020

Updating CHANGELOG

92da7c7

Mistakenly put pagination changes (#3294) under wrong version

thompsongl mentioned this pull request Jun 29, 2020

Upgrade EUI to v26.3.1 elastic/kibana#70243

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

a11y rework of [EuiPagination] and friends #3294

a11y rework of [EuiPagination] and friends #3294

myasonik commented Apr 10, 2020 •

edited

Loading

kibanamachine commented Apr 10, 2020

kibanamachine commented Apr 10, 2020

kibanamachine commented Apr 10, 2020

cchaos left a comment

cchaos Apr 10, 2020

myasonik Apr 10, 2020

myasonik May 28, 2020

This comment has been minimized.

snide commented Apr 10, 2020

chandlerprall left a comment

chandlerprall commented Apr 14, 2020

myasonik commented Apr 14, 2020

myasonik commented May 7, 2020

kibanamachine commented May 7, 2020

myasonik commented May 28, 2020 •

edited

Loading

kibanamachine commented May 28, 2020

snide left a comment

myasonik commented Jun 1, 2020 •

edited

Loading

chandlerprall left a comment

myasonik commented Jun 2, 2020

snide left a comment

kibanamachine commented Jun 2, 2020

a11y rework of [EuiPagination] and friends #3294

a11y rework of [EuiPagination] and friends #3294

Conversation

myasonik commented Apr 10, 2020 • edited Loading

Summary

Breaking change

Checklist

kibanamachine commented Apr 10, 2020

kibanamachine commented Apr 10, 2020

kibanamachine commented Apr 10, 2020

cchaos left a comment

Choose a reason for hiding this comment

Focus state

Compressed version

Truncating / Number display

cchaos Apr 10, 2020

Choose a reason for hiding this comment

myasonik Apr 10, 2020

Choose a reason for hiding this comment

myasonik May 28, 2020

Choose a reason for hiding this comment

This comment has been minimized.

snide commented Apr 10, 2020

chandlerprall left a comment

Choose a reason for hiding this comment

chandlerprall commented Apr 14, 2020

myasonik commented Apr 14, 2020

myasonik commented May 7, 2020

kibanamachine commented May 7, 2020

myasonik commented May 28, 2020 • edited Loading

kibanamachine commented May 28, 2020

snide left a comment

Choose a reason for hiding this comment

myasonik commented Jun 1, 2020 • edited Loading

chandlerprall left a comment

Choose a reason for hiding this comment

myasonik commented Jun 2, 2020

snide left a comment

Choose a reason for hiding this comment

kibanamachine commented Jun 2, 2020

myasonik commented Apr 10, 2020 •

edited

Loading

myasonik commented May 28, 2020 •

edited

Loading

myasonik commented Jun 1, 2020 •

edited

Loading