-
Notifications
You must be signed in to change notification settings - Fork 38
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
Make Resources Accessed chart keyboard accessible (#1190) #1472
Make Resources Accessed chart keyboard accessible (#1190) #1472
Conversation
@@ -413,7 +427,7 @@ function createResourceAccessChart ({ data, weekRange, gradeSelection, resourceT | |||
.text('Percentage of All Students in the Selected Grade Range') | |||
.style('font-size', '14px') | |||
|
|||
mainGroup.append('g') | |||
mainGroup.insert('g', ':first-child') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This moves the link labels before the bars in the DOM, which makes the Tab order go from left to right (expected for our users?).
@@ -192,7 +192,7 @@ function createResourceAccessChart ({ data, weekRange, gradeSelection, resourceT | |||
|
|||
bar.enter() | |||
.append('rect') | |||
.attr('tabindex', d => d.index * 10 + 1) | |||
.attr('tabindex', 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This changes see the tabindex
strategy to use 0
, per general guidance from MDN on using tabindex
(see Warning on https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/tabindex). The downside is that it doesn't go from link label to bar. I have asked for accessibility feedback on this, but I haven't heard back yet. See also change to old line 463.
.on('focus', (e, d) => { | ||
moveBrushOnFocus(e, d.resource_name) | ||
toolTip.show(e, d) | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because the handlers added use focus
, they also trigger on mouse click as well as keyboard tabbing. There's not a simple way (I've found) to determine whether it was triggered by mouse or tabbing. The result is that if users click on bars, the brush and chart may jump a little, but I've tried to make the new location good for visibility of the rest of the chart. This will need some testing.
The Codacy issues are related to using variables ( |
8f0e12e
to
6c316af
Compare
I tested this and it seems like it works better for keyboard only. It allows the list to be navigated with keyboard which it wasn't already. I feel like adding the extra data visible in the graph about whether it was visited or not visited as well as the percentage of student to an aria-label could make this more useful, but perhaps that's a different issue. I was just testing this with voiceover and it just reads the file name when tabbing. It also gives a full overview when first visiting the page but nothing when you start tabbing. Another thing I noticed is that if you use the mouse and click on anything in the svg, then start tabbing (on the current version) it actually does tab between the name and the "Graphics Symbol". However in this code if you click on it it first goes through all of the names first then goes through the bar graphs. However I don't know how to get that svg into the tab order so it works? I feel considering keyboard only that this is working well for me. Though if we could just get it to "click into" the svg after the checkboxes it seems like it would also be functional? |
@jonespm, I didn't understand what you were saying at first, but yes, this is what I was talking about in the meeting earlier. The current version does allow you to tab through label to bar, but it's using positive |
3b5e182
to
29a4907
Compare
@zqian, here's a demo of the behavior. Data shown is fabricated data from a testing site. Screen.Recording.2023-02-16.at.11.41.07.AM.mov |
This PR aims to resolve #1190.
To Do