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): add label for screen readers #1121

Merged
merged 16 commits into from
Apr 22, 2021
Merged
Changes from 1 commit
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
Prev Previous commit
Next Next commit
feat: add code review feedback
rshen91 committed Apr 20, 2021
commit 19fb650fac88c85f7241d793f2ddeb1588b3fbef
57 changes: 36 additions & 21 deletions src/chart_types/xy_chart/renderer/canvas/xy_chart.tsx
Original file line number Diff line number Diff line change
@@ -79,11 +79,11 @@ export interface ReactiveChartStateProps {
annotationSpecs: AnnotationSpec[];
panelGeoms: PanelGeoms;
seriesTypes: Set<SeriesType>;
description?: string;
accessibilityDescription?: string;
useDefaultSummary: boolean;
chartId: string;
label?: string;
labelledBy?: string;
ariaLabel?: string;
ariaLabelledBy?: string;
HeadingLevel: 'h1' | 'h2' | 'h3' | 'h4' | 'h5' | 'h6' | 'p';
Copy link
Member

Choose a reason for hiding this comment

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

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

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 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?

}

@@ -95,6 +95,10 @@ interface ReactiveChartOwnProps {
}
const CLIPPING_MARGINS = 0.5;

type AriaProps = {
[key: string]: string | undefined;
};

