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.

Sign up for GitHub

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

Jump to bottom

[ML] Adds a11y tests for ML plugin #88323

Merged
merged 2 commits into from
Jan 15, 2021
Merged

[ML] Adds a11y tests for ML plugin #88323

merged 2 commits into from
Jan 15, 2021

Conversation

peteharverson
Copy link
Contributor

@peteharverson peteharverson commented Jan 14, 2021 •
edited
Loading

Summary

Adds accessibility tests for the ML UI. Part of #51456.

This PR adds coverage for most of the key pages in the ML plugin:

  • Overview page
  • Anomaly detection jobs list
  • Anomaly detection job wizard (single metric wizard in this initial PR)
  • Single Metric Viewer
  • Anomaly Explorer (test is skipped awaiting the fix for fix(legend): remove ids for circles elastic-charts#973 being in Kibana)
  • Data frame analytics jobs list
  • Data frame analytics job wizard (outlier job in this initial PR)
  • Data frame analytics exploration page (outlier job results in this PR)
  • Settings
  • Manage calendars
  • Edit calendar
  • Manage filter lists
  • Edit filter list
  • Index data visualizer
  • File data visualizer

Resolves #79352

Checklist

Delete any items that are not applicable to this PR.

  • Unit or functional tests were updated or added to match the most common scenarios
  • Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
  • Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)

@peteharverson peteharverson requested a review from a team as a code owner January 14, 2021 12:53
@peteharverson peteharverson self-assigned this Jan 14, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-accessibility (Project:Accessibility)

@peteharverson peteharverson mentioned this pull request Jan 14, 2021
Open
77 tasks
pheyos
pheyos reviewed Jan 14, 2021
View reviewed changes
myasonik
myasonik approved these changes Jan 14, 2021
View reviewed changes
Copy link
Contributor

@myasonik myasonik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great addition! 🚀

x-pack/plugins/ml/public/application/components/data_grid/data_grid.tsx Outdated
Comment on lines 312 to 313
aria-checked={chartsVisible}
role="checkbox"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of changing the role of this to a checkbox let's make this a toggle button instead

Suggested change
aria-checked={chartsVisible}
role="checkbox"
aria-pressed={chartsVisible}

By not changing the role, we have a lot less risk in running into interop bugs between browsers, OSes, and assistive tech.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switched to aria-pressed in 55ed796

x-pack/plugins/ml/public/application/components/multi_select_picker/multi_select_picker.tsx
@@ -100,7 +100,6 @@ export const MultiSelectPicker: FC<{
<EuiFilterGroup data-test-subj={dataTestSubj}>
<EuiPopover
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not something that automated tests will pick, and we do hope to change the default in EUI at some point in the future, but, in the mean time, can we set this popover to ownFocus?

This helps keyboard-only and screen-reader users navigate to and through the popover more quickly and easily.

Suggested change
<EuiPopover
<EuiPopover
ownFocus

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added ownFocus in 55ed796

bhavyarm
bhavyarm approved these changes Jan 15, 2021
View reviewed changes
Copy link
Contributor

@bhavyarm bhavyarm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - ran it in my local. This is an AWESOME first pass at ML app.

walterra
walterra approved these changes Jan 15, 2021
View reviewed changes
Copy link
Contributor

@walterra walterra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🥳

pheyos
pheyos approved these changes Jan 15, 2021
View reviewed changes
Copy link
Member

@pheyos pheyos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - great to have those tests in 🎉

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
ml 7.1MB 7.1MB -15.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@peteharverson peteharverson merged commit c0a833d into elastic:master Jan 15, 2021
@peteharverson peteharverson deleted the ml-a11y-add-tests branch January 15, 2021 16:59
@peteharverson peteharverson mentioned this pull request Jan 15, 2021
Closed
peteharverson added a commit to peteharverson/kibana that referenced this pull request Jan 15, 2021
@peteharverson
[ML] Adds a11y tests for ML plugin (elastic#88323)
3c8aa88 
* [ML] Adds a11y tests for ML plugin

* [ML] Edits to a11y tests following review
peteharverson added a commit to peteharverson/kibana that referenced this pull request Jan 19, 2021
@peteharverson
[ML] Adds a11y tests for ML plugin (elastic#88323)
4befe53 
* [ML] Adds a11y tests for ML plugin

* [ML] Edits to a11y tests following review
@peteharverson peteharverson mentioned this pull request Jan 19, 2021
Merged
@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Jan 19, 2021
@walterra walterra mentioned this pull request Jan 20, 2021
Closed
9 tasks
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Jan 20, 2021
peteharverson added a commit that referenced this pull request Jan 20, 2021
@peteharverson @kibanamachine
[ML] Adds a11y tests for ML plugin (#88323) (#88698)
2e1ded7 
* [ML] Adds a11y tests for ML plugin

* [ML] Edits to a11y tests following review

Co-authored-by: Kibana Machine <[email protected]>
@peteharverson peteharverson mentioned this pull request Jan 27, 2021
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@walterra walterra walterra approved these changes

@pheyos pheyos pheyos approved these changes

@myasonik myasonik myasonik approved these changes

@bhavyarm bhavyarm bhavyarm approved these changes

Assignees

@peteharverson peteharverson

Labels
:ml Project:Accessibility release_note:skip Skip the PR/issue when compiling release notes review v7.12.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[ML] Add a11y tests
7 participants
@peteharverson @elasticmachine @kibanamachine @walterra @pheyos @myasonik @bhavyarm