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.

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

feat(a11y): improve chart figure #1104

Merged
merged 15 commits into from
Apr 12, 2021
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
45 changes: 45 additions & 0 deletions integration/tests/accessibility.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
/*
* Licensed to Elasticsearch B.V. under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch B.V. licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

import { common } from '../page_objects/common';

describe('Accessibility tree', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

😍

it('should include the series types if one type of series', async () => {
const tree = await common.testAccessibilityTree(
'http://localhost:9001/iframe.html?id=annotations-lines--x-continuous-domain',
'.echCanvasRenderer',
);
// the legend has bars and lines as value.descriptions not value.name
const hasTextOfChartTypes = tree.children.filter((value) => {
return value.name === 'bars';
});
expect(hasTextOfChartTypes.length).toBe(1);
});
it('should include the series types if multiple types of series', async () => {
const tree = await common.testAccessibilityTree(
'http://localhost:9001/iframe.html?id=mixed-charts--bars-and-lines',
'.echCanvasRenderer',
);
// the legend has bars and lines as value.descriptions not value.name
const hasTextOfChartTypes = tree.children.filter((value) => {
return value.name === 'bars' || value.name === 'lines';
});
expect(hasTextOfChartTypes.length).toBe(2);
});
});
49 changes: 36 additions & 13 deletions src/chart_types/xy_chart/renderer/canvas/xy_chart.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -152,27 +152,50 @@ class XYChartComponent extends React.Component<XYChartProps> {
initialized,
isChartEmpty,
chartContainerDimensions: { width, height },
geometries,
} = this.props;

if (!initialized || isChartEmpty) {
this.ctx = null;
return null;
}

const seriesTypes: string[] = [];
Object.entries(geometries).forEach((value) => {
if (value[1].length > 0) {
seriesTypes.push(value[0]);
}
});
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 not base our text on internal object keys. We should be able to get that information in a better/cleaner way. We should not think about text changes if we refactor our internal code structure.

Also remember that we need to add i18n, and having a stronger enum instead of internal object keys can improve the robustness of the code.

Copy link

Choose a reason for hiding this comment

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

How would you recommend getting this data if not using the object keys?

On the i18n front, I don't think it's fair to expect a PR that improves a11y to also solve Charts' i18n in one fell swoop. Without an existing i18n solution in place (correct me if I'm wrong as I'm only loosely familiar with the codebase), building with a hypothetical solution in mind brings a lot of overhead with little gain imo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Refactored in e7d1230 thank you!

Copy link
Member

Choose a reason for hiding this comment

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

How would you recommend getting this data if not using the object keys?

I'm referring to the fact that we are extracting the chart type "strings" from an object geometries that is an internal object which structure can change in the future easily.

This refactoring increase also the current issue: if the geometries object change its signature, the getNameFunction can just return unkown chart for each chart type without being warned.

Object.entries(geometries).forEach((value) => {
      if (value[1].length > 0 && value[0]) {
        seriesTypes.push(getNameFunction(value[0]));
      }
    });
....
export const getNameFunction = (key: string): string => {
  const screenReader: Map<string, string> = new Map();
  screenReader
    .set('bars', 'Bar chart')
    .set('areas', 'Area chart')
    .set('lines', 'Line chart')
    .set('bubbles', 'Bubble chart');
  return screenReader.get(key) ?? 'unknown chart';
};

The concept of chart type is already built into each spec, the react component that specifies the chart configuration. I'm saying that. instead of taking the chart type information from an internal object, like geometris, we should create that information from the actual specs using an already well-known and exposed enum called SeriesType.

A selector that picks the actual series types and creates a Set of unique SeriesTypes is cleaner and we can map the well-known SeriesType enum values to a set of internationalized strings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok thanks please see 9e036d3


return (
<canvas
ref={forwardStageRef}
className="echCanvasRenderer"
width={width * this.devicePixelRatio}
height={height * this.devicePixelRatio}
style={{
width,
height,
}}
aria-label="Chart"
// eslint-disable-next-line jsx-a11y/no-interactive-element-to-noninteractive-role
role="img"
/>
<figure>
<canvas
ref={forwardStageRef}
className="echCanvasRenderer"
width={width * this.devicePixelRatio}
height={height * this.devicePixelRatio}
style={{
width,
height,
}}
aria-label="Chart"
Copy link
Member

Choose a reason for hiding this comment

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

As stated in the issue, the aria-* labels should be then moved to the dl element.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please see e7d1230 thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In e7d1230 I ended up removing the aria-label because it's not read by VoiceOver and this PR adds more information. I was leaning towards not having the aria-label since it's not providing more information on the <dl> tags.

// eslint-disable-next-line jsx-a11y/no-interactive-element-to-noninteractive-role
role="presentation"
>
<dl className="screen-reader">
{seriesTypes.length > 0 ? (
<dt>
Chart type(s)
{seriesTypes.map((value, index) => {
return <dd key={`${index}`}>{value}</dd>;
})}
Copy link
Collaborator

Choose a reason for hiding this comment

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

simplified...

Suggested change
{seriesTypes.map((value, index) => {
return <dd key={`${index}`}>{value}</dd>;
})}
{seriesTypes.map((value, index) => <dd key={`${index}`}>{value}</dd>)}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thank you fixed in e7d1230

</dt>
) : (
<dt>No series in chart</dt>
)}
</dl>
</canvas>
</figure>
markov00 marked this conversation as resolved.
Show resolved Hide resolved
);
}
}
Expand Down
1 change: 1 addition & 0 deletions src/chart_types/xy_chart/renderer/dom/_index.scss
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
@import 'highlighter';
@import 'crosshair';
@import 'screen_reader';
@import 'annotations/index';
7 changes: 7 additions & 0 deletions src/chart_types/xy_chart/renderer/dom/_screen_reader.scss
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
.screen-reader {
Copy link
Member

Choose a reason for hiding this comment

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

always prefix classes with ech

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good call thank you - fixed in e7d1230

position: absolute;
left: -10000px;
top: auto;
width: 1px;
height: 1px;
myasonik marked this conversation as resolved.
Show resolved Hide resolved
}
13 changes: 12 additions & 1 deletion src/components/__snapshots__/chart.test.tsx.snap
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,18 @@ exports[`Chart should render the legend name test 1`] = `
</Connect(Crosshair)>
<Connect(XYChart) forwardStageRef={{...}}>
<XYChart forwardStageRef={{...}} initialized={true} isChartEmpty={false} debug={true} geometries={{...}} geometriesIndex={{...}} theme={{...}} chartContainerDimensions={{...}} highlightedLegendItem={[undefined]} rotation={0} renderingArea={{...}} chartTransform={{...}} axesSpecs={{...}} perPanelAxisGeoms={{...}} perPanelGridLines={{...}} axesStyles={{...}} annotationDimensions={{...}} annotationSpecs={{...}} panelGeoms={{...}} onChartRendered={[Function (anonymous)]}>
<canvas className=\\"echCanvasRenderer\\" width={150} height={200} style={{...}} aria-label=\\"Chart\\" role=\\"img\\" />
<figure>
<canvas className=\\"echCanvasRenderer\\" width={150} height={200} style={{...}} aria-label=\\"Chart\\" role=\\"presentation\\">
<dl className=\\"screen-reader\\">
<dt>
Chart type(s)
<dd>
bars
</dd>
</dt>
</dl>
</canvas>
</figure>
</XYChart>
</Connect(XYChart)>
<Connect(Tooltip) getChartContainerRef={[Function (anonymous)]}>
Expand Down