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

refactor: simplify color manipulation code #1372

Merged
merged 60 commits into from
Sep 21, 2021

Conversation

markov00
Copy link
Member

@markov00 markov00 commented Sep 10, 2021

Summary

This PR simplifies and refactor the current color manipulation code. This is required to complete #1320

General changes

  • Use a single data structure for internal color representation/manipulation: RgbaTuple: [r,g,b,a]
  • Only apply conversion from/to string when necessary (avoid calling the internal function with color strings)
  • to avoid expensive parsing of strings into the color object, an LRU cache is used (we already use it, but not for every color)
  • reduced and removed unnecessary functions and code used to parse/format colors
  • removed the use of d3-color
  • extracted the override opacity function and removed duplicated cases
  • fix chroma array input mutation
  • the textContrast,textInvertible and textOpacity props are removed from the API due to favor using always high contrast text color
  • the makeHighContrastColor method now just returns whiteor black text color depending on their contrast with the provided background. The contrast is computed with APCA algorithm that is an improved version of the current WCAG 2 math (part of the new WCAG 3.0 guidelines)

BREAKING CHANGES:

the textContrast textInvertible and textOpacity in the Font type has been removed. This affect the HeatmapConfig type in the cell.label, xAxisLabel, and yAxisLabel properties. This also affect the PartitionConfig properties: fillLabel and linkLabel. The textInvertible and textContrast properties are also removed from the DisplayValueStyle used to display bar values labels. The mentioned changes are replaced with the following behavior: always invert the text color depending on the color contrast with its background using pure black or white colors.

Note for EUI team review

@elastic/eui-design there is a minor change that affects the theme of the chart, in particular, the barSeriesStyle.displayValue property that introduces the following change: https://github.com/elastic/elastic-charts/pull/1372/files#diff-ded06b4515f820c90ab4007ab80a647215fcc9cb4b53998851ce302aee64d5a6L690-L691
Your current theme actually uses a flat color for the changed fill type, so the change doesn't affect your theme.
I'd like to just highlight a change we did to the text color when used above a colored background (aka value labels or labels on partition charts): instead of computing a gradual more dark/light color with the sufficient WCAG AA level, we apply a flat black or white color based on theirs contrast with the background. The color contrast calculation is done using a new algorithm called APCAused in the WCAG 3 draft instead of the previous algorithm that used the perceived luminance. As linked above, this method seems to fix a lot of issues that the luminance method didn't consider.

Checklist

  • The proper chart type label has been added (e.g. :xy, :partition)
  • The proper feature labels have been added (e.g. :interactions, :axis)
  • The :theme label has been added and the @elastic/eui-design team has been pinged when there are Theme API changes
  • New public API exports have been added to packages/charts/src/index.ts
  • Unit tests have been added or updated to match the most common scenarios
  • The proper documentation and/or storybook story has been added or updated
  • The code has been checked for cross-browser compatibility (Chrome, Firefox, Safari, Edge)
  • Visual changes have been tested with all available themes including dark, light, eui-dark & eui-light

@nickofthyme nickofthyme force-pushed the master branch 7 times, most recently from 7148acf to 604b153 Compare September 13, 2021 04:06
@markov00 markov00 changed the title refactor: simplify text contrast code refactor: simplify color manipulation code Sep 13, 2021
@monfera
Copy link
Contributor

monfera commented Sep 17, 2021

Amazing! image

@monfera
Copy link
Contributor

monfera commented Sep 17, 2021

It looks like noticeably worse contrast here, with the elsewhere beneficial more eager use of white text:
image

@monfera
Copy link
Contributor

monfera commented Sep 17, 2021

Besides the above icicle graph, there seems to be cases where the switchover to white happens earlier than I think desired, and at least to me, the black font looks clearly more readable. This despite my wish for more white text when first seeing the former versions based on the old WCAG recommendations, so I was white leaning but it seems to overdo it esp. when the font is small. It's observable on a lot of partition charts
image

@monfera
Copy link
Contributor

monfera commented Sep 17, 2021

This is a great change :chefs_kiss:
image

@monfera
Copy link
Contributor

