[Monitoring] Address multiple accessibility issues #20619

chrisronline · 2018-07-10T14:56:59Z

Fixes #20191
Fixes #20190
Fixes #20189
Fixes #20188

Summary

This PR addresses multiple accessibility issues in monitoring, all related to optimizing screen reader experience.

One visual change is related to #20188 where we are now showing the total number of alerts in the table:

Testing

To test this PR, please see the individual issues and use a screen reader tool (I used ChromeVox) to ensure the issues are properly addressed

…he top

chrisronline · 2018-07-10T14:57:51Z

x-pack/plugins/monitoring/public/components/chart/monitoring_timeseries_container.js

-            <KuiInfoButton />
+            <Fragment>
+              <KuiInfoButton aria-labelledby={`monitoringChart${titleForAriaIds}`} />
+              <span id={`monitoringChart${titleForAriaIds}`} className="kuiScreenReaderOnly">


I added this as a way for the screen reader to connect the content in the tooltip with the button showing it. I couldn't do this in the InfoTooltip itself as that doesn't render any DOM until the button is clicked. Perhaps there is a better way?

chrisronline · 2018-07-10T14:58:20Z

x-pack/plugins/monitoring/public/directives/alerts/index.js

-            rowComponent={alertRowFactory(scope, kbnUrl)}
-          />
+          <Fragment>
+            <p className="kuiText kuiVerticalRhythm" tabIndex="0">Showing {alerts.length} alert(s)</p>


Unsure if this is the way we want to show this or if it's in the right place, but I assumed this change would be short lived as we'll be switching this page to EUI soon.

If it's going to be that short lived, then I'd rather we focus on changing it to use EUI instead of this.

Yeah, if EUI will address the issue that introduced this, let's just bring in the fix when the monitoring table component is updated to use EUI.

chrisronline · 2018-07-10T15:01:14Z

Also, should we wait for 6.4 or put this into the next 6.3?

elasticmachine · 2018-07-10T16:16:16Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

pickypg

Mostly about kui* usage.

pickypg · 2018-07-10T15:07:13Z

x-pack/plugins/monitoring/public/components/chart/monitoring_timeseries_container.js

-            <KuiInfoButton />
+            <Fragment>
+              <KuiInfoButton aria-labelledby={`monitoringChart${titleForAriaIds}`} />
+              <span id={`monitoringChart${titleForAriaIds}`} className="kuiScreenReaderOnly">


Use <EuiScreenReaderOnly /> instead; try to never use kui*.

pickypg · 2018-07-10T15:18:56Z

x-pack/plugins/monitoring/public/components/elasticsearch/shard_activity/shard_activity.js

