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

Charts: provide an accessible title and description #2500

Merged
merged 1 commit into from Jul 12, 2019

Conversation

@dlabrecq
Copy link
Member

dlabrecq commented Jul 11, 2019

Victory provides the ability to add a description and title to SVGs for accessibility. However, users must override containerComponent just to set these properties -- it's not clear how to provide accessibility.

In order to make this more intuitive, I've added ariaDesc and ariaTitle properties to our top level components; Chart, ChartGroup, etc. The property names we chosen because ChartDonut and ChartDonutUtilization already have title and subTitle properties; thus, avoiding a breaking change.

Note that these top-level properties just pass through to the container's desc and title props. The underlying SVG refers to desc and title tags via the SVG's aria-labeledby attribute.

The approach allows users to continue using the allowZoom prop of Chart and ChartGroup without overriding containerComponent. If the user must override containerComponent, the allowZoom property would not be able to provide a default VictoryZoomContainer component.

These top-level properties also help to set desc and title for ChartPie, ChartDonut etc. Overriding containerComponent would not work in this particular case because the chart is wrapped with a ChartContainer component and rendered standalone=false -- required to show title, subTitle, and legend within the same SVG. The desc and title properties must be applied to the outermost container.

Example output:
Screen Shot 2019-07-11 at 2 01 34 AM

Fixes #1498

@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Jul 11, 2019

PatternFly-React preview: https://patternfly-react-pr-2500.surge.sh

@redallen

This comment has been minimized.

Copy link
Contributor

redallen commented Jul 11, 2019

Conflicts with #2497 which just went in.

@dlabrecq dlabrecq force-pushed the dlabrecq:1498-aria-props branch 3 times, most recently from 6c3225c to 1741fcc Jul 11, 2019
@tlabaj tlabaj requested review from jcaianirh and TheRealJon Jul 11, 2019
@tlabaj tlabaj assigned jcaianirh and jessiehuff and unassigned jcaianirh Jul 11, 2019
@tlabaj tlabaj added the PF4 label Jul 11, 2019
@tlabaj

This comment has been minimized.

Copy link
Contributor

tlabaj commented Jul 11, 2019

@dlabrecq can you please link associated issue. Thanks

Copy link
Collaborator

jschuler left a comment

Maybe down to personal preference, but I would drop Label from the props and name them just ariaDesc and ariaTitle. Otherwise it looks good

@dlabrecq

This comment has been minimized.

Copy link
Member Author

dlabrecq commented Jul 11, 2019

Thanks @jschuler . I've renamed those properties as ariaDesc and ariaTitle.

@tlabaj tlabaj requested a review from jgiardino Jul 11, 2019
@dlabrecq dlabrecq force-pushed the dlabrecq:1498-aria-props branch 3 times, most recently from 3d1cbf0 to 7e0537d Jul 11, 2019
@dlabrecq dlabrecq force-pushed the dlabrecq:1498-aria-props branch from 7e0537d to 4a404e5 Jul 11, 2019
@dlabrecq dlabrecq removed request for TheRealJon and jcaianirh Jul 11, 2019
@redallen redallen merged commit 0e5ac2b into patternfly:master Jul 12, 2019
8 checks passed
8 checks passed
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: build_integration Your tests passed on CircleCI!
Details
ci/circleci: build_pf3_docs Your tests passed on CircleCI!
Details
ci/circleci: build_pf4_docs Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: test_jest_other Your tests passed on CircleCI!
Details
ci/circleci: test_jest_pf4 Your tests passed on CircleCI!
Details
ci/circleci: upload_docs Your tests passed on CircleCI!
Details
@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Jul 12, 2019

Your changes have been released in:

  • @patternfly/react-charts@4.6.0

Thanks for your contribution! 🎉

Copy link
Contributor

mattnolting left a comment

I think this is a great start to making chart visuals accessible! To @jgiardino's point, the visual representation benefits from a title and desc. However, the data presented is not traversable, which presents a broader issue. Some possible approaches are adding role="group" to charts, then selectively adding role="presentation" to elements we want to hide from SRs. Another is to generate a hidden table that contains the data represented visually. Either way, that's a different issue. Just found two issues with desc in area charts.

```js
import React from 'react';
import { Chart, ChartArea, ChartAxis, ChartGroup, ChartThemeColor } from '@patternfly/react-charts';
<div>
<div className="area-chart-legend-bottom">
<Chart
ariaDesc="Average number of pets"
ariaTitle="Donut chart example"

This comment has been minimized.

Copy link
@mattnolting

mattnolting Jul 12, 2019

Contributor

Should be "Area chart example"

@@ -163,6 +167,8 @@ class MultiColorChart extends React.Component {
<div ref={this.containerRef}>
<div className="area-chart-legend-bottom-responsive">
<Chart
ariaDesc="Average number of pets"
ariaTitle="Donut chart example"

This comment has been minimized.

Copy link
@mattnolting

mattnolting Jul 12, 2019

Contributor

Should be "Area chart example"

This comment has been minimized.

Copy link
@dlabrecq

dlabrecq Jul 12, 2019

Author Member

Thank you. I'll create a PR to fix the title text.

@redallen

This comment has been minimized.

Copy link
Contributor

redallen commented Jul 12, 2019

@mattnolting Created relevant issue and followup PR for you.

@dlabrecq dlabrecq deleted the dlabrecq:1498-aria-props branch Jul 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.