monfera commented Sep 17, 2021

Nice darkening of axis tick labels, a significant step in the right direction
image

Copy link
Contributor

@monfera monfera left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code wise, and in spirit, looks great 🎉

See some non-blocker comments inline.

The darkening of some (but for reason, not all; see most Cartesians and bullet/gauge/goal) axis tick labels is a welcome change, going from mid gray to slightly darker gray. Not necessarily in this PR, it'd be great to reach black or near-black.

The level of text whitening seems excessive, at least to me resulting in clearly less readable text on lighter or mid luma backgrounds even when compared to the former dark gray, not to mention compared to using the new, real black which many partition charts now do for the dark. See individual comments for image examples

A blocker is that some hover highlights (well, fading the other bits) got lost, for example:
image
image

@markov00
Copy link
Member Author

markov00 commented Sep 20, 2021

@monfera Thanks for catching the missing hover highlights: it was caused by my changes in the clearCanvas:
my initial idea was to clear the canvas with the background color from the theme to fix the wrong color blending value we have identified in one of our calls. The solution was to fix the clearCanvas with:

function clearCanvas(ctx: CanvasRenderingContext2D, bgColor: Color) {
  withContext(ctx, () => {
    ctx.setTransform(1, 0, 0, 1, 0, 0);
    ctx.fillStyle = bgColor;
    ctx.fillRect(0, 0, ctx.canvas.width, ctx.canvas.height);
  });
}

The problem happens when the background color is transparent and the fillRect doesn't actually clear the screen.
I've pushed a change that does both: clear and fill with the background: 70f1774

@markov00
Copy link
Member Author

markov00 commented Sep 20, 2021

darkening of axis tick labels,

@monfera and actually there is no darkening of the axis, but just the fact that the background didn't get cleared correctly caused the axis being rendered twice, one on top of the other, when moving the mouse over the legend (as in the linked screenshot)
We can definitely improve the default theme with better colors/styles, but this is out of the scope of the current PR

@monfera
Copy link
Contributor

monfera commented Sep 20, 2021

@markov00 I had a vague recall that I did thefillRect instead of the default expectation clearRect for some reason, when we started phasing out Konva. An issue like this may or may not have been the reason. It'd be worth adding a comment line above the fillRect to explain why it's used instead of clearRect (eg. mentioning about which test case will fail). Though the test case will fail anyway 😄

Copy link
Contributor

@monfera monfera left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving it with the note that I'm not the authority on the contrast criteria, so I can't speak for or against the extent the new algo leans toward white text over mid colored areas. Thanks for linking the external PR where you enquired about some of the apparent contrast regressions we saw

@cchaos
Copy link
Contributor

cchaos commented Sep 20, 2021

