Accessible Interface for Line/Area Charts #294

ekh64 · 2021-07-13T18:46:31Z

iezer

iezer · 2021-07-13T21:44:46Z

docs/src/docs/A11yInterface/examples/OneDataset.js.example

+        `x Value: ${data[frameIndex].x}, y Value: ${data[frameIndex].y}`
+        }
+        numFrames={data.length}
+       />


I had this idea to simplify the API by using dependency injection and injecting the A11yInterface into LineChart so that a) the API is more declarative and b) we can be more explicit about which components support keyboard navigation. Then I realized it doesn't work for multiple line charts in the same XYPlot which is probably why you didn't go with this in the 1st place!

Still sharing for discussion. Is there any way we could do something like this and still support multiple charts in the same XYPlot?

return <XYPlot scaleType="linear" width={600} height={350}> <XAxis title="X Axis" /> <YAxis title="Y Axis" /> <LineChart data={data} x={d => d.x} y={d => d.y} lineStyle={{stroke: '#ff7f0e', strokeWidth: 3}} ariaLabel={(d, index_in_case_we_need_it) => {/* rename ariaLabelGenerator -> ariaLabel */} `x Value: ${d.x}, y Value: ${d.y}` } numAriaLabels={data.length} {/* Do we even need this? Could be optional and default to data.length*/} /> </XYPlot>;

I'm guessing inside of LineChart we'd add something like this:

{ props.ariaLabel ? <A11yInterface ariaLabelGenerator={props.ariaLabel} numFrames={props.numAriaLabels || props.data.length} /> : null }

We can even provide a sensible default ariaLabelGenerator function for LineChart and then the caller could get this for free.

As I mentioned in another comment, I'd prefer to leave this as a standalone component because this would break if we were to use multiple LineCharts, or Line + Area Charts. My project leverages rendering multiple data in different linecharts, and this built in schema would not work for that use case. However if we export the A11yInterface as an optional component and do this as well, I think that would be great.

iezer · 2021-07-13T21:53:31Z

src/A11yInterface.js

+ * [aria-labels](https://www.w3.org/TR/WCAG20-TECHS/ARIA6.html#ARIA6-description) are critical for users accessing
+ * the web with screenreaders or other assistive technologies.
+ */
+


I really appreciate the detailed comment here.

I'm thinking of a few other ways to tweak the API here to make it more like other Reactochart functions. Why not just pass data as a prop to A11yInterface and make ariaLabel a function that takes a datum and an optional index, similar to most D3 functions? Then numFrames (which I would rename numLabels) could default to data.length and probably doesn't even have to be passed in. It seems like most of the implementations of ariaLabelGenerator need the data anyway.

So I thought a lot about using data. It becomes a little more complex when dealing with multiple data sets. For example, if you have two line charts in an XYPlot, you would only want one A11yInterface, and that A11yInterface would need to account for both data sets. This makes calculating the number of frames a little complicated since data1 and data2 can be different lengths, and can contain overlapping values.

I wanted to avoid looping over the data to find all unique data points, though since lodash is a dependency here, I suppose we could find the union of x values more easily. unionBy seems promising and using Reactochart's passed in dataset prop, this could work. However, it would make generating the aria-labels a little different, since then we'd be working with the x value, as opposed to the frameIndex. This would also make the Interface more robust with data sets of unequal lengths and unequal x values.

iezer · 2021-07-13T21:53:49Z

src/A11yInterface.js

+          x={
+            sliceWidth *
+            (frameIndex === numFrames - 1 ? frameIndex - 1 : frameIndex)
+            // otherwise the last rect renders outside the chart


Thanks for the helpful comment!

iezer · 2021-07-13T21:55:23Z

tests/jsdom/spec/A11yInterface.spec.js

+    rects.at(0).simulate('keydown');
+    expect(onKeyDown).toHaveBeenCalled();
+  });
+});


Thanks for adding these clear tests.

iezer · 2021-07-13T21:56:58Z

src/A11yInterface.js

