feat(a11y): add label for screen readers #1121

rshen91 · 2021-04-14T20:53:12Z

Summary

This PR allows the consumer to set the heading level (or as a p tag), a label, and labelledBy aria-prop for a cartesian chart that can be read by a screen reader.

PR1121.mp4

Renamed the story within test_cases to accessibility customizations with a knob for this change.

Checklist

Delete any items that are not applicable to this PR.

Any consumer-facing exports were added to src/index.ts (and stories only import from ../src except for test data & storybook)
Unit tests were updated or added to match the most common scenarios

codecov-io · 2021-04-14T21:19:17Z

Codecov Report

Merging #1121 (e1c4fcd) into master (c1b59f2) will increase coverage by 0.64%.
The diff coverage is 97.36%.

@@            Coverage Diff             @@
##           master    #1121      +/-   ##
==========================================
+ Coverage   72.26%   72.90%   +0.64%     
==========================================
  Files         387      405      +18     
  Lines       12021    12388     +367     
  Branches     2629     2680      +51     
==========================================
+ Hits         8687     9032     +345     
- Misses       3309     3322      +13     
- Partials       25       34       +9

Flag	Coverage Δ
unittests	`72.48% <97.36%> (+0.21%)`	⬆️

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

Impacted Files	Coverage Δ
src/specs/constants.ts	`100.00% <ø> (ø)`
src/specs/settings.tsx	`90.32% <ø> (ø)`
.../chart_types/partition_chart/state/chart_state.tsx	`70.68% <66.66%> (+0.51%)`	⬆️
...partition_chart/state/selectors/get_debug_state.ts	`100.00% <100.00%> (ø)`
.../chart_types/xy_chart/renderer/canvas/xy_chart.tsx	`95.69% <100.00%> (+0.19%)`	⬆️
src/components/chart_status.tsx	`91.66% <100.00%> (ø)`
src/mocks/series/series.ts	`91.22% <0.00%> (ø)`
src/mocks/xy/domains.ts	`89.47% <0.00%> (ø)`
src/mocks/scale/index.ts	`100.00% <0.00%> (ø)`
src/mocks/store/store.ts	`92.59% <0.00%> (ø)`
... and 18 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 8017edd...e1c4fcd. Read the comment docs.

myasonik

Looking p good overall!

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

myasonik

Left a couple small code comments but otherwise looks good!

(Holding off approving until I have a chance to run through it in VoiceOver though.)

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

myasonik · 2021-04-19T18:59:05Z

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

