Skip to content
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

```diff highlighting colors are really low contrast #16029

Closed
amstan opened this issue Dec 24, 2020 · 7 comments
Closed

```diff highlighting colors are really low contrast #16029

amstan opened this issue Dec 24, 2020 · 7 comments
Labels
A-Themes-Official Official themes (light, dark) A-Theming P2 S-Minor Impairs non-critical functionality or suitable workarounds exist T-Defect

Comments

@amstan
Copy link
Contributor

amstan commented Dec 24, 2020

Description

Using this syntax in an element message:

```diff
there already
+addition
-substraction
something that was there already
```

results in:

there already
+addition
-substraction
something that was there already

The problem here is that the + and - green and red colors in element are really low constrast compared to the background.
I think it looks particularly hard to see in the dark theme (note, you might want to see this image surrounded by dark backgrounds):
dark
Note how it's not even green and red, but turqoise and purple.

@TeknikalDomain thinks it looks even more unreadable on the light theme (I say it's bearable):
light

I narrowed down the offending css to https://github.com/highlightjs/highlight.js/search?q=background%3A+%23dfd%3B
But I didn't want to get into making pull requests there, no idea which theme we're actually using for element.

I would do something like:

.hljs-addition {
-  background: #fdd;
+  background: #f8f;
}

.hljs-deletion {
-  background: #dfd;
+  background: #8ff;
 }

This is easy to test with the Inspect tool in Chrome.

Version information

  • Platform: web

For the web app:

  • Browser: Chrome, probably irrelevant
  • OS: Arch Linux
  • URL: app.element.io
@amstan
Copy link
Contributor Author

amstan commented Dec 24, 2020

  • insert good first issue label here!

@TeknikalDomain
Copy link

TeknikalDomain commented Dec 24, 2020

I'm sorry, but if my color math is correct, that's close to magenta and bright cyan, neither of which I think make that much sense. Are those supposed to be the light mode colors or the dark mode colors?

Feel free to correct my math, but... I don't see that working "better" per se

@amstan
Copy link
Contributor Author

amstan commented Dec 24, 2020

https://github.com/highlightjs/highlight.js/search?q=background%3A+%23dfd%3B
I would do something like:

Now that i look at it again, element doesn't seem to use any of those themes I linked.

I'm sorry, but if my color math is correct

You are very correct. For some reason the dark theme does something weird where it inverts the color and shows that. Chrome Inspect tool confirms it.

Note how it's not even green and red, but turqoise and purple.

It's like someone swapped the addition and removal colors to 'fix' the invert, turqoise and purple were close enough given the low cotrast.

@amstan amstan changed the title ```diff highlighting colors are really low constrast ```diff highlighting colors are really low contrast Dec 27, 2020
@jryans
Copy link
Collaborator

jryans commented Jan 22, 2021

Seems like a visual bug, I'd suggest a PR if you see a clear way to fix it.

@jryans jryans added defect P2 S-Minor Impairs non-critical functionality or suitable workarounds exist A-Themes-Official Official themes (light, dark) A-Theming labels Jan 22, 2021
@jryans jryans removed defect labels Mar 4, 2021
@robintown
Copy link
Member

Fixed by matrix-org/matrix-react-sdk#6320

@justin-sleep
Copy link

The dark theme might need darker diff colors than the light theme's. They are currently very low contrast:

2021-07-20-19:32:46

@robintown
Copy link
Member

@justin-sleep Already a PR for that, too: matrix-org/matrix-react-sdk#6355

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Themes-Official Official themes (light, dark) A-Theming P2 S-Minor Impairs non-critical functionality or suitable workarounds exist T-Defect
Projects
None yet
Development

No branches or pull requests

5 participants