Page MenuHomePhabricator

Gerrit red-green diff make the code hard to read (impossible for color-blind users)
Closed, ResolvedPublic

Assigned To
Authored By
Krinkle
Sep 13 2019, 8:23 PM
Referenced Files
F31473170: a.png
Dec 12 2019, 1:59 AM
F31473171: b.png
Dec 12 2019, 1:59 AM
F31473169: c.png
Dec 12 2019, 1:59 AM
F30341943: d.png
Sep 14 2019, 4:42 PM
F30341942: Screenshot 2019-09-14 at 17.42.21.png
Sep 14 2019, 4:42 PM
F30326742: b.png
Sep 14 2019, 3:03 AM
F30326741: a.png
Sep 14 2019, 3:03 AM
F30330003: c.png
Sep 14 2019, 3:03 AM

Description

Similar to T13374 for MediaWiki (resolved in 2012).

For Gerrit, it's much worse than it was for MediaWiki.

  • The red is quite dark.
  • We have red-on-red text (given syntax highlighting, e.g. red-keywords in red-removed lines of code).
  • We have green-on-red text (e.g. green-comments in green-added lines of code).
  • The green has a yellow hue in it, which is problematic for color-blind users.

From a brief chat with @Paladox, it seems there isn't a plugin interface for additional diff themes or syntax highlight themes in Gerrit. They are hardcoded in the CodeMirror library that Gerrit embeds.

However, the good news is that when Gerrit switches between code mirror themes, it does so by not only swapping out stylesheets, but also by adding an theme-identifiable class name to the document.

This means we could use our site-wide custom stylesheet (puppet: gerrit/GerritSite.css) to augment one of the existing themes to be more optimised for readability.

default theme (unchanged)
default-unchanged.png (2,873×1,597 px, 523 KB)
Proposal
  • Leave default unchanged.
  • Provide 1 alternate diff theme that provides better contrast between foreground and background.
  • Provide 1 alternate diff theme that is ideal for color-blindness, using the familiar MW's diff colors.

Better contrast ("eclipse") - read details at T232893#5492128

eclipse theme (before)eclipse theme (after)
eclipse-before.png (2,869×1,595 px, 520 KB)
eclipse-after.png (2,872×1,594 px, 514 KB)

Colorblind friendly ("elegant") - read details at T232893#5493378

elegent theme (before)elegent theme (after)
Screenshot 2019-09-14 at 17.42.21.png (2,868×1,383 px, 430 KB)
d.png (2,870×1,377 px, 400 KB)

How to change the diff theme?

a.png (682×197 px, 23 KB)
b.png (782×675 px, 404 KB)
c.png (724×217 px, 23 KB)

Event Timeline

Krinkle triaged this task as Medium priority.

Change 536687 had a related patch set uploaded (by Krinkle; owner: Krinkle):
[operations/puppet@production] Gerrit: Add colorblind-friendly diff styles to 'eclipse' syntax theme

https://gerrit.wikimedia.org/r/536687

default theme (unchanged)
default-unchanged.png (2,873×1,597 px, 523 KB)
eclipse theme (before)eclipse theme (after)
eclipse-before.png (2,869×1,595 px, 520 KB)
eclipse-after.png (2,872×1,594 px, 514 KB)

Changes:

  • Steer the greens away from the yellow spectrum, slightly towards blue.
  • Steer the reds away from the magenta spectrum, to mid-way orange-yellow.

I liked the colors Trevor proposed back in 2012:

Diff-Colors.png (350×133 px, 4 KB)

And they are apparently still used. Should we align with MediaWiki colors? And maybe even upstream them to Gerrit!

€100 to anyone starting a yellowblue.diff or noredgreen.diff website initiative ;]

@hashar Hm.. I could not get those to work well before. The main two issues are:

  1. The Gerrit diff is syntax highlighted. This mean most text is not black, but a different color that might need a lighter background colour. It also means we cannot make any of the text bold, as that would be confusing, I think.
  2. The code formatter in Gerrit does not provide a continues HTML element that wraps added or removed characters, which means we cannot use border around the added characters.

My initial attempts did not look like MediaWiki at all:

a.png (1,532×769 px, 152 KB)
b.png (1,476×840 px, 154 KB)
borderbackground
c.png (1,386×480 px, 77 KB)
MediaWiki

But... given you mention it, I gave it another try (with some hacky CSS selectors!). This looks more acceptable, maybe?

c.png (2,860×1,204 px, 350 KB)

elegent theme (before)elegent theme (after)
Screenshot 2019-09-14 at 17.42.21.png (2,868×1,383 px, 430 KB)
d.png (2,870×1,377 px, 400 KB)

Changes:

  • Use white for the background added/removed blocks. Only highlight the changed characters with a background color.
  • Use MediaWiki's diff yellow and blue for added/removed characters.
  • Add a border around each block using MediaWiki's diff colors as well.
  • Give context lines a light grey shade instead of white.
  • Update scrollbar blocks the same way as well.

Hmm, the blue in the elegent theme (after) screenshot looks like user-selected text to me, but if this improves accessibility than I'm all for it.

Another option maybe worth popularizing more is doing gerrit reviews from your IDE (PhpStorm has a plugin) where you are free to choose whatever theme you like.

Hmm, the blue in the elegent theme (after) screenshot looks like user-selected text to me, but if this improves accessibility than I'm all for it.

Yep, I've included the yellow/blue one as matching theme to MediaWiki's diff theme (example) which has the same ambiguity (assuming default OS configuration). It's a compromise, but not a new one.

Personally I'd be using the eclipse theme (T232893#5492128) which leans more towards traditional diff colours whilst still being (imho) more readable than the default we have today.

In any event, let me emphasise:

  • My proposal does not change the Gerrit default theme in any way. I assume the vast majority are using this and thus nobody will notice anything different.
  • The diff settings panel in Gerrit (gear icon in old UI) has a few dozen alternative themes built-in that are already exist today. Due to not being able to easily add new themes, I've picked two of these that are closest to the default (least likely to be strongly preferred) to fork and turn into the alternative options.

Both the "Eclipse" (readable red-green) and "Elegant" (MW yellow-blue) are entirely opt-in only.

Krinkle updated the task description. (Show Details)
Krinkle lowered the priority of this task from Medium to Low.

Handing the stick over for deployment.

Change 536687 merged by Dzahn:
[operations/puppet@production] Gerrit: Make 'eclipse' and 'elegant' themes colorblind-friendly

https://gerrit.wikimedia.org/r/536687

I have revisited this task after T256615 which is about the label voting chips being hard to read. I really like the effort made here to make the diff colors different, it seems to be from Gerrit 2.15 era and the new Gerrit does not seem to have a way to change the diff theme anymore?

So should we just override the default colors to match the yellow/blue here?

Change 672987 had a related patch set uploaded (by Hashar; owner: Hashar):
[operations/puppet@production] gerrit: GerritSite.css remove unused diff customization

https://gerrit.wikimedia.org/r/672987

Change 672987 merged by Dzahn:
[operations/puppet@production] gerrit: GerritSite.css remove unused diff customization

https://gerrit.wikimedia.org/r/672987