1180600 - Make side-by-side diffs more screen-reader friendly

Status: RESOLVED INVALID

(MozReview Graveyard :: Review Board: Upstream, enhancement)

Description

[Marco Zehe (:MarcoZ)]

The side-by-side diff isn't bad at all per se, but it could use some improvements to be more friendly to screen readers. Here are some of the proposed changes:

1. Make the file name, which is now a th in the first row of the table, a caption element instead. Each diffed file in a commit or set of commits gets its own table, so it makes semantically more sense to make this the caption of the table rather than an arbitrary header cell that gets repeated by screen readers over and over. It is also semantically more correct.

2. Add text to the line number header cells that is only visible to screen readers (e. g. collapsed via CSS rules) that annotates what the change is. Is it a changed line, a newly added, or a removed one? Right now, these things are, as far as i can tell, only communicated via color. Making this information part of the header cells that contain the line numbers will make this information immediately accessible to screen reader users.

3. Make the diffs more keyboard friendly by making them keyboard actionable, so one doesn't have to use the mouse to add a comment. Github has something similar, which works really well. This would entail binding the click (or double click?) handler to a keypress on either the line or line number cell, so it becomes actionable. Probably not tabable, because that would make it cumbersome for keyboard users, but enabling screen readers to execute on the line number by making it focusable via tabindex="-1" would help.

Final goal is to upstream this to ReviewBoard so others can benefit from it outside of Mozilla, too.

Comment 1

[Gregory Szorc [:gps]]

Great feedback!

This is almost certainly something that will get fixed upstream and will trickle down to us. The reason is we don't customize the diff viewer too much, as it is somewhat complicated and our changes tend to be things that upstream likes.

smacleod often reports Mozilla-filed issues upstream. But if you want to submit these suggestions, feel free to do it at https://code.google.com/p/reviewboard/issues/list.

Comment 2

[Marco Zehe (:MarcoZ)]

Hi gps! Done: https://code.google.com/p/reviewboard/issues/detail?id=3912

Comment 3

[Marco Zehe (:MarcoZ)]

Hi mconley, just thought of adding you to our version of the ReviewBoard side-by-side diff accessibility bug after thinking about our conversation from last Friday again. So that you're in on the full picture. :)

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Thanks Marco. I've got buy-in from the other Review Board devs about making screenreader accessibility a student project. I'm meeting with a batch of students to pitch projects at them the weekend after next - I'll let you know if I get any bites!

Comment 5

[:Gijs (he/him)]

Is there some other way to get progress here, now that no students "bit", so to speak?

Comment 6

[Mike Conley (:mconley) (:⚙️)]

One way forward is to make it part of the MOSS grant request that Review Board is submitting. We're putting a new draft together for the second round - I'll try my best to steer it towards hiring a contractor to work on accessibility work.

Alternatively, the team working on Review Board could be contracted to do the work.

Alternatively, we could do the work and submit patches upstream.

These, in my mind, are the ways to progress here.

Comment 7

[Marco Zehe (:MarcoZ)]

For reference, here's the GreaseMonkey script that Jamie from the NV Access team wrote to make the side by side diffs for ReviewBoard more accessible. It takes away some table headers that aren't actually headers and tweaks some other things to indicate inserts, replaces, deletions and such. This script makes side by side diffs and reviews much easier for me.

Link: https://github.com/nvaccess/axSGrease/blob/master/ReviewBoardA11yFixes.user.js

Comment 8

[Mike Conley (:mconley) (:⚙️)]

This seems like it'd be useful to get this into Review Board core...

Any chance this could get triaged?

Comment 9

[Mark Côté [:mcote]]

davidwalsh, want to take a look?  Since it's something for upstream it'll probably take a bit of back and forth, so something you could do on the side.

Comment 10

[David Walsh :davidwalsh]

I'd love to try to get this upstreamed.  Will have a look later today at what it will take to implement the strategies from the Greasemonkey script.

Comment 11

[Marco Zehe (:MarcoZ)]

(In reply to David Walsh :davidwalsh from comment #10)
> I'd love to try to get this upstreamed.  Will have a look later today at
> what it will take to implement the strategies from the Greasemonkey script.

Did you ever get around to checking this out?

Comment 12

[David Walsh :davidwalsh]

Hey Marco!  We switched focus to a different project but our planning meeting is tomorrow and I'll bring it up again!  Thank you for keeping us honest!

Comment 13

[David Walsh :davidwalsh]

Hey Marco!  We're working on an upgrade now, which is hopefully almost done, then I'll be adding your A11y fixes to our MozReview.  If you approve of those changes, I'll make an effort to move upstream!

Comment 14

[BMO Automation]

MozReview is now obsolete. Please use Phabricator instead. Closing this bug.