+
+    const ariaProps = {
+      'aria-labelledby': labelledBy || label ? labelledBy ?? chartIdLabel : '',
+      'aria-describedby': `${labelledBy || ''} ${useDefaultSummary ? chartIdLabel : ''}`,


Did you mean to use chartIdDescription here?

Suggested change

'aria-describedby': `${labelledBy || ''} ${useDefaultSummary ? chartIdLabel : ''}`,

'aria-describedby': `${labelledBy || ''} ${useDefaultSummary ? chartIdDescription : ''}`,

And I'm also confused by the fact that we are reusing labelledByin the describedBy. Should we introduce a new props called describedBy?

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

codecov-commenter · 2021-04-20T16:01:03Z

Codecov Report

Merging #1121 (e1c4fcd) into master (c1b59f2) will increase coverage by 0.64%.
The diff coverage is 97.36%.

❗ Current head e1c4fcd differs from pull request most recent head deff638. Consider uploading reports for the commit deff638 to get more accurate results

@@            Coverage Diff             @@
##           master    #1121      +/-   ##
==========================================
+ Coverage   72.26%   72.90%   +0.64%     
==========================================
  Files         387      405      +18     
  Lines       12021    12388     +367     
  Branches     2629     2680      +51     
==========================================
+ Hits         8687     9032     +345     
- Misses       3309     3322      +13     
- Partials       25       34       +9

Flag	Coverage Δ
unittests	`72.48% <97.36%> (+0.21%)`	⬆️

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

Impacted Files	Coverage Δ
src/specs/constants.ts	`100.00% <ø> (ø)`
src/specs/settings.tsx	`90.32% <ø> (ø)`
.../chart_types/partition_chart/state/chart_state.tsx	`70.68% <66.66%> (+0.51%)`	⬆️
...partition_chart/state/selectors/get_debug_state.ts	`100.00% <100.00%> (ø)`
.../chart_types/xy_chart/renderer/canvas/xy_chart.tsx	`95.69% <100.00%> (+0.19%)`	⬆️
src/components/chart_status.tsx	`91.66% <100.00%> (ø)`
src/mocks/index.ts	`100.00% <0.00%> (ø)`
src/mocks/utils.ts	`100.00% <0.00%> (ø)`
src/mocks/xy/domains.ts	`89.47% <0.00%> (ø)`
src/mocks/store/store.ts	`92.59% <0.00%> (ø)`
... and 18 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 ab82bde...deff638. Read the comment docs.

markov00 · 2021-04-20T15:44:24Z

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

@@ -82,6 +82,9 @@ export interface ReactiveChartStateProps {
  description?: string;
  useDefaultSummary: boolean;
  chartId: string;
+  label?: string;
+  labelledBy?: string;
+  HeadingLevel: 'h1' | 'h2' | 'h3' | 'h4' | 'h5' | 'h6' | 'p';


each prop name should follow the camelCase with the first char lowercase, independently if you use them later as component

We should also sanitize this: it's true that in a TS this will throw error if the value is one of the union type, but in the JS compiled world not. This can lead to errors if this heading comes from an user input, or stored string value.
we can have a function that checks and returns/construct the appropriate header based on the input, like:

{label && <HeadingLevel id={chartIdLabel}>{label}</HeadingLevel>} const ChartLabel = ({headingLevel: 'h1' | 'h2' | 'h3' | 'h4' | 'h5' | 'h6' | 'p', id: string, label?: string}) => { if(!label) return null; switch(headingLevel){ case 'h1': return <h1 id={id}>{label}</h1> ...... } } ..... <ChartLabel id={chartIdLabel} label={label} headingLevel={headingLevel} />

@nickofthyme other suggestions?

markov00 · 2021-04-20T16:03:39Z

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

+
+    const ariaProps = {
+      'aria-labelledby': labelledBy || label ? labelledBy ?? chartIdLabel : '',
+      'aria-describedby': `${labelledBy || ''} ${useDefaultSummary ? chartIdLabel : ''}`,


And I'm also confused by the fact that we are reusing labelledByin the describedBy. Should we introduce a new props called describedBy?

rshen91 · 2021-04-20T16:35:56Z

@markov00 @nickofthyme do you think it would be better in terms of Kibana dashboards etc. for the headingLevel to have the default being a <p> vs a <h2>? With an <h2> it's very easily navigable, but do you think it will be overwhelming for users? TY

myasonik · 2021-04-20T16:42:01Z

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

+    if (ariaLabelledBy || useDefaultSummary) {
+      ariaProps['aria-describedby'] = `${ariaLabelledBy || ''} ${useDefaultSummary ? chartIdLabel : undefined}`;
+    }


@markov00 Good catch in #1121 (comment)

Adding a suggestion here so I could target more lines, I think the ariaLabelledBy stuff snuck in by mistake. It should probably be something like:

Suggested change

if (ariaLabelledBy || useDefaultSummary) {

ariaProps['aria-describedby'] = `${ariaLabelledBy || ''} ${useDefaultSummary ? chartIdLabel : undefined}`;

}

if (accessibilityDescription || useDefaultSummary) {

ariaProps['aria-describedby'] = `${accessibilityDescription ? chartIdDescription : undefined} ${useDefaultSummary ? aNewIdForTheChartSeriesTypesDD : undefined}`;

}

(I'm suggesting making a new id just for the chartSeriesTypes <dd> because description text can't be structured data so we can't just set an id for the whole <dl> and use that. It's not perfect but it works well enough for a quick description imo.

@rshen91 Feel free to reach out if this suggestion doesn't make any sense! It's a little convoluted.

Woops added this in 6150d2d thank you!

myasonik

One small tweak but otherwise looks good to me! Ship it!

This will be an awesome one to get integrated into Dashboards!

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

rshen91 · 2021-04-21T19:25:04Z

jenkins test this

This commit extracts all the Settings props related to accessibility into an internal configuration, and reuse that configuration within the code without thinking about defaults" BREAKING CHANGE: `description` prop in `<Settings/>` is renamed to `ariaDescription`

markov00

LGTM!
Remember to add the BREAKING CHANGE: footer when merging

myasonik · 2021-04-22T15:50:05Z

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

+              <dl id={a11ySettings.defaultSummaryId}>
+                <dt>Chart type</dt>
+                <dd>{chartSeriesTypes}</dd>
+              </dl>


@markov00 Setting the summary id here would be ideal but aria-labelledby can't reliably report structured content so we're better off putting it on a simple string. Reporting just the chart type seems appropriate for a supplemental description (better than nothing, type of situation).

Suggested change

<dl id={a11ySettings.defaultSummaryId}>

<dt>Chart type</dt>

<dd>{chartSeriesTypes}</dd>

</dl>

<dl>

<dt>Chart type</dt>

<dd id={a11ySettings.defaultSummaryId}>{chartSeriesTypes}</dd>

</dl>

committing in #1118

myasonik · 2021-04-22T15:51:37Z

src/specs/constants.ts

-  headingLevel: 'p',
-
+  ariaUseDefaultSummary: true,
+  ariaLabelHeadingLevel: 'h2',


@rshen91 @markov00

I thought we were going to use p to be less noisy on pages like Dashboards which can have tons of charts.

committing in #1118 thank you!

thanks sorry about that, I was reading the API docs and was marked as h2

# [29.0.0](v28.2.0...v29.0.0) (2021-04-22) ### Features * **a11y:** add label for screen readers ([#1121](#1121)) ([920e585](920e585)), closes [#1096](#1096) * **annotations:** marker body with dynamic positioning ([#1116](#1116)) ([601abac](601abac)) ### BREAKING CHANGES * **a11y:** `description` prop in `<Settings/>` is renamed to `ariaDescription` Co-authored-by: Marco Vettorello <vettorello.marco@gmail.com>

nickofthyme · 2021-04-22T19:56:52Z

🎉 This PR is included in version 29.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

# [29.0.0](elastic/elastic-charts@v28.2.0...v29.0.0) (2021-04-22) ### Features * **a11y:** add label for screen readers ([opensearch-project#1121](elastic/elastic-charts#1121)) ([ddb8782](elastic/elastic-charts@ddb8782)), closes [opensearch-project#1096](elastic/elastic-charts#1096) * **annotations:** marker body with dynamic positioning ([#1116](elastic/elastic-charts#1116)) ([997d487](elastic/elastic-charts@997d487)) ### BREAKING CHANGES * **a11y:** `description` prop in `<Settings/>` is renamed to `ariaDescription` Co-authored-by: Marco Vettorello <vettorello.marco@gmail.com>

feat: add label for screen readers

1087852

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

rshen91 added 2 commits April 14, 2021 15:32

test: update tests

4cb8ddc

Merge remote-tracking branch 'upstream/master' into add-labels

e1c4fcd

myasonik reviewed Apr 15, 2021

View reviewed changes

src/chart_types/xy_chart/renderer/canvas/xy_chart.tsx Outdated Show resolved Hide resolved

src/chart_types/xy_chart/renderer/canvas/xy_chart.tsx Outdated Show resolved Hide resolved

src/chart_types/xy_chart/renderer/canvas/xy_chart.tsx Outdated Show resolved Hide resolved

rshen91 added 2 commits April 15, 2021 13:07

feat: add aria props

e20033a

Merge remote-tracking branch 'upstream/master' into add-labels

2612a2a

rshen91 requested a review from myasonik April 15, 2021 20:26

test: add unit tests

a88737d

rshen91 marked this pull request as ready for review April 19, 2021 15:39

myasonik reviewed Apr 19, 2021

View reviewed changes

feat: add code review feedback

19fb650

Merge remote-tracking branch 'upstream/master' into add-labels

d9f34a7

markov00 reviewed Apr 20, 2021

View reviewed changes

fix: update chart api

532bd0d

myasonik reviewed Apr 20, 2021

View reviewed changes

rshen91 added 4 commits April 20, 2021 15:25

fix: add code review feedback

356b754

chore: update chart api

e770e99

test: updated missed snapshot test

c1a9b8d

fix: add idforchartseries types

6150d2d

rshen91 requested review from myasonik and markov00 April 21, 2021 15:05

myasonik approved these changes Apr 21, 2021

View reviewed changes

src/chart_types/xy_chart/renderer/canvas/xy_chart.tsx Outdated Show resolved Hide resolved

feat: remove aria-label

b81e6f8

test: update chart snap

deff638

markov00 approved these changes Apr 22, 2021

View reviewed changes

rshen91 merged commit 920e585 into elastic:master Apr 22, 2021

myasonik reviewed Apr 22, 2021

View reviewed changes

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

rshen91 mentioned this pull request Apr 29, 2021

feat(a11y): add alt text for all chart types #1118

Merged

1 task

rshen91 deleted the add-labels branch April 30, 2021 18:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(a11y): add label for screen readers #1121

feat(a11y): add label for screen readers #1121

rshen91 commented Apr 14, 2021 •

edited

Loading

codecov-io commented Apr 14, 2021 •

edited

Loading

myasonik left a comment

myasonik left a comment

myasonik Apr 19, 2021

markov00 Apr 20, 2021

codecov-commenter commented Apr 20, 2021 •

edited

Loading

markov00 Apr 20, 2021

markov00 Apr 20, 2021

markov00 Apr 20, 2021

rshen91 commented Apr 20, 2021

myasonik Apr 20, 2021

rshen91 Apr 21, 2021

myasonik left a comment

rshen91 commented Apr 21, 2021

markov00 left a comment

myasonik Apr 22, 2021

rshen91 Apr 22, 2021

myasonik Apr 22, 2021

rshen91 Apr 22, 2021

markov00 Apr 22, 2021

nickofthyme commented Apr 22, 2021

	'aria-describedby': `${labelledBy \|\| ''} ${useDefaultSummary ? chartIdLabel : ''}`,
	'aria-describedby': `${labelledBy \|\| ''} ${useDefaultSummary ? chartIdDescription : ''}`,

feat(a11y): add label for screen readers #1121

feat(a11y): add label for screen readers #1121

Conversation

rshen91 commented Apr 14, 2021 • edited Loading

Summary

Checklist

codecov-io commented Apr 14, 2021 • edited Loading

Codecov Report

myasonik left a comment

Choose a reason for hiding this comment

myasonik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Apr 20, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rshen91 commented Apr 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

myasonik left a comment

Choose a reason for hiding this comment

rshen91 commented Apr 21, 2021

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

nickofthyme commented Apr 22, 2021

rshen91 commented Apr 14, 2021 •

edited

Loading

codecov-io commented Apr 14, 2021 •

edited

Loading

codecov-commenter commented Apr 20, 2021 •

edited

Loading