@@ -56,6 +56,7 @@ const ToggleCompletedSwitch = ({ toggleHistory, showHistory }) => (
  <KuiToolBarSection>
    <KuiToolBarText>
      <EuiSwitch
+        aria-label="Completed recoveries"
        label="Completed recoveries"


We should probably get EuiSwitch tweaked to have label represent the aria-label. This should be fine in the short term.

I discussed with @elastic/kibana-design and they mentioned the consumer should manually add aria-label here as the assumption that label and aria-label are the same is not always safe to make.

I do wonder if the default should be for label to be aria-label, but then allow aria-label to be set explicitly for such cases.

I'm confused as to why you need to add aria-label at all. The rendered HTML of a EuiSwitch should look something like:

<input type="checkbox" class="euiSwitch__input" id="tbav8l2s"> <span class="euiSwitch__body"> ...No need to worry about what's in here... </span> <label class="euiSwitch__label" for="tbav8l2s">I am a switch</label>

which connects the input (hidden for the stylized switch look) with the form label via the for and id attributes. By adding another aria-label you will be duplicating that inherit accessibility. So as long as you pass both a label and id, there is no need for aria-label.

Ah okay. So we either need an explicit aria-label OR add an id. I'd vote the latter. Thoughts?

Sounds reasonable to me. Seems like a good way to get good coverage of its usage.

pickypg · 2018-07-10T15:21:10Z

x-pack/plugins/monitoring/public/directives/alerts/index.js

-            rowComponent={alertRowFactory(scope, kbnUrl)}
-          />
+          <Fragment>
+            <p className="kuiText kuiVerticalRhythm" tabIndex="0">Showing {alerts.length} alert(s)</p>


Don't add new usages of kui*. Exclusively use Eui for new content; the only exception is existing usage of KuiTable (which we want to switch over to EuiTable!).

I don't have any strong feelings about this text, but its placement seems abnormal for the rest of Monitoring (and Kibana for that matter). Shouldn't this reported counting issue be solved generically within MonitoringTable?

@tsullivan What are your thoughts here?

I created #20672 to describe how we might fix this within EUI but I'm still not sure the best place for it here

Just to be clear, MonitoringTable is our wrapper of KuiTable (which hopefully becomes EuiTable soon) and streamlines all of our tables. We could add our own count logic there, but it does make sense to have counts appear near the paging controls, which I assume for accessibility should always be visible even if they're disabled.

@pickypg I moved the results label to the bottom of the table so it looks like:

I'm not sure that works great from an accessibility perspective as the user will need to tab through all the rows before seeing the total count, but I'm not an expert here to know for sure.

Any thoughts @elastic/kibana-design?

We could probably accomplish something like @cchaos is showing by adding it to the right of the filter text box? That seems like a good place for the screen reader to get the data as well, especially since we may not know the total versus what we're displaying.

@pickypg @cchaos ^^

@chrisronline I like it, but I cannot remember: is that count the actual total or just the total displayed on that page? If it's the former, then that seems like the other example. If it's the amount displayed on the current page, then the phrasing is misleading.

pickypg · 2018-07-10T15:22:57Z

x-pack/plugins/monitoring/public/components/chart/monitoring_timeseries_container.js

-      <h2 className="euiTitle">
-        { getTitle(series) }{ units ? ` (${units})` : '' }
+      <h2 className="euiTitle" tabIndex="0">
+        <span className="kuiScreenReaderOnly">This chart is not accessible yet</span>


Use <EuiScreenReaderOnly /> instead.

Is this the best way that we can phrase this? It seems kind of stiff to me. Perhaps "This chart does not support screen readers."?

I took the text directly from the issue which I'm assuming is what we want?

@gchaps Relative to #20191, what do you think about this particular text?

How about "This chart is not screen reader accessible" since this text is just for screen reader to announce

pickypg · 2018-07-10T17:02:46Z

Also, should we wait for 6.4 or put this into the next 6.3?

For the things I would deem fixes, 6.3 would be fine, but for the addition I think it should go into 6.4 since it's a visual change.

elasticmachine · 2018-07-11T16:22:19Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

This reverts commit d622eb7.

elasticmachine · 2018-07-11T18:46:51Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-07-11T19:19:05Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-07-13T14:42:17Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

tsullivan

I still need some more time to look at the changes, but first off I I have a suggestion about simplifying a way of creating an array out of different kinds of things

elasticmachine · 2018-07-19T16:17:01Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

tsullivan

Let's keep all the CSS together, for now :D

tsullivan · 2018-07-19T20:00:43Z

x-pack/plugins/monitoring/public/components/table/toolbar.js

@@ -13,6 +13,8 @@ import {
  KuiToolBarText
 } from '@kbn/ui-framework/components';

+import './toolbar.css';
+


We've been keeping all our CSS separate from public/components, and we have a place in public/less that would be more suitable for the new flex rule in public/less/components/table.less

tsullivan · 2018-07-19T20:08:08Z

x-pack/plugins/monitoring/public/directives/alerts/index.js

@@ -54,17 +54,17 @@ const alertRowFactory = (scope, kbnUrl) => {

    return (
      <KuiTableRow>
-        <KuiTableRowCell>
+        <KuiTableRowCell tabIndex="0">


Does each cell really need to be tabbable? Is this just to make it visible to a screen-reader?

I'd imagine so, otherwise the screen reader can't read any of the content in non tabbable cells

I think I've found this to be true myself. I was hoping there'd be another way :D

elasticmachine · 2018-07-19T21:51:26Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

tsullivan · 2018-08-06T17:56:02Z

The interval in the tooltip is blank:

tsullivan

So close! Just saw a small issue with the interval field not coming through in the chart tooltip.

Also, I was not able to test out the cluster alerts table... having an issue getting cluster alerts to run in the latest ES 7.0 docker snapshot

…bility

chrisronline · 2018-08-07T14:55:22Z

@tsullivan Nice catch! Pushed a fix

elasticmachine · 2018-08-07T16:16:36Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

tsullivan · 2018-08-07T17:09:50Z

this'll need to get updated with master for merge conflicts to resolve

elasticmachine · 2018-08-07T18:51:43Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

chrisronline · 2018-08-07T18:53:00Z

@tsullivan All fixed up!

elasticmachine · 2018-08-07T19:01:15Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

tsullivan

LGTM

pickypg

LGTM from code

* Add screen reader only text for monitoring charts * Add aria label for completed recoveries * Ensure we have aria label coverage for monitoring chart tooltips * Ensure table rows are tabbable and include the number of results at the top * Use EuiScreenReaderOnly * Updated copy * Use EUI * Remove kui usage * Use an id instead of aria-label * Show results in the table footer * Revert "Show results in the table footer" This reverts commit d622eb7. * Show total row count within the monitoring table * PR feedback * PR feedback * Ensure all charts show an interval in the tooltip * Fix padding issue with the cluster status

chrisronline · 2018-08-08T16:54:30Z

Backport:

6.x: a635df9

chrisronline added 4 commits July 9, 2018 15:16

Add screen reader only text for monitoring charts

ea6f802

Add aria label for completed recoveries

1bf8fdb

Ensure we have aria label coverage for monitoring chart tooltips

08b2247

Ensure table rows are tabbable and include the number of results at t…

deb0df8

…he top

chrisronline added review Project:Accessibility v7.0.0 v6.4.0 Team:Monitoring Stack Monitoring team labels Jul 10, 2018

chrisronline self-assigned this Jul 10, 2018

chrisronline requested review from tsullivan and pickypg July 10, 2018 14:57

chrisronline commented Jul 10, 2018

View reviewed changes

pickypg requested changes Jul 10, 2018

View reviewed changes

chrisronline added 3 commits July 10, 2018 15:37

Use EuiScreenReaderOnly

a4bcd32

Updated copy

3f86b94

Use EUI

c67a7b3

chrisronline added 4 commits July 11, 2018 13:09

Remove kui usage

62f8683

Use an id instead of aria-label

ca1b7af

Show results in the table footer

d622eb7

Revert "Show results in the table footer"

bf93d63

This reverts commit d622eb7.

Show total row count within the monitoring table

18cc017

tsullivan requested changes Jul 17, 2018

View reviewed changes

PR feedback

0f2fd25

tsullivan requested changes Jul 19, 2018

View reviewed changes

tsullivan reviewed Jul 19, 2018

View reviewed changes

PR feedback

7f98d5a

LeeDr added v6.5.0 and removed v6.5.0 v6.4.0 labels Jul 25, 2018

tsullivan requested changes Aug 6, 2018

View reviewed changes

chrisronline added 2 commits August 7, 2018 10:12

Merge remote-tracking branch 'elastic/master' into monitoring/accessi…

ff6eea2

…bility

Ensure all charts show an interval in the tooltip

ec68f59

chrisronline added 2 commits August 7, 2018 13:22

Fix merge conflicts with master

c813b48

Fix padding issue with the cluster status

a8fd4f8

tsullivan approved these changes Aug 7, 2018

View reviewed changes

pickypg approved these changes Aug 8, 2018

View reviewed changes

chrisronline merged commit dd40ac3 into elastic:master Aug 8, 2018

chrisronline deleted the monitoring/accessibility branch August 8, 2018 14:39

chrisronline mentioned this pull request Aug 8, 2018

[6.x] [Monitoring] Address multiple accessibility issues (#20619) #21793

Merged

[Monitoring] Address multiple accessibility issues #20619

[Monitoring] Address multiple accessibility issues #20619

Conversation

chrisronline commented Jul 10, 2018 • edited Loading

Summary

Testing

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tsullivan Jul 17, 2018 • edited Loading

Choose a reason for hiding this comment

chrisronline commented Jul 10, 2018

elasticmachine commented Jul 10, 2018

💚 Build Succeeded

pickypg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pickypg commented Jul 10, 2018

elasticmachine commented Jul 11, 2018

💔 Build Failed

elasticmachine commented Jul 11, 2018

💚 Build Succeeded

elasticmachine commented Jul 11, 2018

💔 Build Failed

elasticmachine commented Jul 13, 2018

💚 Build Succeeded

tsullivan left a comment

Choose a reason for hiding this comment

elasticmachine commented Jul 19, 2018

💚 Build Succeeded

tsullivan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Jul 19, 2018

💚 Build Succeeded

tsullivan commented Aug 6, 2018

tsullivan left a comment

Choose a reason for hiding this comment

chrisronline commented Aug 7, 2018

elasticmachine commented Aug 7, 2018

💚 Build Succeeded

tsullivan commented Aug 7, 2018

elasticmachine commented Aug 7, 2018

💚 Build Succeeded

chrisronline commented Aug 7, 2018

elasticmachine commented Aug 7, 2018

💚 Build Succeeded

tsullivan left a comment

Choose a reason for hiding this comment

pickypg left a comment

Choose a reason for hiding this comment

chrisronline commented Aug 8, 2018

chrisronline commented Jul 10, 2018 •

edited

Loading

tsullivan Jul 17, 2018 •

edited

Loading