Update contrast function and tests, fixes #2743 #2754
Conversation
|
Some tests still use the deprecated threshold param. Is this intentional? |
|
Of course - we still need to check still that old source files don't break; Deprecation doesn't necessarily imply a BC break. |
Update contrast function and tests, fixes #2743
|
Seems like removing the threshold broke the implementation on my work's website 👎 Now where we had white buttons before we have black text because its choosing black instead of white. |
|
It will be doing that because black has higher contrast in that situation. If you want it to use a fixed colour, use a fixed colour, don't use contrast. |
|
Our use case has multiple themes that required us to use that kind of thing initially because most of the colours were set with variables. But I'm just doing a workaround having to duplicate styles for each theme now - our build plan automatically runs an npm update so we can't deploy til we fix these so sadly have to do it not the best way. |
|
Please, can we have "threshold" param back? |
|
|
|
@mhawk0 Your example points to the earlier change (when This PR made the different thing: removing the threshold parameter at all (which was quite lame thing from the very beginning to be honest). See #2743 for more details. So in general the recommendation is to update your Less version and forget of the threshold parameter. If you still find the legacy version to be useful you can always copy the old code into a plugin of your custom functions. |
|
Thank you for the answer. I'm pretty sure that codepen uses less older than 1.7 (I just don't know how to prove it), since |
|
Per #2906 (comment), I think the "Fixing" the contrast function may be what this does, but I misunderstood that to mean that this was a bug fix. When it's actually a major implementation change that produces a more "correct" result. Someone mentioned to me recently that if you have a bug long enough, then developers will build things that depend on the bug. One suggestion might be to revert this change, and then get a proper 3.0 branch going with this and other pending breaking changes (with a document that tracks those breaking changes). I believe the color mathematicians that say the reason for the change is sound, but it probably should not have been shipped in Less 2.7. |
|
The threshold param was only ever included for compatibility with sass, and what it did was entirely arbitrary - I've never seen any justification for what it did, and sass dropped it in exactly the same way with identical impact. Because what it did alter would only affect colours in the middle of the range (i.e. near the threshold), it was only ever going to be edge cases that ran into it, as we're seeing here. One of the deleted comments in here said that there wasn't such a thing as a "more correct" contrast function when there definitively is - the WCAG formula that |
Of course not. I never suggested that. I'm talking about nothing more than versioning and documentation. |
|
OK. Just to refer back to @mhawk0's examples: the contrast ratio for white on |
Black may satisfy a particular color contrast algorithm, but I would agree with @mhawk0's assessment that the white text I perceive as being higher contrast. But I don't know a better contrast algorithm. Do we have a testing page that shows contrast output between the old function and the new (or other algorithms)? It should be visually apparent when one is more correct, since which one "contrasts better" is a matter of human perception.
That's correct. There is not. There are dozens of "color space" models, if not more, and the mathematical value of a color in that space differs for each model, which means that the calculated "contrast" between two colors in that color space also differs. For example: https://www.npmjs.com/package/color-space#spaces |
|
Not really, because luma is simply luma, and it's not dependent on any particular colour space. We happen to be obtaining its value from an RGB source because it's convenient, but we could start from any other colour space and we would still end up with the same value, within the margin of rounding error. There is a color metrics package linked from that color space one, and it will accept colours from any space and still return a single luma value. Personally I think that the white looks better too, but the example given produces a WCAG fail for white and a pass for black, which I'm sure matters to some. I don't think we are really best placed to come up with a new formula. I have no objection to this change being moved to 3.0, but I do think it's better than the more arbitrary calc we had before - my very first version did a conversion to HSL and simply compared L values, and nobody complained about the far more severe problems that caused. There's much more info on the WCAG formula here http://www.w3.org/TR/UNDERSTANDING-WCAG20/visual-audio-contrast-contrast.html#visual-audio-contrast-contrast-73-head |
|
Some color spaces skew luma based on human perceptual differences in ranges of the color spectrum. (i.e. given the same luma of two RGB colors, a typical human being will perceive one as being brighter, for certain color ranges / comparisons). HUSL is one such example. I do agree however that MOST color spaces commonly used will have the same luma value, especially in CSS. So I was kind of wondering aloud why the previous algorithm produced a more pleasing result, and the answer is likely that this is only true for this particular example, and it's otherwise a crap-shoot for mid-range comparisons. WCAG is not a bad guideline for contrast algorithm, so that seems reasonable. There are a few options here:
The reason why I suggest no.3 is that it's a very easy adjustment for the breaking change. We can then say that mid-range comparisons are different, so they may need to add a value of |
|
Surely the only outcome of 3 is that you'll break a different set of people's colours, and require them to change their code to cope with it - how is that different to what we've got now? |
Nope. Not using that parameter would mean it has the current behavior. Unless you're talking about backwards compatibility with the threshold parameter (if used), in which case the range could be from 0 to 1 rather than -1 to 1. It's not an exact match, but by then you're narrowing more and more the potentially affected code base. Still a better outcome than changing the colors without an easy fix IMO. |
|
The major problem with the N3 (not counting me thinking it's just another artificial backdoor crack - since instead of writing a generic code to work with ideally whatever input colors, we again need to test and fine-tune some magic number to meet a set of colors we'd expect), is that it also relies on the order of the color parameters, while the (current version) of the function is supposed to be order independent, i.e. both So my proposal to meet the issue is as always - separate P.S. After all we already have a contrast(darken(#1687af, -50%));
contrast(darken(#1687af, +50%));
contrast(#1687af + #111);
contrast(#1687af * .42);
// etc.tada! Not friendly, sure - but not really less friendly than an explicit parameter that you never know what to set to until you manually try some values with exact colors. |
@seven-phases-max Your proposal is looking less problematic with time. There really isn't a way to reasonably resolve the change. They're different types of calculations. I might propose an alternative syntax. What about a keyword switch rather than a new function name? As in: constrast(wcag, #1687af, black, white);
constrast(legacy, #1687af, black, white, 0.4);With one of them becoming default with the keyword ommitted. It might allow for future expansion. On the other hand, I'm not against |
contrast(wcag, #1687af, black, white);
contrast(legacy, #1687af, black, white, 0.4);I'm afraid it's even more worse. It will be the forth breaking change to the function (relatively) recently and it only makes the situation even more cluttered (after all the issue is as simple as people compile their old sources and get a different result). So I'd rather keep things as is saying "OK, we've done a bad thing, sorry. Here's how you change your code to get what you need": contrast(#1687af * shift, black, white);
// with `shift > 1` to shift the result towards darker color
// and `shift < 1` to shift the result towards lighter colorObviously, a fix like: |
Or revert the PR back and make a new function that could eventually replace both? But yeah, I agree that completely changing the methodology of the function was not great. |
|
If we're going to revert, can we revert now before 3.0 is done? (Whenever that will be?) |
|
I contributed to one of the earlier BC commits. But that was minor in the way that it corrected the output of the color slightly based on the spec. This one completely breaks the functionality as it was intended to work before and breaks dozens and dozens of tests on our application! Can we please make sure that this bug gets fixed ASAP in an upcoming minor update? If you really want parity on function name and behaviour with SASS please schedule it for a major bump in version. |
|
Did anybody already create a PR that reverts this functionality? If not I am willing to pivot it. Can not believe this major break is out there since may! |
Not so "major" counting the number of comments/commenters here, is it? (Yes, it's maybe annoying but nor that critical really, no nuclear plant is going to explode from it AFAIK :P). |
|
I meant major as in the sem-ver definition! Not blaming anyone. Just hoping to get this one resolved! Thanks for the input. Created a PR that reverts the functionality to get this thing moving 🚋. |
|
@roelvanduijnhoven Reverted in 2.7.2. If someone has a suggestion for a new contrast function name for 3.0, please post it. |
|
How about |
|
I initially was against 2 different functions, but as they are incompatible algorithms, I think that's fine. I later found a perceptual contrast algorithm that claims to be even more accurate (linked from another one), so there's lots of ways to do it. There isn't really one that's more correct than another. However, they're not just incompatible algorithms but also different measurements (measurements of different values). It's been argued that the existing (legacy) contrast function doesn't measure contrast. And it's true, it sort of doesn't. It measures the luma threshold to choose a "light" (higher luma) or "dark" (lower luma) value, which @seven-phases-max noted made a breaking change from lightness to gamma-corrected perceptual brightness. So even aligning them as "contrast" functions with 2 algorithms isn't quite right. That was the major problem with this change. It's taking similar inputs but not measuring the same data points against each other. What I mean is:
@Synchro - you said about the threshold value: "what it did was entirely arbitrary - I've never seen any justification for what it did." It isn't arbitrary. It's simply the comparison luma value. It's 0.43 by default instead of 0.5 because that's the perceptual brightness mid-point. The legacy function is not silly or bad, and the threshold value was not an unreasonable way to shift bias. It's a perfectly reasonable function for what it does, but perhaps calling that "contrast" is misleading. So what about... for 3.0: Renaming the legacy function What do you think? |
|
That would work @matthew-dean! What about deprecating this behavior in a new minor release? So in the next 2.x.
Then at 3.x we drop |
|
I'd offer something like |
|
@roelvanduijnhoven It's actually Most of my spare time / energy is focused on helping to get 3.x shipped, so I don't really want to invest more than the minimum amount of time on 2.x releases. Obviously @Synchro and probably others see value in measuring contrast, which really is what his version is really doing (measuring contrast of What about this..... Right now, here's our two functions in pseudo-code: // threshold()
if luma(a) < d output b;
else output c;
// contrast()
if abs(luma(a) - luma(b)) > abs(luma(a) - luma(c)) output b;
else output c;What if we simplified this into a single function:
So, for example, to replicate a threshold (legacy contrast).
To replicate newer contrast:
This would have the advantages of: What do you think? |
|
That's actually quite similar to what I was thinking way back in the very first version of the contrast function. At that time, the only way of implementing conditions was with guards, which was very cumbersome, and getting it to switch colours on a whole theme was just impractical, so I effectively wrote the contrast function as a kind of embedded conditional. At the time I wrote some other oddments that had similarly abstract applications, such as random(). |
|
OR you simplify this whole process to an @a: #222222;
@b: #101010;
@c: #dddddd
// legacy
if(luma(@a) < 0.43, @b, @c);
// wcag contrast
if(abs(luma(@a) - luma(@b)) > abs(luma(@a) - luma(@c)), @b, @c);I think I like this most of all. It's even more generic and solves way way way more problems. |
|
@Synchro Instead of an embedded conditional, how about just a conditional? ;) |
|
I'm leaning towards executive decision-ing this. It makes no sense to create narrow use-case functions who essentially compare as an if/then/else statement instead of just exposing an if/then/else. I mean, we're essentially bending over backwards to add additional things / ways to compare. It adds more complexity every time, instead of a generic comparison function. |
|
Long discussion. I took some time to read the history. The problem with this PR has been that it was a major break in a minor update. That is why I complained, and asked for a revert. However: had this been a major bump (to 3) it would have been fine. Obviously I see that the function So. My take on how to go forward? Land the revert in next minor release (done). And simply update the The only thing I would advise is to deprecate the threshold parameter in the next minor version. So people can prepare this way before moving on to 3. The thing is.. what are you going to tell them? There is no alternative function, right? Update If you land |
|
@roelvanduijnhoven The problem with that is, as noted in this thread, that the newer function does not always produce a desirable result. I think a generic purpose |
|
I think it's worth retaining a standardised function for sites that have to pass WCAG validation (e.g. all US gov sites). It seems silly to require that possibly non-technical authors are up to speed on precise calculations, especially given the difficulties we have had historically in doing exactly that - not having it is asking for a zillion incorrect implementations. |
|
@Synchro A fair point. What about replacing We could retain |
|
@matthew-dean wouldn't that introduce another breaking change by renaming It seems that everybody in this thread is on the same page about the following functions:
It's also great that authors who depended on |
|
@gyoshev No, not renaming I'm fine with using the WCAG I doubt that If someone wants to do that work and make a PR, you're welcome to it. In the short term, let's just address the first question i.e. accept new |
|
My only (minor) objection is that |
|
@Synchro That's true.......... then what do you suggest? |
|
@Synchro |
|
That'll do. |
|
Okay, that's good enough for me. I'll do that unless someone has a strong objection, and I'll add |
No objections, just some concerns of implementation details. I hope you realize that for |
|
@seven-phases-max Haha, yes, once I started to look at parsing and evaluation of |
I think we've discussed this in details at #2072 (comment) but the issue itself about a different thing. The closest one is #1894 but it's too abstract so I guess it'd be fine to create a new one (and closing #1894 in favour of it). |
This PR cleans up the contrast function so that it actually does what it is supposed to! There's a minor BC break in that it no longer uses the threshold param, which is also what SASS has done, but it's unlikely this will actually affect anyone.
Note that I have trashed some earlier commits in my repo that were based on the wrong revision in favour of this single commit.