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

Validate contrast color functions #24508

Closed
silverwind opened this issue May 4, 2023 · 14 comments · Fixed by #24586
Closed

Validate contrast color functions #24508

silverwind opened this issue May 4, 2023 · 14 comments · Fixed by #24586
Labels
type/enhancement An improvement of existing functionality

Comments

@silverwind
Copy link
Member

silverwind commented May 4, 2023

Backend has at least 2 implementations of a contrast color function, frontend has one.

This post has some potentially useful insights for the calculation. We should ensure the functions work the same in backend and frontend, and ideally also check if there are any existing issues where the function outputs suboptimal colors.

@silverwind silverwind added type/proposal The new feature has not been accepted yet but needs to be discussed first. type/feature Completely new functionality. Can only be merged if feature freeze is not active. type/enhancement An improvement of existing functionality and removed type/proposal The new feature has not been accepted yet but needs to be discussed first. type/feature Completely new functionality. Can only be merged if feature freeze is not active. labels May 4, 2023
@HesterG
Copy link
Contributor

HesterG commented May 5, 2023

From the reference post and github css styles, Looks like github is using Relative Luminance to calculate the text color based on background color, where calculation of RGB is also different by definition (But I also find github just use the RGB values directly or maybe I misunderstood something). We are using color contrast for calculation now. I think the relative luminance way will make the contrast between background and text color of labels look better.

@silverwind
Copy link
Member Author

I think our methods are similar to GitHub with the exception that GitHub also has/had the bug with the grey shade mentioned in the post.

I would like to see the new functions modeled after CSS contrast-color. Here is what the AI came up for such a question: Go, JS.

@HesterG
Copy link
Contributor

HesterG commented May 5, 2023

I think our methods are similar to GitHub with the exception that GitHub also has/had the bug with the grey shade mentioned in the post.

Actually this has been fixed using exatly the suggested solution by the article:

github's current css:

Screen Shot 2023-05-05 at 15 18 11

And the labels look good in the demo repo from the article now

@silverwind
Copy link
Member Author

silverwind commented May 5, 2023

Thanks. I don't see the need for us to do this in CSS (which I think is clumsy and not unit-testable) but we should do the same they do, only in Go and JS. We can use the demo repos colors for our own unit tests.

@HesterG
Copy link
Contributor

HesterG commented May 5, 2023

Thanks. I don't see the need for us to do this in CSS (which I think is clumsy and not unit-testable) but we should do the same they do, only in Go and JS. We can use the demo repos colors for our own unit tests.

Yep agreed.

@silverwind
Copy link
Member Author

silverwind commented May 5, 2023

For reference, demo label colors. Will be useful for our tests:

style="--label-r:215;--label-g:58;--label-b:74;--label-h:353;--label-s:66;--label-l:53;"
style="--label-r:0;--label-g:117;--label-b:202;--label-h:205;--label-s:100;--label-l:39;"
style="--label-r:207;--label-g:211;--label-b:215;--label-h:210;--label-s:9;--label-l:82;"
style="--label-r:162;--label-g:238;--label-b:239;--label-h:180;--label-s:70;--label-l:78;"
style="--label-r:112;--label-g:87;--label-b:255;--label-h:248;--label-s:100;--label-l:67;"
style="--label-r:0;--label-g:134;--label-b:114;--label-h:171;--label-s:100;--label-l:26;"
style="--label-r:228;--label-g:230;--label-b:105;--label-h:60;--label-s:71;--label-l:65;"
style="--label-r:216;--label-g:118;--label-b:227;--label-h:293;--label-s:66;--label-l:67;"
style="--label-r:255;--label-g:255;--label-b:255;--label-h:0;--label-s:0;--label-l:100;"
style="--label-r:43;--label-g:134;--label-b:133;--label-h:179;--label-s:51;--label-l:34;"
style="--label-r:43;--label-g:135;--label-b:134;--label-h:179;--label-s:51;--label-l:34;"
style="--label-r:44;--label-g:135;--label-b:134;--label-h:179;--label-s:50;--label-l:35;"
style="--label-r:59;--label-g:182;--label-b:179;--label-h:178;--label-s:51;--label-l:47;"
style="--label-r:124;--label-g:114;--label-b:104;--label-h:30;--label-s:8;--label-l:44;"
style="--label-r:126;--label-g:113;--label-b:108;--label-h:16;--label-s:7;--label-l:45;"
style="--label-r:129;--label-g:112;--label-b:109;--label-h:9;--label-s:8;--label-l:46;"
style="--label-r:128;--label-g:112;--label-b:112;--label-h:0;--label-s:6;--label-l:47;"

