Open sam-silv opened 7 years ago
You raise some great points! I guess we don't usually think about accessibility in visualization frameworks, but we obviously should be doing that.
I'm not incredibly familiar with the process of making things accessible, though we could certainly be better with using semantic tag names rather than just divs everywhere. Would you mind filing a pr to update one of the legends as an example of what we should be doing?
Sure thing, I'll have a go at putting together a PR for DiscreteColorLegend
, and I'll ponder approaches for a broader strategy
@mcnuttandrew would you have objections to adding something like eslint-plugin-jsx-a11y
to the config of react-vis
?
Using the recommended config, this is the resulting output
/Users/samh/Projects/react-vis/src/plot/voronoi.js
16:9 error onMouseOver must be accompanied by onFocus for accessibility jsx-a11y/mouse-events-have-key-events
16:9 error onMouseOut must be accompanied by onBlur for accessibility jsx-a11y/mouse-events-have-key-events
/Users/samh/Projects/react-vis/src/sankey/index.js
107:15 error onMouseOver must be accompanied by onFocus for accessibility jsx-a11y/mouse-events-have-key-events
107:15 error onMouseOut must be accompanied by onBlur for accessibility jsx-a11y/mouse-events-have-key-events
/Users/samh/Projects/react-vis/src/treemap/treemap-leaf.js
82:7 error Visible, non-interactive elements with click handlers must have at least one keyboard listener jsx-a11y/click-events-have-key-events
82:7 warning Visible, non-interactive elements should not have mouse or keyboard event listeners jsx-a11y/no-static-element-interactions
82:7 error Visible, non-interactive elements with click handlers must have role attribute jsx-a11y/onclick-has-role
/Users/samh/Projects/react-vis/showcase/index.js
78:9 error Links must not point to "#". Use a more descriptive href or use a button instead jsx-a11y/href-no-hash
/Users/samh/Projects/react-vis/showcase/plot/voronoi-line-chart.js
78:9 error Form controls using a label to identify them must be programmatically associated with the control using htmlFor jsx-a11y/label-has-for
✖ 9 problems (8 errors, 1 warning)
Interestingly, it doesn't catch the issue with DiscreteColorLegendItem
, probably due to the object spread notation used to add the onClick
prop to the div
.
Would like to hear your take, I don't think it's too much effort to put together something that is compliant.
@samhogg, I think adding eslint-plugin-jsx-ally is a splendid idea! Maybe you know more about the react-ally stuff than I do: do you know if the react-ally package will produce different results than the linter? If so should we use both?
@mcnuttandrew
I'm going to do a bit more digging, but from what I can tell, react-a11y
actually inspects rendered elements, so might catch more than the linter, which is just static. It does mutate the React
object though, so we should only use it when hacking on the library (and potentially testing), not in the packaged build - it's also pretty verbose in your console
@mcnuttandrew (and anyone else who might be playing along at home) - in the midst of putting together a PR here - should have it in by the end of the week.
In their current form, in our product we can't use the builtin
react-vis
legends as they provide no keyboard accessibility, specificallytabindex
andaria-*
attributes.Probably a bit wide for one issue, but it would be nice to be able to make series that have event handlers focusable too.
As always, happy to pitch in with a PR along these lines!