Better color scheme for diffs
Closed, ResolvedPublic

Description

As someone who reads a ton of diffs (more important) and is color blind (I think less important here, but may be relevant), I find myself checking out the diff locally instead of reading it on the diff page. I wanted to bring this up since it didn't seem like there were any other tasks about this: the root problem here is that diffs are hard to read.

From my perspective, the main thing is that the green highlight is too saturated/dark, so syntax highlighting doesn't stand out as much. When I visit the equivalent commit view on github (which we mirror to), I find the code to be much more readable - many folks on our team use the github mirror to explore code instead of using the phabricator one. I think being able to customize the colors would also suit my needs (I'd probably just copy whatever github is using), but I feel like phabricator users in general might appreciate some default color tweaks.

Totally fine if the answer here is "you're colorblind, josh, it looks fine to most people" - I realize there's a lot going on and making changes based on just my opinions would go beyond just colorblind accessibility. But I think I'd be markedly happier if the colors changed, so here I am. :)

(If there's no intent on making changes here, my second route would probably be to use stylish to customize the colors, and any tips you have re: css classes there would be appreciated.)

joshma created this task.Tue, Jan 31, 12:13 AM

What version of Phabricator are you using?

See also T12060.

+ @chad

chad added a comment.Tue, Jan 31, 12:25 AM

Yeah I don't know what to do with this task. We've made two separate changes to colors in the past 2 weeks at the request of a color blind user. This change would be the third request.

chad added a comment.Tue, Jan 31, 12:29 AM

I would guess that we should:

  • Revert to standard (previously well loved) colors for 2-up diffs.
  • Come up with separate 1-up colors (since I think that was the core of T12060)

Versions:

phabricator a4f8aff2a2e41e5853488689c9d79ee425482fb9 (Sat, Jan 14)
arcanist ade25facfdf22aed1c1e20fed3e58e60c0be3c2b (Thu, Jan 5)
phutil 9d85dfab0f532d50c2343719e92d574a4827341b (Thu, Jan 12)

@chad here's an example:

Keywords (for/if/raise) look very similar to black text, strings don't stand out either. The whole file is new, so the entire page is a bright green.

I actually didn't notice the recent color changes (if there were recent ones) - this is an issue I've had since using phabricator a few years ago, but it's just gotten more on my mind recently. I appreciate the time you're spending on this - I spend hours a day in phabricator, so these sorts of things really affect my workflow.

chad added a comment.Tue, Jan 31, 12:42 AM

We had "full new lines" light green for all of a week before @epriestley stole my kittens until I changed it back.

We recently darkened that color in T12060 to improve the UI for colorblind users, and the change was specifically vetted by a colorblind user (although maybe not a colorblind engineer who uses Phabricator? T12060#205789).

I think we should just revert to the old colors and put a blue/orange mode in SettingsDisplayAccessibility, since no one seems excited about doing the legwork in T12060 to pick other colors.

:( Maybe this is more isolated than I thought - I can probably make do with a stylish extension for now (if the blue/orange mode doesn't work). For what it's worth I find github's colors pretty readable, seems like a useful reference to me.

@chad, I don't mind doing the legwork on this if you're OK with a separate mode.

pink / slightly different pink incoming

😺 😺 😺

Sorry I mean indigo

Better? Both just look grey?

Better - are keywords/classnames red? I think for me reds are duller, so they look very similar to the plain black text - brightening that would help.

For the prose, the non-blue color actually looks green to me (I'm guessing it's red?? I'm rediscovering how colorblind I am), so it looks like there are two additions instead of a delete + add. With my great powers of deduction, I can figure out the blue is new, so I'm not too upset (I don't use that view much, heh), but you might want additional input on that one.

Do you want to give me four colors that work well for you as a starting point, by, e.g., control-clicking an existing diff and fiddling with the CSS?

(Those screenshots are orange for deleted text and blue for added text.)

We can change the syntax highlighting colors but that's significantly more involved.

Specifically:

  • A very light "removed" color (default: very pale red) for lines which have removed text on them.
  • A more visible "removed" color (default: red) for lines which have been completely removed, text on lines which has been removed, and removed text in prose diffs.
  • A very light "added" color (default: very pale green) for lines which have added text on them.
  • A more visible "added" color (default: green) for lines which have been completely added, text on lines which has been added, and added text in prose diffs.

We're also assuming that you're red/green colorblind -- is that right? If you have some other kind of colorblindness, I think that would tend to explain why the colorblind changes made things worse for you, and I probably should not call the accessibility mode "Red/Green Colorblind".

This comment was removed by epriestley.
joshma added a comment.Wed, Feb 8, 5:08 PM

(by the way @epriestley I haven't forgotten; some things have come up but I'll try to get back to you this weekend!)

No problem, we're not in any rush.

(I'm 99.99% sure that guy who I nuked above was just SEO spam: vague not-actually-on-topic-comment with a link to an unrelated paid service.)

So I took another look today. Given that the syntax colors are hard to change, looking only at the 4 bg colors I realized that the main change that would help is if td.new-full just used td.new's background color instead (#eaffea). I think it's because:

  1. In general, the red keywords don't stand out as much, so when their background is more saturated with green it blends in for me
  2. When I filed this task, I was reading a lot of new code. This means I was looking at large blobs of #a6f3a6, making it hard to read the code in front.

So unfortunately my suggestion doesn't fall into one of the four colors you specified, but rather: for lines which have been completely added, use the very light "added" color instead.

If syntax colors can change, my added suggestions would be:

  • Use a more orangey color for keywords, or use a blue instead

What I'm now using (via Stylish - it's not super consistent / a bit ugly but works better):

.differential-diff td.new-full {
    background: #eaffea;
}
.remarkup-code .k,
.remarkup-code .kd,
.remarkup-code .kn,
.remarkup-code .kt {
    color: #795da3;
}
.remarkup-code .nd,
.remarkup-code .ow {
    color: #795da3;
}
chad added a comment.Sun, Feb 12, 2:27 AM

if td.new-full just used td.new's background color instead (#eaffea). I think it's because:

This was the explicit change I made before the current version, which was reported by other users as a bug / reason they prefer Phabricator diffs. I personally like it as well, so if we go that route, we'd have to do something with CelerityPostProcessor. FWIW, if I could have a separate "DiffViewer" celerity engine separate from the Accessibility version, I'd be happy to come up with a handful of diff viewer color schemes.

chad added a comment.Sun, Feb 12, 2:27 AM

Basically, can we support "Large Fonts" and "Colorblind Diffs" at the same time?

:/ colors are hard. From my end, I'm very happy with what I ended up doing in Stylish (and this conversation helped, thank you) so it's not as pressing for me anymore. But LMK if there's anything I can help test out.

T5701 describes a strategy for Pygments styles. I don't think we should tackle that as an accessibility issue, but I think building it as a user preference issue is reasonable.

I think the T12060 stuff was deuteranomaly, not protanomaly, and maybe made the protanomaly case worse because the older colors were less saturated. So D17269 might help this case (reverting to the old, less-saturated colors would improve visibility for protanomaly), but it would potentially leave us with a new "deuteranomaly" mode that just makes colors very very slightly more saturated. Maybe that's fine, though. D17269 also puts us in a better place technically, since all of the email and inline stuff now uses the CSS variables.

As a user with normal (I think?) color vision, I slightly prefer the older less-saturated colors and very, very strongly prefer "bright" colors for fully-added new lines.

Technically, we can turn the accessibility options into several different options, like "Fonts: default / larger", "UI: default / high contrast", "Colors: default / deuteranomaly". The more dimensions of this we add, the more likely we are to to get conflicts eventually, but this probably isn't really a huge deal since these options probably shouldn't overlap much.

I'm also generally more willing to support options/preferences around diff display than elsewhere in the product since it's so core, and I don't think a "use weak colors for new lines" option is necessarily totally unreasonable, but I don't like it much as an approach on trying to improve issues with color vision.

So maybe the pathway forward is:

  • Make D17269 use the old colors for default vision (which would effectively be "non-deuteranomaly" mode, i.e. the old colors are presumably also better for protanomaly) and the new (slightly more saturated) colors for a deuteranomaly mode. Adding this mode to get like a 10% saturation bump feels like sort of a waste. The D17269 changes to make everything (particularly: mail) use the variables should definitely come upstream, but maybe we don't actually add the new color vision mode and just stick with the newer "kind of okay for everyone except users with protanomaly" colors. This feels like a decision we'll be forced to revisit sooner or later to me, though, and I'd maybe rather just split it now so the next time this comes up we just say "sure, this takes 10 seconds, what colors do you want?" and maybe that user just picks a nice sensible shade of blue and a lovely shade of orange instead of asking for a slightly different shade of light green and a rewrite of syntax coloring.
  • Split the accessibility preference into "Font Size", "Color Contrast", and "Color Vision" options (unless we don't add a second color vision mode). But there's no real reason you should have to pick between "Large Text" and "High Contrast".
  • Pursue T5701 primarily to offer greater flexibility to all users, but also to let users with protanomaly select a syntax color mode which doesn't use red on green.

Also, above, I used the term "deuteranomaly" when I meant "deuteranopia". I mistakenly believed they were describing the same type of colorblindness, but they are two different types.