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

[Monitoring] Address multiple accessibility issues #20619

Merged
merged 18 commits into from
Aug 8, 2018
Merged

[Monitoring] Address multiple accessibility issues #20619

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
ea6f802
Add screen reader only text for monitoring charts
chrisronline Jul 9, 2018
1bf8fdb
Add aria label for completed recoveries
chrisronline Jul 9, 2018
08b2247
Ensure we have aria label coverage for monitoring chart tooltips
chrisronline Jul 9, 2018
deb0df8
Ensure table rows are tabbable and include the number of results at t…
chrisronline Jul 10, 2018
a4bcd32
Use EuiScreenReaderOnly
chrisronline Jul 10, 2018
3f86b94
Updated copy
chrisronline Jul 11, 2018
c67a7b3
Use EUI
chrisronline Jul 11, 2018
62f8683
Remove kui usage
chrisronline Jul 11, 2018
ca1b7af
Use an id instead of aria-label
chrisronline Jul 11, 2018
d622eb7
Show results in the table footer
chrisronline Jul 11, 2018
bf93d63
Revert "Show results in the table footer"
chrisronline Jul 11, 2018
18cc017
Show total row count within the monitoring table
chrisronline Jul 13, 2018
0f2fd25
PR feedback
chrisronline Jul 19, 2018
7f98d5a
PR feedback
chrisronline Jul 19, 2018
ff6eea2
Merge remote-tracking branch 'elastic/master' into monitoring/accessi…
chrisronline Aug 7, 2018
ec68f59
Ensure all charts show an interval in the tooltip
chrisronline Aug 7, 2018
c813b48
Fix merge conflicts with master
chrisronline Aug 7, 2018
a8fd4f8
Fix padding issue with the cluster status
chrisronline Aug 7, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 1 addition & 4 deletions x-pack/plugins/monitoring/public/components/chart/info_tooltip.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,8 @@
*/

import React from 'react';
import { first, get } from 'lodash';