Thanks for the note @markov00. I love that you all continue to think through accessibility improvements. I will throw my quick concern out there for jumping into the APCA calculation (without much research of my own). Mainly, that I have 2 concerns.

  1. Consistency with our calculations we use throughout Elastic products including any automated a11y testing like through Axe.
  2. Actually produces less contrast (based on previous criteria) which seems counter to other conversations being had around better accessibility (i.e. feat(all): global, multi-channel prefers-contrast option #1378).

Screen Shot 2021-09-20 at 12 41 30 PM

A ratio of 2.97 is notably lower than our typical 4.5 minimum.

Copy link
Collaborator

@nickofthyme nickofthyme left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think in some cases the white what easier to read IMHO but seems like it's half way between how it was and how it is with these changes. Anyway, nothing to change if we still to the standard that is fine with me.

Is this intentional? Seems like an error to me.

image

@markov00
Copy link
Member Author

Thanks @cchaos for your comments, I'd like to add few more comments to clarify the choices:

  1. Consistency with our calculations we use throughout Elastic products including any automated a11y testing like through Axe.

You are right, we should prefer a consistent methodology across Elastic. I reverted the changes to use the WCAG2/relative luminance algorithm, for now, but I will keep the APCA method around for testing.

If I'm not wrong Axe works testing accessibility of DOM elements, but it can't read/interpret the content of an image, providing valuable hints if a text has more/less contrast than its background. Charts are rendered in canvas and Axe can't give us hints on the accessibility of the image itself (we at the moment have a set of hidden texts surrounding the chart to improve the accessibility). We can check the a11y and readability of the chart (the canvas part) only manually at the moment, unfortunately.

  1. Actually produces less contrast (based on previous criteria) which seems counter to other conversations being had around better accessibility (i.e. feat(all): global, multi-channel prefers-contrast option #1378).

The method used in your example to calculate the contrast is the one available in the WCAG 2 specification (based on relative luminance, with a set of AA, AAA levels specific to the calculated contrast ratio). The APCA method instead shows that white on orange has higher contrast than black on orange (see this interesting article about the orange button problem in a11y https://www.bounteous.com/insights/2019/03/22/orange-you-accessible-mini-case-study-color-ratio/). There are definitely differences in the calculated contrast but, as described here, these differences between old and new ways are the output of multiple research for improving text readability and contast.

The WCAG 3.0 is still a draft, but it's announced that the APCA method will replace the existing one:
https://www.w3.org/TR/wcag-3.0/#issue-container-generatedID-7

Chrome also has added the APCA contrast in the dev tools (now only behind an experimental flag and with some bugs 😬 ) https://developer.chrome.com/blog/new-in-devtools-89/#apca

Anyway, I've already switched back to the previous WCAG 2.0 calculation for consistency. APCA is still in development/research so there can be improvement in the near feature that can push us to adopt it earlier, in the meantime, we can live with the relative luminance method.

@markov00
Copy link
Member Author

I think in some cases the white what easier to read IMHO but seems like it's half way between how it was and how it is with these changes. Anyway, nothing to change if we still to the standard that is fine with me.

Is this intentional? Seems like an error to me.

image

You have probably picked up an outdated commit, this was solved in 70f1774

@markov00 markov00 merged commit b6645d3 into elastic:master Sep 21, 2021
@markov00 markov00 deleted the 2021_09_10-simplify_text_contrast branch September 21, 2021 16:33
@cchaos
Copy link
Contributor

cchaos commented Sep 22, 2021

Thanks for the explanation @markov00 . I definitely agree that the current luminance method is not ideal in certain situations, but I just couldn't believe how different the results between that and the new method were. It worries me a bit and since it's still in draft, I'd worry about jumping to use it right away. I appreciate that it's back to the original calculation at least for now. It's definitely something we'll want to continue to monitor and most likely an effort we'll want to take across multiple packages, not just charts.

As an FYI, I've also been monitoring this issue for a while, just now realizing that it was started/continues to be updated by the creator of that APCA method. w3c/wcag#695. But the fact that it remains open makes me think there's not been a final decision/method yet.

@nickofthyme
Copy link
Collaborator

🎉 This PR is included in version 37.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@nickofthyme nickofthyme added the released Issue released publicly label Oct 5, 2021
@Myndex
Copy link

Myndex commented Oct 14, 2021

Hello everyone, @monfera @cchaos @markov00 this post is replying specifically to Marco, Caroline, and Robert:

First, just FYI, there is an APCA repo here

And I've opened the discussion tab to field any questions or concerns.

Some general responses to comments in this thread:

  1. WCAG 3 APCA is a paradigm shift in how visual contrast is predicted and how those predictions apply to design guidelines.
    • WCAG 3 does not compare directly to WCAG2 in terms of results or how it's used.
    • Font size and font weight are part of the key parameters to consider.
  2. WCAG 2 contrast is known to be wrong in a number of areas, which is why I've been developing APCA.
    • WCAG 2 is not perceptually accurate, it does not follow human perception.
    • It's only a reasonably useful contrast value when both of the following are true:
      • The text is darker than the background AND
      • The background is lighter than #A0A0A0
  3. Per the examples in this thread, the concerns raised don't appear to be due to APCA
  4. The total APCA Lc contrast scale from black to white is a little more than 100.
    • It is perceptually uniform, so cutting the Lc in half results in an apparent halving of contrast.
    • The distance between the middle and white or black is around Lc 51 to Lc 54
    • The minimum recommended for fluent content text is Lc60 to 75
    • The minimum recommended for large text is Lc45 to 60

Specifics

Luminance vs Lightness for Readability Contrast

contrast calculation ... new algorithm called APCA used in the WCAG 3 draft instead of the previous algorithm that used the perceived luminance. ,,, this method seems to fix a lot of issues that the luminance method didn't consider.

Hi Marco, yes the WCAG 2 method does not follow human perception of suprathreshold contrast, which is critical for readability. APCA does use luminance, but it goes much farther, weighting the luminances per a perception model to achieve uniformity that follows our perception of text on a screen.

Chrome also has added the APCA contrast in the dev tools (now only behind an experimental flag and with some bugs 😬)

I was surprised by the interest and the large number of early adopters. I seriously was not expecting the turhout if you will, so I'm a tad embarrassed that some of the early code was a little less stable. The current version has been going since February, and seems stable, further adjustments are expected to be more like tweaks.

You are right, we should prefer a consistent methodology across Elastic. I reverted the changes ... I will keep the APCA method around for testing.

There are some developers that are doing both WCAG 2 and 3 at one with success (you lose the APCA flexibility though).

Feel free to ask questions at the APCA GitHub repo in the discussions area.... I'm going to respond to some of your other comments with Caroline's below...

Mid Contrast, and the W/B Switch Point — Responding to @monfera

Hello Robert, it appears to me that the fonts may be too small for the given contrast?

One of the changes in APCA is the recognition of spatial frequency as a primary force driving contrast perception. For our purposes, spatial frequency means font size and weight.

Another change is the definition of the "middle" of the contrast range on a display ... where to flip from black to white text? The short answer is that if we are going to "auto flip" between black and white, we want to do so when relative luminance (Y) is between 0.32Y and 0.40Y, with 0.35Y - 0.36Y being about ideal for most colors.

This translates to a BG color of equivalent luminance to #999(0.32Y) to #aaa (0.4Y) with #a0a0a0 being a "nearly ideal" 0.351. However, that does not mean that any size font can be used. The APCA contrast for black or white text against #a0a0a0 is not much more than Lc50, and the minimum recommended font is about 32px to 36px for normal weight.

In the example below, just the top two lines qualify, bottom row is a fail for fluent readability:

DataExample0100mixed

This is using a color from your icicle chart, and now let's look there:

Screen Shot 2021-10-13 at 1 46 28 PM

The font is fairly thin, and perhaps font-smoothing is set to the faux antialias? While the black font is indeed near black, the white font is no where near white, it's only at 50% the luminance of white, so perceptually is about 65% lightness when we'd want it to be ~95% lightness.

This has nothing to do with the APCA algorithm, it's a function of the font weight and size, and the way the rasterizer is antialiasing, reducing contrast. One of the stipulations in WCAG 3 is to not use faux antialiasing, and leave "webkit font smoothing" at auto, which allows the device to do subpixel anti-aliasing. Counterintuitively, the anti-alias setting in Webkit-font-smoothing is actually a blending that reduces resolution and contrast.

This same issue is true here:

Screen Shot 2021-10-13 at 1 45 52 PM

Again, with that BG color, only the top two rows in this test sample are recommended. Second from the bottom is "poor but passing" and the bottom row is a fail for fluent readability.

DataExample0200mixed


In response to @cchaos

Hello Caroline, regarding your concerns with "consistency" & "less contrast", in reverse order:

2) To your comment "Actually produces less contrast" — this is not true, though I see how you might come to that conclusion.

APCA does not produce contrast. APCA does nothing to the colors, it only predicts how a human will perceive them, and their effect on readability. And yes, if you try to compare the math it might appear that APCA allows lower contrast, but the reality is perhaps a bit hidden. For instance, your example:

CarolinesExample

Let me ask you if that seems like less than 3:1 contrast? Perhaps it's not fair without a comparison. Take a look at this one below to compare, which one has better contrast? The one above or this one below?

Screen Shot 2021-10-13 at 9 20 39 PM

After thinking about this for a moment, click the spoiler for the answer:

**Spoiler: Which is better contrast?**

Okay, so the first example that you provided was the Orange Example I am running on my sites. This problem brought WCAG 2.x to my attention and is part of why the design community loathes WCAG 2 contrast.

In the first example, APCA correctly reports the contrast as readable at Lc61 for the appropriate font sizes. WCAG reports a fail at 2.97:1.

Okay but then on the second one, APCA reports a total fail and prohibits any text at this level.

WCAG reports the second one as a PASS at 4.5:1 (!!)

Should this second one be a pass? At 4.5:1? The colors are #797979 and #110402. APCA fails it for all text, but WCAG 2 says it's 4.5:1 and fine even for body text. Here's a body text sample with those colors:

Screen Shot 2021-10-13 at 9 28 52 PM

WCAG 2 says this is fine and passes AA. Do you agree?

FACT: When the brightest color is darker than #a0a0a0, WCAG 2 degrades until it is no longer providing useful results.

And here is a central problem: the numbers generated by WCAG 2's contrast math is not relative to human perception of supra-threshold contrast, which is the contrast needed for best readability. APCA very specifically is perceptually accurate, and predicts constant contrast across the entire range. WCAG values are artificially inflated over part of the range to try and make up for these shortcomings, but APCA does not need any such "brute force".

Since APCA is perceptually uniform, one correct Lc value works across the entire available range.

At the SAPC development site, we have a "Research Mode" you can click that reveals some interactive experiments that demonstrate some of the concepts.

And here are links to some brief articles (shorter than this post LOL) with loads of examples that more fully demonstrate this:

The Lighter Side of Dark Backgrounds

Orange You Wondering About Contrast?

Answer to Caroline's first question:

1) Consistency: it is not possible to create something that works correctly and also be consistent with something that is incorrect.

And that is the problem we face with APCA. There is no way to compare the two. WCAG 2's problems are well understood. APCA is superior in accuracy and useful predictions. Fortunately, there is enough overlap that a "transition" can be had, some of the tools developed by other developers have a "pass all" setting, to pass both WCAG 2 and WCAG 3 at once, though the trade off is a loss of the flexibility that APCA normally adds.

Also, APCA has an "escape level" called Score 1, it is the weakest passing score, and considered "poor" but placed there so as to allow sites to pass that are currently passing WCAG 2, while providing guidance to improve to the newer standard.

Caroline:

"As an FYI, I've also been monitoring this issue for a while, just now realizing that it was started/continues to be updated by the creator of that APCA method. w3c/wcag#695. But the fact that it remains open makes me think there's not been a final decision/method yet.

Yes, I started thread 695. In that thread I explain how and why I ended up taking this on as a full time research project. I originally thought I'd be done in three months. LOL. I mean ROTFLMAO. Going into year three, I can say that we will have a new readable world!

The internet killed the print industry. Magazines are all but gone. I miss reading real paper, and I find it very difficult to read on a screen. In fact, I believe the reason that so few people actually read today is due to this issue. Consider the chilling effects of a populace that declines reading, as a civil rights author, I can find no better motivation that making the world a more readable place.

695 is OPEN in part because it is tagged as normative, and in part because this is a major paradigm shift, and comments and discussion will continue, as it has here in THIS thread. I am trying to be open to the degree possible, which is why I opened the discussion tab at the APCA repo.

A last question, perhaps for Marco?

If I can ask, what are you using to generate colors? Are you using CIELUV or a variant? Or CAM02? Or?? Regardless of the method, are you having any difficult problems with color or contrast generation?

TL;DR (LOL)

APCA is demonstrably superior to the old method, but is also more involved than the distance between two colors, as spatial frequency is a key part of the prediction.

I hope I addressed everyone's thoughts and concerns. Feel free to start a chat at the APCA repo discussion area as well.

Thank you,

Andy

Andrew Somers
Invited Expert, AGWG, Silver, LVTF
Myndex Color Science Researcher
Inventor SAPC/APCA

https://github.com/Myndex/SAPC-APCA

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:all Applies to all chart types code quality :colors colors related issue released Issue released publicly skip-newsletter :theme
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants