Adding a11y tests and moving from whitelist to blacklist + misc fixes #3502

myasonik · 2020-05-19T17:23:13Z

Summary

This PR does 2 things.

Removes menu and menuitem roles from KeyPadMenu and KeyPadMenuItem.
- These roles are super specific and shouldn't be used for generic lists of links. We also weren't implementing any of the expected keyboard functionality for them so they weren't technically complete either.
Changes our a11y testing strategy from a whitelist of pages to test to a blacklist of pages to skip.
- We weren't adding new pages that we were adding to the automated testing making it increasingly difficult to catch up. Switching to a blacklist means that new pages are automatically tested.
- Added several new pages to the tests: Header, Key Pad Menu, Data Grid Focus, Comment List, Image, Tour, Filter Group, Beacon, Highlight and Mark, HTML ID Generator, Overlay Mask, Changelog (Making progress on [Meta][Docs] Add automated tests for all pages #2679)

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-05-19T17:52:13Z

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

cchaos · 2020-05-20T12:56:17Z

The title of this PR could be more specific.

Removes menu and menuitem roles from KeyPadMenu and KeyPadMenuItem.

I think this is fine and agree that it's not correct for all implementations. I think we should also then add some accessibility guidelines to their docs page around what to add if:
a. It's used as the primary navigation for the site/app
b. It's used in a menu-like dropdown with selection (there's a selected item)

Changes our a11y testing strategy from a whitelist of pages to test to a blacklist of pages to skip.

🙏 Thank you! This makes way more sense and I'm definitely in the group of people who forget to add to this list.

myasonik · 2020-05-20T17:42:17Z

The title of this PR could be more specific.

😆 The title of the PR was terrible. My bad, it's better now.

we should also then add some accessibility guidelines

I added a note about what to do if it's a bunch of links but I couldn't think of any good broad guidelines that I could give about the buttons use case. Anything I could think of had plenty of exceptions so I just didn't mention it. I don't think anything should be super broken if they don't do anything but it could be if they do the wrong thing so I thought rather overly simplified is better than misleading.

kibanamachine · 2020-05-20T18:05:03Z

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

CHANGELOG.md

src-docs/src/components/guide_page/guide_page_chrome.js

src-docs/src/views/header/header.js

src-docs/src/views/header/header_spaces_menu.js

src-docs/src/views/header/header_user_menu.js

src-docs/src/views/key_pad_menu/key_pad_menu_example.js

src/components/key_pad_menu/_key_pad_menu.scss

src/components/key_pad_menu/key_pad_menu_item.tsx

cchaos · 2020-05-26T18:45:04Z

src/components/key_pad_menu/key_pad_menu_item.test.tsx