@HesterG
Copy link
Contributor

HesterG commented May 5, 2023

I would like to see the new functions modeled after CSS contrast-color. Here is what the AI came up for such a question: Go, JS.

I am trying to modify the luminance calculation function to use the one same as github. But not sure how is this "selecting the one with the highest contrast from the list" function suppose to help? I see the current contrast calculation for scoped labels, scopeColor and itemColor are calculated based on luminance.

@silverwind
Copy link
Member Author

silverwind commented May 5, 2023

I am trying to modify the luminance calculation function to use the one same as github. But not sure how is this "selecting the one with the highest contrast from the list" function suppose to help?

To be truly universally usable, the function can not just return hardcoded black and white values. There is one case in the go codebase where it returns shades like #555555 and #aaaaaa. I don't know the reason for these odd shades, but to support arbritrary output colors, the user must be able to pass in this color list.

I do think we should make all functions return #111111 or #eeeeee so the function can be simplified to only accept a single color argument.

@silverwind
Copy link
Member Author

I suggest we put the new functions in:

  • modules/util/color.go with tests in modules/util/color_test.go
  • web_src/js/utils/color.js with tests in web_src/js/utils/color.test.js

@HesterG
Copy link
Contributor

HesterG commented May 6, 2023

To be truly universally usable, the function can not just return hardcoded black and white values. There is one case in the go codebase where it returns shades like #555555 and #aaaaaa. I don't know the reason for these odd shades, but to support arbritrary output colors, the user must be able to pass in this color list.

I see, do you know where this function is? I searched 555/555555/aaa/aaaaaa but I didn't find it..

@HesterG
Copy link
Contributor

HesterG commented May 6, 2023

I tried to use the new luminance function with RGB channel values directly and RGB in relative luminance definiton respectively and got the following results, we might need to pick which one to use:

left: use rgb directly (same as github does) right: use rgb in relative luminance definiton

current labels for reference

Screen Shot 2023-05-06 at 11 32 47Screen Shot 2023-05-06 at 11 33 24

Screen Shot 2023-05-06 at 11 32 31Screen Shot 2023-05-06 at 11 57 04

@silverwind
Copy link
Member Author

Maybe I was imagining things. Searching again, I can't find it anymore. The three instances of these functions are:

modules/templates/util_render.go:158:   luminance := (0.299*r + 0.587*g + 0.114*b) / 255
web_src/js/utils.js:149:  const brightness = (0.299 * r + 0.587 * g + 0.114 * b) / 255;
models/issues/label.go:181:     brightness := (0.299*r + 0.587*g + 0.114*b) / 255

@silverwind
Copy link
Member Author

left: use rgb directly (same as github does) right: use rgb in relative luminance definiton

Those on the right look great to me. Left side has many issues with too little contrast, right has none.

@HesterG
Copy link
Contributor

HesterG commented May 8, 2023

left: use rgb directly (same as github does) right: use rgb in relative luminance definiton

Those on the right look great to me. Left side has many issues with too little contrast, right has none.

Personally I also like the right one better. Then I will go with the rgb calculation for the right one

silverwind added a commit that referenced this issue May 10, 2023
…e files (#24586)

Close #24508

Main changes:
As discussed in the issue

1. Change luminance calculation function to use [Relative
Luminance](https://www.w3.org/WAI/GL/wiki/Relative_luminance)
2. Move the luminance related functions into color.go/color.js
3. Add tests for both the files (Not sure if test cases are too many
now)

Before (tests included by `UseLightTextOnBackground` are labels started
with `##`):
https://try.gitea.io/HesterG/testrepo/labels

After:
<img width="1307" alt="Screen Shot 2023-05-08 at 13 37 55"
src="https://user-images.githubusercontent.com/17645053/236742562-fdfc3a4d-2fab-466b-9613-96f2bf96b4bc.png">
<img width="1289" alt="Screen Shot 2023-05-08 at 13 38 06"
src="https://user-images.githubusercontent.com/17645053/236742570-022db68e-cec0-43bb-888a-fc54f5332cc3.png">
<img width="1299" alt="Screen Shot 2023-05-08 at 13 38 20"
src="https://user-images.githubusercontent.com/17645053/236742572-9af1de45-fb7f-460b-828d-ba25fae20f51.png">

---------

Co-authored-by: silverwind <me@silverwind.io>
Co-authored-by: Giteabot <teabot@gitea.io>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants