Page MenuHomePhabricator

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

Tokens
"Love" token, awarded by Zoranzoki21."Mountain of Wealth" token, awarded by Nemo_bis."Like" token, awarded by MarcoAurelio."Like" token, awarded by D3r1ck01."Love" token, awarded by hashar.
Assigned To
Authored By
Krinkle, Sep 13 2019

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)
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)

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

elegent theme (before)elegent theme (after)

Event Timeline

Krinkle created this task.Sep 13 2019, 8:23 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptSep 13 2019, 8:23 PM
Krinkle claimed this task.Sep 13 2019, 8:52 PM
Krinkle triaged this task as Normal 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

Krinkle added a comment.EditedSep 13 2019, 9:03 PM
default theme (unchanged)
eclipse theme (before)eclipse theme (after)

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.
hashar added a subscriber: hashar.Sep 13 2019, 9:49 PM

I liked the colors Trevor proposed back in 2012:

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:

borderbackground
MediaWiki

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

elegent theme (before)elegent theme (after)

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.

Krinkle added a comment.EditedFri, Sep 20, 3:04 PM

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)Fri, Sep 20, 3:17 PM
Krinkle updated the task description. (Show Details)
Krinkle reassigned this task from Krinkle to thcipriani.Fri, Sep 20, 7:10 PM
Krinkle lowered the priority of this task from Normal to Low.

Handing the stick over for deployment.