@@ -87,7 +87,7 @@ describe('EuiKeyPadMenuItem', () => {
      </EuiKeyPadMenuItem>
    );

-    $button.simulate('click');
+    $button.find('button').simulate('click');


To make the test read more appropriately can you change the const name to $item instead of $button?

This also might break downstream app tests, do we then consider this a breaking change @chandlerprall ?

This was reverted by moving the <li> wrapping element to the parent component

Somehow it still exists as changed?

@myasonik Can you remove this change, it shouldn't be necessary anymore.

Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>

kibanamachine · 2020-05-27T20:31:28Z

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

src-docs/src/views/key_pad_menu/key_pad_menu_example.js

kibanamachine · 2020-05-27T21:18:02Z

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

kibanamachine · 2020-05-28T18:46:19Z

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

kibanamachine · 2020-06-03T20:24:33Z

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

kibanamachine · 2020-06-03T20:58:54Z

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

cchaos · 2020-06-03T21:05:26Z

Looks like there's another/new a11y issue:

16:51:29 Errors on tp://localhost:9999/#/layout/header
16:51:29 [landmark-unique]: Landmarks must have a unique role or role/label/title (i.e. accessible name) combination
16:51:29   Help: https://dequeuniversity.com/rules/axe/3.4/landmark-unique?application=axe-puppeteer
16:51:29   Elements:
16:51:29     - .euiHeaderSectionItem--borderLeft.euiHeaderSectionItem > .euiHeaderLinks[aria-label="App\ navigation"]
16:51:29     - .euiHeaderSectionItem--borderLeft.euiHeaderSectionItem > .euiHeaderLinks[aria-label="App\ navigation"] > .euiHeaderLinks__list[role="navigation"]
16:58:54 1 accessibility errors

Probably my bad...

kibanamachine · 2020-06-03T22:32:34Z

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

cchaos

Thanks! LGTM!

Michail Yasonik added 2 commits May 19, 2020 13:09

simplifying KeyPadMenu, KeyPadMenuItem semantics

3c6d066

switching a11y tests to a blacklist instead of a whitelist

ba9fdd2

myasonik added chore accessibility testing Issues or PRs that only affect tests - will not need changelog entries labels May 19, 2020

adding changelog

aa284cf

myasonik changed the title ~~Adding tests~~ Adding a11y tests and moving from whitelist to blacklist + misc fixes May 20, 2020

adding a11y tip to keypadmenu and wrapping in a UL

e90df87

cchaos suggested changes May 26, 2020

View reviewed changes

Michail Yasonik and others added 2 commits May 27, 2020 16:06

PR cleanup

6036ac1

Update EUI nav name

34c06c1

Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>

cchaos reviewed May 27, 2020

View reviewed changes

src-docs/src/views/key_pad_menu/key_pad_menu_example.js Outdated Show resolved Hide resolved

moving key pad menu a11y note

802e1a6

Merge remote-tracking branch 'upstream/master' into adding-tests

2215332

myasonik requested a review from cchaos May 29, 2020 21:24

removing redudant find in tests

36a6567

Merge remote-tracking branch 'upstream/master' into adding-tests

56d133a

fixing a11y errors in header examples

7f7762c

cchaos approved these changes Jun 4, 2020

View reviewed changes

cchaos merged commit a6e49f0 into elastic:master Jun 4, 2020

myasonik mentioned this pull request Jun 4, 2020

[Meta][Docs] Add automated tests for all pages #2679

Closed

26 tasks

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

Adding a11y tests and moving from whitelist to blacklist + misc fixes #3502

Adding a11y tests and moving from whitelist to blacklist + misc fixes #3502

myasonik commented May 19, 2020 •

edited

Loading

kibanamachine commented May 19, 2020

cchaos commented May 20, 2020

myasonik commented May 20, 2020 •

edited

Loading

kibanamachine commented May 20, 2020

cchaos May 26, 2020

cchaos May 26, 2020

myasonik May 27, 2020

cchaos May 27, 2020

cchaos Jun 1, 2020

kibanamachine commented May 27, 2020

kibanamachine commented May 27, 2020

kibanamachine commented May 28, 2020

kibanamachine commented Jun 3, 2020

kibanamachine commented Jun 3, 2020

cchaos commented Jun 3, 2020 •

edited

Loading

kibanamachine commented Jun 3, 2020

cchaos left a comment

Adding a11y tests and moving from whitelist to blacklist + misc fixes #3502

Adding a11y tests and moving from whitelist to blacklist + misc fixes #3502

Conversation

myasonik commented May 19, 2020 • edited Loading

Summary

Checklist

kibanamachine commented May 19, 2020

cchaos commented May 20, 2020

myasonik commented May 20, 2020 • edited Loading

kibanamachine commented May 20, 2020

cchaos May 26, 2020

Choose a reason for hiding this comment

cchaos May 26, 2020

Choose a reason for hiding this comment

myasonik May 27, 2020

Choose a reason for hiding this comment

cchaos May 27, 2020

Choose a reason for hiding this comment

cchaos Jun 1, 2020

Choose a reason for hiding this comment

kibanamachine commented May 27, 2020

kibanamachine commented May 27, 2020

kibanamachine commented May 28, 2020

kibanamachine commented Jun 3, 2020

kibanamachine commented Jun 3, 2020

cchaos commented Jun 3, 2020 • edited Loading

kibanamachine commented Jun 3, 2020

cchaos left a comment

Choose a reason for hiding this comment

myasonik commented May 19, 2020 •

edited

Loading

myasonik commented May 20, 2020 •

edited

Loading

cchaos commented Jun 3, 2020 •

edited

Loading