export function InfoTooltip({ series }) {

const bucketSize = get(first(series), 'bucket_size'); // bucket size will be the same for all metrics in all series
export function InfoTooltip({ series, bucketSize }) {
const tableRows = series.map((item, index) => {
return (
<tr
Expand Down
29 changes: 24 additions & 5 deletions x-pack/plugins/monitoring/public/components/chart/monitoring_timeseries_container.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,8 @@
* you may not use this file except in compliance with the Elastic License.
*/

import React from 'react';
import React, { Fragment } from 'react';
import { first, get } from 'lodash';
import { Tooltip } from 'pivotal-ui/react/tooltip';
import { OverlayTrigger } from 'pivotal-ui/react/overlay-trigger';
import { KuiInfoButton } from '@kbn/ui-framework/components';
Expand All @@ -13,24 +14,42 @@ import { getUnits } from './get_units';
import { MonitoringTimeseries } from './monitoring_timeseries';
import { InfoTooltip } from './info_tooltip';

import {
EuiScreenReaderOnly
} from '@elastic/eui';

export function MonitoringTimeseriesContainer({ series, onBrush }) {
if (series === undefined) {
return null; // still loading
}

const title = getTitle(series);
const titleForAriaIds = title.replace(/\s+/, '--');
const units = getUnits(series);
const bucketSize = get(first(series), 'bucket_size'); // bucket size will be the same for all metrics in all series

const seriesScreenReaderTextList = [`Interval: ${bucketSize}`]
.concat(series.map(item => `${item.metric.label}: ${item.metric.description}`));

return (
<div className="monitoring-chart__container">
<h2 className="euiTitle">
{ getTitle(series) }{ units ? ` (${units})` : '' }
<h2 className="euiTitle" tabIndex="0">
<EuiScreenReaderOnly><span>This chart is not screen reader accessible</span></EuiScreenReaderOnly>
{ title }{ units ? ` (${units})` : '' }
<OverlayTrigger
placement="left"
trigger="click"
overlay={<Tooltip><InfoTooltip series={series}/></Tooltip>}
overlay={<Tooltip><InfoTooltip series={series} bucketSize={bucketSize}/></Tooltip>}
>
<span className="monitoring-chart-tooltip__trigger overlay-trigger">
<KuiInfoButton />
<Fragment>
<KuiInfoButton aria-labelledby={`monitoringChart${titleForAriaIds}`} />
<EuiScreenReaderOnly>
<span id={`monitoringChart${titleForAriaIds}`}>
{seriesScreenReaderTextList.join('. ')}
</span>
</EuiScreenReaderOnly>
</Fragment>
</span>
</OverlayTrigger>
</h2>
Expand Down
1 change: 1 addition & 0 deletions x-pack/plugins/monitoring/public/components/elasticsearch/shard_activity/shard_activity.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ const ToggleCompletedSwitch = ({ toggleHistory, showHistory }) => (
<KuiToolBarSection>
<KuiToolBarText>
<EuiSwitch
id="monitoring_completed_recoveries"
label="Completed recoveries"
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

onChange={toggleHistory}
checked={showHistory}
Expand Down
1 change: 1 addition & 0 deletions x-pack/plugins/monitoring/public/components/table/table.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,6 +256,7 @@ export class MonitoringTable extends React.Component {
pageIndexFirstRow={numVisibleRows ? firstRow + 1 : 0}
pageIndexLastRow={numVisibleRows ? numVisibleRows + firstRow : 0}
rowsFiltered={numAvailableRows}
totalRows={this.state.rows.length}
filterText={this.state.filterText}
paginationControls={this.getPaginationControls(numAvailableRows, this.props.alwaysShowPageControls)}
onFilterChange={this.onFilterChange}
Expand Down
12 changes: 11 additions & 1 deletion x-pack/plugins/monitoring/public/components/table/toolbar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,15 +37,25 @@ export function MonitoringTableToolBar(props) {
)
: null;

const totalRows = Boolean(props.showTotalRows)
? (
<p tabIndex="0" className="monitoringTableToolbarTotalRows">
{props.totalRows} in total
</p>
)
: null;

return (
<KuiToolBar>
{ searchBox }
{ totalRows }
{ props.renderToolBarSections(props) }
{ paginationSection }
</KuiToolBar>
);
}
MonitoringTableToolBar.defaultProps = {
renderToolBarSections: noop,
showSearchBox: true
showSearchBox: true,
showTotalRows: true
};
12 changes: 6 additions & 6 deletions x-pack/plugins/monitoring/public/directives/alerts/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,17 +54,17 @@ const alertRowFactory = (scope, kbnUrl) => {

return (
<KuiTableRow>
<KuiTableRowCell>
<KuiTableRowCell tabIndex="0">
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

<Tooltip text={severityIcon.title} placement="bottom" trigger="hover">
<EuiHealth color={severityIcon.color} data-test-subj="alertIcon" aria-label={severityIcon.title}>
{ capitalize(severityIcon.value) }
</EuiHealth>
</Tooltip>
</KuiTableRowCell>
<KuiTableRowCell>
<KuiTableRowCell tabIndex="0">
{ resolution.icon } { resolution.text }
</KuiTableRowCell>
<KuiTableRowCell>
<KuiTableRowCell tabIndex="0">
<FormattedMessage
prefix={props.prefix}
suffix={props.suffix}
Expand All @@ -73,13 +73,13 @@ const alertRowFactory = (scope, kbnUrl) => {
changeUrl={changeUrl}
/>
</KuiTableRowCell>
<KuiTableRowCell>
<KuiTableRowCell tabIndex="0">
{ linkToCategories[props.metadata.link] ? linkToCategories[props.metadata.link] : 'General' }
</KuiTableRowCell>
<KuiTableRowCell>
<KuiTableRowCell tabIndex="0">
{ formatDateTimeLocal(props.update_timestamp) }
</KuiTableRowCell>
<KuiTableRowCell>
<KuiTableRowCell tabIndex="0">
{ formatTimestampToDuration(props.timestamp, CALCULATE_DURATION_SINCE) } ago
</KuiTableRowCell>
</KuiTableRow>
Expand Down
4 changes: 4 additions & 0 deletions x-pack/plugins/monitoring/public/less/components/table.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,3 +60,7 @@
color: #666977;
font-size: 12px;
}

.monitoringTableToolbarTotalRows {
flex: 2; /* This is to ensure this shows directly next to the search bar in the table toolbar */
}