Dashboard a11y tests by majagrubic · Pull Request #58122 · elastic/kibana

majagrubic · 2020-02-20T13:53:17Z

Summary

Continuation of: #57821

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Documentation was added for features that require explanation or tutorials
Unit or functional tests were updated or added to match the most common scenarios
This was checked for keyboard-only and screenreader accessibility
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser
This was checked for cross-browser compatibility, including a check against IE11

For maintainers

This was checked for breaking API changes and was labeled appropriately

majagrubic · 2020-02-21T09:19:05Z

There's still that one failure and adding main didn't help:
https://dequeuniversity.com/rules/axe/3.5/bypass?application=axeAPI

Other than that, this should be good to go.

myasonik · 2020-02-20T23:09:40Z

...ibana/public/discover/np_ready/angular/doc_table/components/pager/tool_bar_pager_buttons.tsx

+        aria-label={i18n.translate(
+          'kbn.discover.docTable.pager.toolbarPagerButtons.previousButtonAriaLabel',
+          {
+            defaultMessage: 'previous page button',


Suggested change

defaultMessage: 'previous page button',

defaultMessage: 'Previous page in table',

I'd suggest cutting the word "button" because the type of control will be communicated by the assistive tech.

And I wanted to clarify what "previous page" means because that could also mean it does something similar to a browser back button. "in table" is still kind of vague but this UI will be changing soon so I don't think we need to invest too much into it to make it perfect.

myasonik · 2020-02-20T23:10:17Z

...ibana/public/discover/np_ready/angular/doc_table/components/pager/tool_bar_pager_buttons.tsx

+        aria-label={i18n.translate(
+          'kbn.ddiscover.docTable.pager.toolbarPagerButtons.nextButtonAriaLabel',
+          {
+            defaultMessage: 'next page button',


Suggested change

defaultMessage: 'next page button',

defaultMessage: 'Next page in table',

Same as "previous page button" above

myasonik · 2020-02-20T23:27:22Z

src/legacy/core_plugins/input_control_vis/public/components/vis/list_control.tsx

        onChange={this.handleOnChange}
        singleSelection={!this.props.multiselect}
        data-test-subj={`listControlSelect${this.props.controlIndex}`}
+        inputRef={this.setTextInputRef}


So I'm not 100% sure where this is getting rendered so I wasn't able to test it but while poking around this pointed to a much larger issue (that I think this is covering up) of, why is so much of dashboard wrapped in a aria-hidden="true" and with a screen reader only warning saying it's not accessible?

This is concerning on several levels...

So, (almost?) all visualizations have area-hidden=true attribute. Their DOM structure is usually just a bunch of divs and an svg. Input control visualization, however, has this inner input field, which was focusable before and the test was failing:

What do you mean that much of dashboard is wrapped in "area-hidden=true"? If the point here is that dashboard is basically useless when all the visualizations are hidden in accessibility mode, I completely agree. I think it doesn't make much sense to show the dashboard in the first place, but I kinda assumed you and Bhavya had already discussed that

I hadn't done an a11y deep dive into the Dashboard page before so this is the first that I'm seeing it... I guess it's too large to address in this PR though. I'll focus on helping you merge this so that we can get the tests merged and I'll take a note to revisit it later. Might ping you about it later depending on how investigation goes.

myasonik

That's for looking into these!

test/accessibility/apps/dashboard.ts

majagrubic · 2020-02-27T16:19:19Z

Jenkins, test this

majagrubic · 2020-02-27T16:42:40Z

@elasticmachine merge upstream

majagrubic · 2020-02-27T19:01:52Z

@elasticmachine merge upstream

myasonik · 2020-02-27T19:16:16Z

@majagrubic @bhavyarm Anything else on this or is it good to merge?

I think if this merges to master, it'll supersede #57821 but they have diverged a little. (Bhavya's committed cab78c9 since this branched off.)

majagrubic · 2020-02-27T19:27:56Z

I'm trying to get the tests to pass, but otherwise good to merge from my side

elasticmachine · 2020-02-27T19:28:24Z

Pinging @elastic/kibana-app (Team:KibanaApp)

majagrubic · 2020-02-28T09:58:35Z

@elasticmachine merge upstream

lizozom

Code owner changes LGTM

Co-Authored-By: Michail Yasonik <michail.yasonik@elastic.co>

src/legacy/core_plugins/input_control_vis/public/components/vis/list_control.tsx

myasonik · 2020-03-02T15:00:45Z

src/legacy/ui/public/validated_range/validated_dual_range.js

          fullWidth={fullWidth}
          value={this.state.value}
          onChange={this._onChange}
+          focusable={false}


Add a comment above to make focusable again when #59039 is fixed

…s/list_control.tsx Co-Authored-By: Michail Yasonik <michail.yasonik@elastic.co>

majagrubic · 2020-03-02T18:58:37Z

@elasticmachine merge upstream

kibanamachine · 2020-03-02T20:54:01Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: f9224cd

History

💔 Build #30124 failed 919221b
💔 Build #30066 failed 59fd20f
💚 Build #30001 succeeded 1976b66
💚 Build #29665 succeeded 41776c0
💔 Build #29504 failed 73e19b2

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

* master: (26 commits) [Endpoint] Alert Details Overview (elastic#58412) Service map language icons (elastic#58633) [SIEM] [Case] Comments to case view (elastic#58315) Remove appBasePath from docs + add mock for AppMountParameters (elastic#58775) [kbn/optimizer] fix ui/* url rewrites in dist (elastic#58627) Dashboard a11y tests (elastic#58122) Downgrade "setting up plugin" log to debug (elastic#58776) [CI] Pipeline refactoring (elastic#56447) [Advanced Settings] Fix a11y of unsaved indicator (elastic#58511) put params into short url instead of behind it (elastic#58846) show timepicker in timelion and tsvb (elastic#58857) improve graph missing workspace error message (elastic#58876) [Maps] direct Discover "visualize" to open Maps application (elastic#58549) Disallow duplicate percentiles (elastic#57444) (elastic#58299) removing references to visTypes uiExports (elastic#58337) [SIEM] Default the Timeline events filter to show All events (elastic#58953) [Remote clusters] Add indexManagement as required plugin (elastic#58915) [DOCS] Rework of main get started page (elastic#58260) [Endpoint] [Tests] fixes elastic#57946 flaky endpoint policy list test (elastic#58348) [Endpoint] add resolver middleware (elastic#58288) ...

* adding comprehensive dashboard tests * fixing delete and adding dima changes * Fixing some of the a11y test failures * Fixing i18n issue * Extracting exit fullscreen logic in a separate function * Fixing typo * Upgrading axe * Fixing failing jest tests * Removing main tag as it was causing a test to fail * Adding focusable=false to a range control as well * Update test/accessibility/apps/dashboard.ts Co-Authored-By: Michail Yasonik <michail.yasonik@elastic.co> * Fixing linting error * Update src/legacy/core_plugins/input_control_vis/public/components/vis/list_control.tsx Co-Authored-By: Michail Yasonik <michail.yasonik@elastic.co> * Add comments Co-authored-by: Bhavya RM <bhavya@elastic.co> Co-authored-by: Michail Yasonik <michail@yasonik.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Bhavya RM <bhavya@elastic.co> Co-authored-by: Michail Yasonik <michail@yasonik.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

* master: Dashboard a11y tests (elastic#58122) Downgrade "setting up plugin" log to debug (elastic#58776) [CI] Pipeline refactoring (elastic#56447) [Advanced Settings] Fix a11y of unsaved indicator (elastic#58511) put params into short url instead of behind it (elastic#58846) show timepicker in timelion and tsvb (elastic#58857) improve graph missing workspace error message (elastic#58876)

majagrubic changed the title ~~Dashboard a11y tes~~ Dashboard a11y tests Feb 20, 2020

majagrubic requested a review from myasonik February 21, 2020 09:24

myasonik reviewed Feb 21, 2020

View reviewed changes

myasonik suggested changes Feb 21, 2020

View reviewed changes

majagrubic force-pushed the pr/57821 branch from 8f8d474 to 88b41fa Compare February 21, 2020 17:18

myasonik mentioned this pull request Feb 21, 2020

(Accessibility) Skip links to main content #38980

Closed

myasonik approved these changes Feb 21, 2020

View reviewed changes

test/accessibility/apps/dashboard.ts Outdated Show resolved Hide resolved

majagrubic marked this pull request as ready for review February 27, 2020 19:27

majagrubic requested a review from a team February 27, 2020 19:27

majagrubic requested a review from a team as a code owner February 27, 2020 19:27

majagrubic added v7.7.0 v8.0.0 Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// labels Feb 27, 2020

majagrubic added release_note:skip Skip the PR/issue when compiling release notes Feature:Dashboard Dashboard related features labels Feb 27, 2020

majagrubic requested a review from myasonik February 28, 2020 11:41

lizozom approved these changes Mar 1, 2020

View reviewed changes

bhavyarm and others added 5 commits March 2, 2020 09:59

adding comprehensive dashboard tests

d99cb06

fixing delete and adding dima changes

924fc62

Fixing some of the a11y test failures

06a7eea

Fixing i18n issue

ed518fe

Extracting exit fullscreen logic in a separate function

f29d5b3

Maja Grubic and others added 7 commits March 2, 2020 09:59

Fixing typo

37b6bdb

Upgrading axe

3520d1b

Fixing failing jest tests

c998cec

Removing main tag as it was causing a test to fail

4be666b

Adding focusable=false to a range control as well

d67877b

Update test/accessibility/apps/dashboard.ts

154a40c

Co-Authored-By: Michail Yasonik <michail.yasonik@elastic.co>

Fixing linting error

1976b66

majagrubic force-pushed the pr/57821 branch from 41776c0 to 1976b66 Compare March 2, 2020 10:15

myasonik approved these changes Mar 2, 2020

View reviewed changes

Maja Grubic and others added 2 commits March 2, 2020 15:02

Update src/legacy/core_plugins/input_control_vis/public/components/vi…

59fd20f

…s/list_control.tsx Co-Authored-By: Michail Yasonik <michail.yasonik@elastic.co>

Add comments

919221b

Merge branch 'master' into pr/57821

f9224cd

majagrubic merged commit b7c8e3a into master Mar 2, 2020

majagrubic deleted the pr/57821 branch March 2, 2020 20:56

myasonik mentioned this pull request Mar 2, 2020

Comprehensive dashboard a11y tests #57821

Closed

majagrubic mentioned this pull request Mar 3, 2020

[7.x] Dashboard a11y tests (#58122) #59131

Merged

majagrubic mentioned this pull request Mar 3, 2020

Fixing a small typo #59185

Merged

1 task

nreese mentioned this pull request Mar 13, 2020

Migrate dual validated range #59689

Merged

	defaultMessage: 'previous page button',
	defaultMessage: 'Previous page in table',

	defaultMessage: 'next page button',
	defaultMessage: 'Next page in table',

Conversation

majagrubic commented Feb 20, 2020

Summary

Checklist

For maintainers

Uh oh!

majagrubic commented Feb 21, 2020

Uh oh!

myasonik Feb 20, 2020

Choose a reason for hiding this comment

Uh oh!

myasonik Feb 20, 2020

Choose a reason for hiding this comment

Uh oh!

myasonik Feb 20, 2020

Choose a reason for hiding this comment

Uh oh!

majagrubic Feb 21, 2020

Choose a reason for hiding this comment

Uh oh!

majagrubic Feb 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

myasonik Feb 21, 2020

Choose a reason for hiding this comment

Uh oh!

myasonik left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

majagrubic commented Feb 27, 2020

Uh oh!

majagrubic commented Feb 27, 2020

Uh oh!

majagrubic commented Feb 27, 2020

Uh oh!

myasonik commented Feb 27, 2020

Uh oh!

majagrubic commented Feb 27, 2020

Uh oh!

elasticmachine commented Feb 27, 2020

Uh oh!

majagrubic commented Feb 28, 2020

Uh oh!

lizozom left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

myasonik Mar 2, 2020

Choose a reason for hiding this comment

Uh oh!

majagrubic commented Mar 2, 2020

Uh oh!

kibanamachine commented Mar 2, 2020

💚 Build Succeeded

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

majagrubic Feb 21, 2020 •

edited

Loading