type XYChartProps = ReactiveChartStateProps & ReactiveChartDispatchProps & ReactiveChartOwnProps;
class XYChartComponent extends React.Component<XYChartProps> {
static displayName = 'XYChart';
@@ -162,11 +166,11 @@ class XYChartComponent extends React.Component<XYChartProps> {
isChartEmpty,
chartContainerDimensions: { width, height },
seriesTypes,
description,
accessibilityDescription,
useDefaultSummary,
chartId,
label,
labelledBy,
ariaLabel,
ariaLabelledBy,
HeadingLevel,
} = this.props;

@@ -178,12 +182,16 @@ class XYChartComponent extends React.Component<XYChartProps> {
const chartSeriesTypes =
seriesTypes.size > 1 ? `Mixed chart: ${[...seriesTypes].join(' and ')} chart` : `${[...seriesTypes]} chart`;
const chartIdDescription = `${chartId}--description`;
const chartIdLabel = label ? `${chartId}--label` : undefined;
const chartIdLabel = ariaLabel ? `${chartId}--label` : undefined;

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

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

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Woops added this in 6150d2d thank you!

return (
<figure {...ariaProps}>
<canvas
@@ -198,10 +206,10 @@ class XYChartComponent extends React.Component<XYChartProps> {
// eslint-disable-next-line jsx-a11y/no-interactive-element-to-noninteractive-role
role="presentation"
>
{label && <HeadingLevel id={chartIdLabel}>{label}</HeadingLevel>}
{(description || useDefaultSummary) && (
{ariaLabel && <HeadingLevel id={chartIdLabel}>{ariaLabel}</HeadingLevel>}
{(accessibilityDescription || useDefaultSummary) && (
<div className="echScreenReaderOnly">
{description && <p id={chartIdDescription}>{description}</p>}
{accessibilityDescription && <p id={chartIdDescription}>{accessibilityDescription}</p>}
{useDefaultSummary && (
<dl>
<dt>Chart type</dt>
@@ -264,11 +272,11 @@ const DEFAULT_PROPS: ReactiveChartStateProps = {
annotationSpecs: [],
panelGeoms: [],
seriesTypes: new Set(),
description: undefined,
accessibilityDescription: undefined,
useDefaultSummary: true,
chartId: '',
label: undefined,
labelledBy: undefined,
ariaLabel: undefined,
ariaLabelledBy: undefined,
HeadingLevel: 'h2',
};

@@ -278,7 +286,14 @@ const mapStateToProps = (state: GlobalChartState): ReactiveChartStateProps => {
}

const { geometries, geometriesIndex } = computeSeriesGeometriesSelector(state);
const { debug, description, useDefaultSummary, label, labelledBy, HeadingLevel } = getSettingsSpecSelector(state);
const {
debug,
accessibilityDescription,
useDefaultSummary,
ariaLabel,
ariaLabelledBy,
HeadingLevel,
} = getSettingsSpecSelector(state);

return {
initialized: true,
@@ -300,11 +315,11 @@ const mapStateToProps = (state: GlobalChartState): ReactiveChartStateProps => {
annotationSpecs: getAnnotationSpecsSelector(state),
panelGeoms: computePanelsSelectors(state),
seriesTypes: getSeriesTypes(state),
description,
accessibilityDescription,
useDefaultSummary,
chartId: getChartIdSelector(state),
label,
labelledBy,
ariaLabel,
ariaLabelledBy,
HeadingLevel,
};
};
Original file line number Diff line number Diff line change
@@ -43,12 +43,18 @@ describe('test accessibility prop defaults', () => {
});
it('should test defaults', () => {
const state = store.getState();
const { description, useDefaultSummary, HeadingLevel, label, labelledBy } = getSettingsSpecSelector(state);
expect(description).toBeUndefined();
const {
accessibilityDescription,
useDefaultSummary,
HeadingLevel,
ariaLabel,
ariaLabelledBy,
} = getSettingsSpecSelector(state);
expect(accessibilityDescription).toBeUndefined();
expect(useDefaultSummary).toBeTrue();
expect(HeadingLevel).toBe('h2');
expect(label).toBeUndefined();
expect(labelledBy).toBeUndefined();
expect(ariaLabel).toBeUndefined();
expect(ariaLabelledBy).toBeUndefined();
});
});
describe('custom description for screen readers', () => {
@@ -72,14 +78,14 @@ describe('custom description for screen readers', () => {
MockStore.addSpecs(
[
MockGlobalSpec.settings({
description: 'This is sample Kibana data',
accessibilityDescription: 'This is sample Kibana data',
}),
],
store,
);
const state = store.getState();
const { description } = getSettingsSpecSelector(state);
expect(description).toBe('This is sample Kibana data');
const { accessibilityDescription } = getSettingsSpecSelector(state);
expect(accessibilityDescription).toBe('This is sample Kibana data');
});
it('should be able to disable generated descriptions', () => {
MockStore.addSpecs(
@@ -116,27 +122,27 @@ describe('custom labels for screen readers', () => {
MockStore.addSpecs(
[
MockGlobalSpec.settings({
label: 'Label set by user',
ariaLabel: 'Label set by user',
}),
],
store,
);
const state = store.getState();
const { label } = getSettingsSpecSelector(state);
expect(label).toBeTruthy();
const { ariaLabel } = getSettingsSpecSelector(state);
expect(ariaLabel).toBeTruthy();
});
it('should allow labelledBy set by the user', () => {
MockStore.addSpecs(
[
MockGlobalSpec.settings({
labelledBy: 'Label',
ariaLabelledBy: 'Label',
}),
],
store,
);
const state = store.getState();
const { labelledBy } = getSettingsSpecSelector(state);
expect(labelledBy).toBeTruthy();
const { ariaLabelledBy } = getSettingsSpecSelector(state);
expect(ariaLabelledBy).toBeTruthy();
});
it('should allow users to specify valid heading levels', () => {
MockStore.addSpecs(
4 changes: 2 additions & 2 deletions src/components/__snapshots__/chart.test.tsx.snap
Original file line number Diff line number Diff line change
@@ -72,8 +72,8 @@ exports[`Chart should render the legend name test 1`] = `
</Crosshair>
</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={{...}} seriesTypes={{...}} description={[undefined]} useDefaultSummary={true} chartId=\\"chart1\\" label={[undefined]} labelledBy={[undefined]} HeadingLevel=\\"h2\\" onChartRendered={[Function (anonymous)]}>
<figure aria-labelledby=\\"\\" aria-describedby=\\" undefined\\">
<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={{...}} seriesTypes={{...}} accessibilityDescription={[undefined]} useDefaultSummary={true} chartId=\\"chart1\\" ariaLabel={[undefined]} ariaLabelledBy={[undefined]} HeadingLevel=\\"h2\\" onChartRendered={[Function (anonymous)]}>
<figure aria-describedby=\\" undefined\\">
<canvas className=\\"echCanvasRenderer\\" width={150} height={200} style={{...}} role=\\"presentation\\">
<div className=\\"echScreenReaderOnly\\">
<dl>
10 changes: 5 additions & 5 deletions src/specs/settings.tsx
Original file line number Diff line number Diff line change
@@ -556,15 +556,15 @@ export interface SettingsSpec extends Spec, LegendSpec {
/**
* User can set a label
*/
label?: string;
ariaLabel?: string;
/**
* User can set a string to the aria-labelled by
* */
labelledBy?: string;
ariaLabelledBy?: string;
/**
* User can provide a custom description to be read by a screen reader about their chart
*/
description?: string;
accessibilityDescription?: string;
/**
* Disable the automated charts series types from being provided for screen readers
* @defaultValue true
@@ -631,9 +631,9 @@ export type DefaultSettingsProps =
| 'showLegendExtra'
| 'legendPosition'
| 'legendMaxDepth'
| 'description'
| 'accessibilityDescription'
| 'useDefaultSummary'
| 'label'
| 'ariaLabel'
| 'HeadingLevel';

/** @public */
4 changes: 2 additions & 2 deletions stories/test_cases/6_a11y_custom_description.tsx
Original file line number Diff line number Diff line change
@@ -33,9 +33,9 @@ export const Example = () => {
return (
<Chart className="story-chart">
<Settings
description={customDescriptionForScreenReaders}
accessibilityDescription={customDescriptionForScreenReaders}
useDefaultSummary={automatedSeries}
label={customLabelForScreenReaders}
ariaLabel={customLabelForScreenReaders}
HeadingLevel={headingLevelForScreenReaders}
/>
<AreaSeries