+ */
+
+export default function A11yInterface(props) {
+  const { ariaLabelGenerator, numFrames, onKeyDown, height, width } = props;


I wonder if we can brainstorm some better names for this component that describes what it does. There are many other parts to A11y that we might tackle in the future using different components. How about AriaLabelContainer, AriaLabelChart or InteractiveAriaLabels?

Totally open to renaming, I like AriaLabelContainer

ekh64 · 2021-07-14T19:58:56Z

iezer · 2021-07-16T15:54:51Z

src/A11yInterface.js

        <rect
          className="rct-chart-visually-hidden-rect"
-          aria-label={ariaLabelGenerator(frameIndex)}
-          key={frameIndex}
+          aria-label={ariaLabelGenerator(d)}


This variable name is a bit confusing because if I'm reading this correct it seems like d is really an xValue. Seems like ariaLabelGenerator will almost always want the array of all data points for that xValue so how about figuring that out here in order to simplify the handlers that are passed in? This line could be something like

aria-label={ariaLabelGenerator(xValue, data, index)}

where data is the array of data just or that xValue, not the whole data for the chart.

You'll probably have to change line 78 to something like this:

data.map(d => [d, getValue(accessor, d)]),

My one concern is this approach could lose the context of the data. We can return the data points, but it could become unclear to the end user which data point belongs to which dataset it belongs to. For instance, I could have a schema like:

{ name: 'Beyonce' data: [array of values to be rendered] } { name: 'Drake' data: [...] }

In the schema I defined, that context (name) is lost when returning the two datapoints for a given xValue, and since aria-labels need to provide that context so the user understand what they're hearing, I'm not sure this is a better approach.

Can you give a full example with that structure? All the examples just have an array of data with no name. Isn't the same end user who's defining ariaLabelGenerator also defining datasetWithAccessor and could reasonably assume that the data points come in the same order?

The issue is less with the datapoint and more with needing context outside of the array used to generate the LineChart, which currently is all that's being used in datasetWithAccessor

So in the example I provided, we'd only be passing in the data attribute to the LineChart and datasetWithAccessor, but we'd want to know what name data0 and data1 belong to in the ariaLabelGenerator. The user can define this themselves if we provide the xValue, but I worry about providing the other arguments if the end user doesn't actually get value given multiple datasets. They'd have to do that figuring out themselves anyway.

Oh I see, my suggestion was to pass the xValue and the data (and even the index in case people need it) since I'm guessing in 99% of cases (and both your docs examples) users will want the data. So in your 2 datasets example, instead of

const ariaLabelGenerator = xValue => { const data0Point = data0.find(d => d.x === xValue); const data1Point = data1.find(d => d.x === xValue); let ariaLabelString = `x Value, ${xValue}`; if(data0Point) { ariaLabelString += `, data0 y Value ${data0Point.y}`; } if(data1Point) { ariaLabelString += `, data1 y Value ${data1Point.y}`; } return ariaLabelString; }

you could have

const ariaLabelGenerator = (xValue, [data0Point, data1Point], /* index */) => { let ariaLabelString = `x Value, ${xValue}`; if(data0Point) { ariaLabelString += `, data0 y Value ${data0Point.y}`; } if(data1Point) { ariaLabelString += `, data1 y Value ${data1Point.y}`; } return ariaLabelString; }

Sounds good, I'll make those changes this week and update the PR 👍

ekh64 · 2021-12-08T21:13:57Z

iezer

iezer · 2021-12-09T13:56:40Z

src/AriaLabelContainer.js

+      {zippedDatapoints.map(({ xValue, data }, index) => (
+        <rect
+          className="rct-chart-visually-hidden-rect"
+          aria-label={ariaLabelGenerator(xValue, data, index)}


👍 this looks great.

ekh64 requested review from acmei, iezer and scottsheffield as code owners July 13, 2021 18:46

scottsheffield approved these changes Jul 13, 2021

View reviewed changes

iezer suggested changes Jul 13, 2021

View reviewed changes

iezer reviewed Jul 16, 2021

View reviewed changes

ekh64 added 11 commits November 10, 2021 14:47

A11yInterface in reactochart

cefbd76

npm run make-docs + render A11yInterface docs

c094fa2

fix offset bug + more examples

d2d3d79

refactor for simplicity

28103af

revert the styling since the outline wasn't a problem

4281c43

A11yInterface Tests

3648475

refactor to use datasetsWithAccessor instead

5195585

rename + doc generation

0d32ae5

wip: refactor of AriaLabelContainer

57fdf3d

fix an example, fix bug with chart

5336403

fix my examples & refactor again

89c8f42

ekh64 force-pushed the ehyman/a11y branch from 94d3118 to 89c8f42 Compare December 8, 2021 21:05

update package-lock

aaf6a1a

iezer approved these changes Dec 9, 2021

View reviewed changes

ekh64 merged commit 50d2532 into spotify:master Dec 9, 2021

ekh64 deleted the ehyman/a11y branch December 9, 2021 15:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Accessible Interface for Line/Area Charts #294

Accessible Interface for Line/Area Charts #294

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Accessible Interface for Line/Area Charts #294

Accessible Interface for Line/Area Charts #294

Uh oh!

Conversation

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!