feat(a11y): improve chart figure #1104

rshen91 · 2021-04-01T17:00:42Z

Summary

Adding series types for Cartesian charts.

Checklist

Delete any items that are not applicable to this PR.

Unit tests were updated or added to match the most common scenarios
Unfortunately the previous test in the accessibility.test.tsx tests has been removed

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);
  });

The issue is that the accessibilitySnapshot is not capturing the aria-label consistently in the testAccessibilityTree function in common

await page.accessibility.snapshot().then((value) => {
      return value;
    });

Luckily, we are capturing the aria-label in chart.test.tsx.snap

codecov-io · 2021-04-01T17:13:08Z

Codecov Report

Merging #1104 (9b11e85) into master (74df29b) will increase coverage by 0.46%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1104      +/-   ##
==========================================
+ Coverage   72.05%   72.52%   +0.46%     
==========================================
  Files         381      398      +17     
  Lines       11929    12255     +326     
  Branches     2603     2652      +49     
==========================================
+ Hits         8596     8888     +292     
- Misses       3308     3334      +26     
- Partials       25       33       +8

Flag	Coverage Δ
unittests	`72.11% <100.00%> (+0.05%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
.../chart_types/xy_chart/renderer/canvas/xy_chart.tsx	`94.80% <100.00%> (+0.21%)`	⬆️
...types/xy_chart/state/selectors/get_series_types.ts	`100.00% <100.00%> (ø)`
src/mocks/series/data.ts	`100.00% <0.00%> (ø)`
src/mocks/series/index.ts	`100.00% <0.00%> (ø)`
src/mocks/series/series_identifiers.ts	`100.00% <0.00%> (ø)`
src/mocks/series/series.ts	`91.22% <0.00%> (ø)`
src/mocks/index.ts	`100.00% <0.00%> (ø)`
src/mocks/geometries.ts	`92.85% <0.00%> (ø)`
src/mocks/scale/scale.ts	`77.77% <0.00%> (ø)`
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 74df29b...9b11e85. Read the comment docs.

rshen91 · 2021-04-02T14:08:37Z

jenkins test this please

nickofthyme

Overall, LGTM. I just left a few comments.

nickofthyme · 2021-04-05T22:31:46Z

integration/tests/accessibility.test.ts

+
+import { common } from '../page_objects/common';
+
+describe('Accessibility tree', () => {


src/chart_types/xy_chart/renderer/dom/_screen_reader.scss

nickofthyme · 2021-04-05T22:35:07Z

src/chart_types/xy_chart/renderer/canvas/xy_chart.tsx

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


simplified...

Suggested change

{seriesTypes.map((value, index) => {

return <dd key={`${index}`}>{value}</dd>;

})}

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

thank you fixed in e7d1230

markov00

Despite the fact that I like the approach taken, I think we should follow what was suggested in the original issue first.
I'd recommend tackling this after #1096, #1097, and #1098 otherwise it's too much structure for too little info

markov00 · 2021-04-02T15:29:16Z

src/chart_types/xy_chart/renderer/dom/_screen_reader.scss

@@ -0,0 +1,7 @@
+.screen-reader {


always prefix classes with ech

good call thank you - fixed in e7d1230

markov00 · 2021-04-06T08:58:38Z

src/chart_types/xy_chart/renderer/canvas/xy_chart.tsx

+    Object.entries(geometries).forEach((value) => {
+      if (value[1].length > 0) {
+        seriesTypes.push(value[0]);
+      }
+    });


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.

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.

Refactored in e7d1230 thank you!

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.

Ok thanks please see 9e036d3

markov00 · 2021-04-06T08:59:53Z

src/chart_types/xy_chart/renderer/canvas/xy_chart.tsx

+            width,
+            height,
+          }}
+          aria-label="Chart"


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

Please see e7d1230 thanks!

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.

src/chart_types/xy_chart/renderer/canvas/xy_chart.tsx

myasonik · 2021-04-06T21:52:06Z

Despite the fact that I like the approach taken, I think we should follow what was suggested in the original issue first.

I'd recommend tackling this after #1096, #1097, and #1098 otherwise it's too much structure for too little info

@markov00 I know I wrote that ☝️ but this PR still does improve the state of charts a11y so I'd rather merge it (when it's green) and do the issues in a different order than have to sit on this PR while it rots.

It does actually mean less work in the long run imo because this PR changes the DOM structure somewhat and the other work can be built directly into this new structure instead of needing to refactor it after the fact.

TL;DR The new structure is arguably a little overkill... But that's better than the vast gap we have now...

rshen91 · 2021-04-07T21:19:45Z

jenkins test this please

myasonik

🚀

markov00

Looks good to me, I've added few minor comments

markov00 · 2021-04-08T08:20:47Z

src/chart_types/xy_chart/renderer/canvas/xy_chart.tsx

+    const multipleSeriesTypes: string[] = [];
+    seriesTypes.forEach((value) => multipleSeriesTypes.push(value));


Suggested change

const multipleSeriesTypes: string[] = [];

seriesTypes.forEach((value) => multipleSeriesTypes.push(value));

It looks like you are not using multipleSeriesTypes so you can remove these lines

Thank you! 9b11e85

markov00 · 2021-04-08T08:21:30Z

src/chart_types/xy_chart/renderer/canvas/xy_chart.tsx

+          role="presentation"
+        >
+          <dl className="echScreen-reader">
+            <dt> Chart type </dt>


nit

Suggested change

<dt> Chart type </dt>

<dt>Chart type</dt>

Thank you! 9b11e85

markov00 · 2021-04-08T08:24:44Z

src/chart_types/xy_chart/renderer/dom/_screen_reader.scss

@@ -0,0 +1,8 @@
+.echScreen-reader {


Suggested change

.echScreen-reader {

.echScreenReader {

or similar to EUI

Suggested change

.echScreen-reader {

.echScreenReaderOnly {

We should follow the BEM naming coventions

Good call - 9b11e85

markov00 · 2021-04-08T08:30:38Z

src/chart_types/xy_chart/state/selectors/get_series_types.ts

+export const getSeriesTypes = createCachedSelector(
+  [getSeriesSpecsSelector],
+  (specs): Set<SeriesType> => {
+    const seriesTypes = new Set<SeriesType>();
+    specs.forEach((value) => seriesTypes.add(value.seriesType));
+    return seriesTypes;
+  },
+)(getChartIdSelector);


A way better!
With this, we can call an internationalization function (in the future) that uses SeriesType to translate the enum values accordingly without having issues when renaming the value of the SeriesType.

rshen91 · 2021-04-12T15:20:56Z

Jenkins, test this please

# [28.1.0](v28.0.1...v28.1.0) (2021-04-13) ### Bug Fixes * **legend:** sizing for short labels with scrollbar ([#1115](#1115)) ([6e1f223](6e1f223)) * **xy:** negative bar highlight and click ([#1109](#1109)) ([ec17cb2](ec17cb2)), closes [#1100](#1100) ### Features * **a11y:** improve chart figure ([#1104](#1104)) ([815cf39](815cf39)) * **partition:** order slices and sectors ([#1112](#1112)) ([74df29b](74df29b)) * **partitions:** small multipies events pass on smAccessorValue ([#1106](#1106)) ([a3234fe](a3234fe)) * **xy:** optionally rounds the domain to nice values ([#1087](#1087)) ([f644cc4](f644cc4)) * **xy:** specify pixel and ratio width for bars ([#1114](#1114)) ([58de413](58de413)) * mosaic ([#1113](#1113)) ([64bdd88](64bdd88))

nickofthyme · 2021-04-13T14:28:13Z

🎉 This PR is included in version 28.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

feat: a11y chart figure

3558657

rshen91 added :accessibility Accessibility related issue :xy Bar/Line/Area chart related labels Apr 1, 2021

Merge remote-tracking branch 'upstream/master' into a11y-chart-type

6560602

test: add a11y tests

7e499b9

rshen91 marked this pull request as ready for review April 2, 2021 14:35

rshen91 requested review from myasonik and markov00 April 2, 2021 14:35

rshen91 added 2 commits April 2, 2021 11:20

Merge remote-tracking branch 'upstream/master' into a11y-chart-type

81926ae

Merge remote-tracking branch 'upstream/master' into a11y-chart-type

36cacf7

nickofthyme approved these changes Apr 5, 2021

View reviewed changes

markov00 requested changes Apr 6, 2021

View reviewed changes

Merge remote-tracking branch 'upstream/master' into a11y-chart-type

d091a14

rshen91 added 2 commits April 6, 2021 16:18

feat: add code review changes

e7d1230

Merge remote-tracking branch 'upstream/master' into a11y-chart-type

5ef8ace

rshen91 requested a review from markov00 April 6, 2021 22:22

rshen91 added 4 commits April 6, 2021 16:31

style: add spaces in the <dt> tags for voiceover readability

01a46af

Merge remote-tracking branch 'upstream/master' into a11y-chart-type

947b29f

refactor: add selector

9e036d3

Merge remote-tracking branch 'upstream/master' into a11y-chart-type

a03433f

test: update tests

bf0e013

myasonik approved these changes Apr 8, 2021

View reviewed changes

markov00 approved these changes Apr 12, 2021

View reviewed changes

rshen91 added 2 commits April 12, 2021 08:07

Merge remote-tracking branch 'upstream/master' into a11y-chart-type

5935818

fix: add code review feedback

9b11e85

rshen91 merged commit 815cf39 into elastic:master Apr 12, 2021

rshen91 deleted the a11y-chart-type branch April 12, 2021 15:48

nickofthyme added the released Issue released publicly label Apr 13, 2021

rshen91 mentioned this pull request Apr 14, 2021

Add chart type to chart label #1098

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(a11y): improve chart figure #1104

feat(a11y): improve chart figure #1104

rshen91 commented Apr 1, 2021 •

edited

Loading

codecov-io commented Apr 1, 2021 •

edited

Loading

rshen91 commented Apr 2, 2021

nickofthyme left a comment

nickofthyme Apr 5, 2021

nickofthyme Apr 5, 2021

rshen91 Apr 6, 2021

markov00 left a comment

markov00 Apr 2, 2021

rshen91 Apr 6, 2021

markov00 Apr 6, 2021

myasonik Apr 6, 2021

rshen91 Apr 6, 2021

markov00 Apr 7, 2021

rshen91 Apr 7, 2021

markov00 Apr 6, 2021

rshen91 Apr 6, 2021

rshen91 Apr 6, 2021

myasonik commented Apr 6, 2021

rshen91 commented Apr 7, 2021

myasonik left a comment

markov00 left a comment

markov00 Apr 8, 2021

rshen91 Apr 12, 2021

markov00 Apr 8, 2021

rshen91 Apr 12, 2021

markov00 Apr 8, 2021

rshen91 Apr 12, 2021

markov00 Apr 8, 2021

rshen91 commented Apr 12, 2021

nickofthyme commented Apr 13, 2021


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

		describe('Accessibility tree', () => {

		const multipleSeriesTypes: string[] = [];
		seriesTypes.forEach((value) => multipleSeriesTypes.push(value));

feat(a11y): improve chart figure #1104

feat(a11y): improve chart figure #1104

Conversation

rshen91 commented Apr 1, 2021 • edited Loading

Summary

Checklist

codecov-io commented Apr 1, 2021 • edited Loading

Codecov Report

rshen91 commented Apr 2, 2021

nickofthyme 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

markov00 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

myasonik commented Apr 6, 2021

rshen91 commented Apr 7, 2021

myasonik left a comment

Choose a reason for hiding this comment

markov00 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

rshen91 commented Apr 12, 2021

nickofthyme commented Apr 13, 2021

rshen91 commented Apr 1, 2021 •

edited

Loading

codecov-io commented Apr 1, 2021 •

edited

Loading