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

Heatmap colors don't have enough contrast #17273

Closed
brunouber opened this issue May 9, 2022 · 8 comments · Fixed by #17298
Closed

Heatmap colors don't have enough contrast #17273

brunouber opened this issue May 9, 2022 · 8 comments · Fixed by #17298

Comments

@brunouber
Copy link

Click to see a demo.

We've an heatmap like the one linked above.
We use #CAE3DF, #B5BFCD, #9F9EBC, #8B7FAC, #76609B, #61478C, and #4F2F7D as background color, and do not set any data label color, so that as stated in the documentation, highcharts can use the color with maximum contrast.

We saw that highcharts uses Black text only for the first two colors #CAE3DF, and #B5BFCD - but it would have been better to use Black also on #9F9EBC and #8B7FAC.

heatmap

We used this tool to grade the color contrast.

@pawellysy
Copy link
Member

Hello @brunouber, thanks for creating this issue.
The contrast color is chosen based on our internal function, which implementation you can find here:

/**
* Returns white for dark colors and black for bright colors.
*
* @function Highcharts.SVGRenderer#getContrast
*
* @param {Highcharts.ColorString} rgba
* The color to get the contrast for.
*
* @return {Highcharts.ColorString}
* The contrast color, either `#000000` or `#FFFFFF`.
*/
public getContrast(rgba: ColorString): ColorString {
rgba = Color.parse(rgba).rgba as any;
// The threshold may be discussed. Here's a proposal for adding
// different weight to the color channels (#6216)
(rgba[0] as any) *= 1; // red
(rgba[1] as any) *= 1.2; // green
(rgba[2] as any) *= 0.5; // blue
return (rgba[0] as any) + (rgba[1] as any) + (rgba[2] as any) >
1.8 * 255 ?
'#000000' :
'#FFFFFF';
}

As you can see, the coefficients were set to fit most of the cases, but can easily be adjusted for each case. Here is an example of how you can do it: https://jsfiddle.net/BlackLabel/guLd2nmy/4/

Also one of the prior discussions about getContrast function: #6216

I think that currently, this function does a pretty good job of choosing the correct contrast, so I will set the label type: undecided.
What do you think @TorsteinHonsi?

@KacperMadej KacperMadej added this to To do in Development-Flow via automation May 10, 2022
@TorsteinHonsi
Copy link
Collaborator

We can probably tune the contrast algorithm more. In the other issue, I mentioned dynamic weighting, but it doesn't seem that we went through with it.

@pawellysy It would be interesting to dynamically generate a color table with coverage of both hue, brightness and saturation, and see how our contrast function handles it. It may be that there are edge cases where we could improve.

@pawellysy
Copy link
Member

pawellysy commented May 11, 2022

Here is the demo of the color table: https://jsfiddle.net/BlackLabel/g7tq01np/10/

I guess it mostly depends on personal preferences. IMO we could lower the threshold a bit so that there are more black labels.

Maybe we should train someone(something) to adjust the coefficient for us? 😅

@TorsteinHonsi
Copy link
Collaborator

TorsteinHonsi commented May 15, 2022

@pawellysy Awesome! That's a great help. If we remove the textOutline, we get a better impression on how the text contrasts against its background: https://jsfiddle.net/highcharts/vcL9z1gd/ . Because the textOutline is black for white text and white for black text.

I agree some more of the labels should be black.

One way we could go about this is to use w3c's definition of perceived color luminence to find the settings for optimal contrast. The formulas can be found at https://dev.to/alvaromontoro/building-your-own-color-contrast-checker-4j7o . I think if we adjust getContrast to compute the luminance for the background color according to the given formula, then we can set a threshold for the luminance. So if the luminance is lower than the threshold, return white, otherwise return black.

@TorsteinHonsi
Copy link
Collaborator

TorsteinHonsi commented May 16, 2022

Here's an implementation of the getContrast function, using the W3C's relative luminance. It provides better contrast in my opinion: https://jsfiddle.net/highcharts/vcL9z1gd/1/

    public getContrast(color: ColorString): ColorString {
        const rgba = Color.parse(color).rgba
            .map((b8): number => {
                const c = b8 / 255;
                return c <= 0.03928 ?
                    c / 12.92 :
                    Math.pow((c + 0.055) / 1.055, 2.4);
            });

        // Relative luminance according to the W3C's formula:
        // https://www.w3.org/WAI/GL/wiki/Relative_luminance
        const l = 0.2126 * rgba[0] + 0.7152 * rgba[1] + 0.0722 * rgba[2];

        return l > 0.45 ? '#000000' : '#FFFFFF';
    }

@oysteinmoseng In its current state, I used 0.45 as the threshold.

return l > 0.45 ? '#000000' : '#FFFFFF'

But as seen in the fiddle, arguably a lower threshold would provide better readability (in practice, more black text). Do you know how low we can set this without violating the required 4.5:1 contrast?

@oysteinmoseng
Copy link
Member

oysteinmoseng commented May 16, 2022

@TorsteinHonsi Because WCAG uses the same relative luminance for its contrast computation we could simply calculate that (see https://www.w3.org/TR/UNDERSTANDING-WCAG20/visual-audio-contrast-contrast.html#contrast-ratiodef)

Untested, but probably something like this:

const useWhite = (whiteLuminance + 0.05) / (colorLuminance + 0.05) >
    (colorLuminance + 0.05) / (blackLuminance + 0.05);

Instead of using a fixed threshold we would then always pick the most contrasting of white or black.

Edit: And of course we don't need the actual contrast numbers, so we can forgo the 0.05 constants and just look at the ratio with precalculated whiteLuminance/blackLuminance constants.

const useWhite = whiteLuminance / colorLuminance > colorLuminance / blackLuminance;

Edit 2: Seems the relative luminance of black might be 0, so in that case we might want the 0.05 after all (to avoid div by 0).

@TorsteinHonsi
Copy link
Collaborator

TorsteinHonsi commented May 16, 2022

@oysteinmoseng If my eyes and monitor are correctly calibrated, this looks very good: https://jsfiddle.net/highcharts/vcL9z1gd/2/

PS:

Seems the relative luminance of black might be 0

Yes. Used the 0.05 in the fiddle above.

@oysteinmoseng
Copy link
Member

Looks great here too!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging a pull request may close this issue.

4 participants