Charts: provide an accessible title and description - #2500
Conversation
Sorry, something went wrong.
|
PatternFly-React preview: https://patternfly-react-pr-2500.surge.sh |
Sorry, something went wrong.
|
Conflicts with #2497 which just went in. |
Sorry, something went wrong.
|
@dlabrecq can you please link associated issue. Thanks |
Sorry, something went wrong.
jschuler
left a comment
There was a problem hiding this 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
Sorry, something went wrong.
|
Thanks @jschuler . I've renamed those properties as ariaDesc and ariaTitle. |
Sorry, something went wrong.
|
Your changes have been released in:
Thanks for your contribution! 🎉 |
Sorry, something went wrong.
mattnolting
left a comment
There was a problem hiding this 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.
Sorry, something went wrong.
| <div className="area-chart-legend-bottom"> | ||
| <Chart | ||
| ariaDesc="Average number of pets" | ||
| ariaTitle="Donut chart example" |
There was a problem hiding this comment.
Should be "Area chart example"
Sorry, something went wrong.
| <div className="area-chart-legend-bottom-responsive"> | ||
| <Chart | ||
| ariaDesc="Average number of pets" | ||
| ariaTitle="Donut chart example" |
There was a problem hiding this comment.
Should be "Area chart example"
Sorry, something went wrong.
There was a problem hiding this comment.
Thank you. I'll create a PR to fix the title text.
Sorry, something went wrong.
|
@mattnolting Created relevant issue and followup PR for you. |
Sorry, something went wrong.
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:
Fixes #1498