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

Contrast Incorrect 2.7.1 #2906

Closed
ParsonsProjects opened this issue Jun 3, 2016 · 18 comments
Closed

Contrast Incorrect 2.7.1 #2906

ParsonsProjects opened this issue Jun 3, 2016 · 18 comments

Comments

@ParsonsProjects
Copy link

When using the latest node version 2.7.1 the contrast function seems to be returning incorrectly please see http://codepen.io/ParsonsProjects/pen/ezNWmg when changing the cdn file from 2.7.1 to 2.6.0 the contrast on the body goes from black to white.

This is using the color #2f905d to contrast.

Is this working correctly??

@Synchro
Copy link
Member

Synchro commented Jun 3, 2016

That colour is pretty close to a mid grey in brightness terms, so it's likely the recent changes to the contrast function (removal of threshold) is making it flip colours - it might be different, but it should be more correct that it was before.

@ParsonsProjects
Copy link
Author

(@Synchro) I think i will add some more colors to test this.

@ParsonsProjects
Copy link
Author

Added some more color examples and I would have expected the contrast to give white text on the red background for better readability. Which it does in 1.6.0

@ltodorov
Copy link

ltodorov commented Jun 17, 2016

Such of change to contrast or any other function has to be treated as a major not minor release. I used contrast in several projects and suddenly the result has changed. It is not good to get black instead of white without any change in your color values.

b76db17#diff-8c2e50ef84a06711745a46e22c67d631R1721

@matthew-dean
Copy link
Member

Such of change to contrast or any other function has to be treated as a major not minor release

That's probably correct. I wasn't aware that the contrast change was making such drastic changes. In fact, I thought these were fixes for incorrect contrast calculations. I didn't realize it was a different function signature altogether. @Synchro?

@Synchro
Copy link
Member

Synchro commented Jun 17, 2016

The signature is only different in that it ignores the pointless threshold param; param meaning and order are unchanged, so there is no BC break. The calculations are not drastically different, but the old one was demonstrably wrong, especially in the critical midrange, so it was definitely a bug fix. That doesn't mean that the output remains identical, but that the old output was a bug. If we had an add function that returned 50+50=98 and it was fixed to return 100, would you revert it because someone was relying on the incorrect value? It's the same situation here. Either you want colours with correctly-calculated contrasting colours, or you want fixed colours. If you don't want the former, don't use contrast. Any flip from back to white (or the reverse) on your site meant that your site was previously suffering from the consequences of a bug, which is now fixed, and you now have better contrast than you did before, i.e. the contrast function is doing exactly what it was designed to do.

@matthew-dean
Copy link
Member

If we had an add function that returned 50+50=98 and it was fixed to return 100, would you revert it because someone was relying on the incorrect value?

It depends. As a parallel, IE6 had these kinds of math problems all over the place in the layout engine. So developers began to depend on them. 50+50 did indeed equal 98, so when you wanted a result of 98, you added 50+50. IE7 fixed many IE6 bugs, but had a whole slew of different ones. IE8 fixed a lot of those bugs, and in fact, fixed so many bugs that Microsoft decided to create "Compatibility Modes" where you could reliably get 50+50=98 if you needed to.

It sounds crazy, but their reasoning was they didn't want to break the entire Internet which had built a whole infrastructure with a dependency on errors. AND at the same time, they wanted to of course fix the bug as a default setting.

Any flip from back to white (or the reverse) on your site meant that your site was previously suffering from the consequences of a bug

So another, probably more correct way to say this is that a given site was depending on the bug, not suffering from it. The site wasn't in any particular pain, and the result would have been visually inspected to see that it had the desired result.

What would be best if contrast is going to stay moving forward would be a way for the developer to easily adjust the new result. IMO the final threshold parameter should be used to "weight" the result towards color1 or color2. This would be similar to how the previous contrast function worked. Then, if a developer upgrades to Less and it does cause some different color calculations, they should be able to just tweak the threshold for that particular value.

@demiankatz
Copy link

I've just run afoul of this problem as well -- the new version of the compiler is choosing black as a contrasting color to #619144 instead of white, which looks wrong to me.

In any case, specific colors aside, I think this is a question of interpretation of the spec:

http://lesscss.org/functions/#color-operations-contrast

My reading here is that the threshold value is not pointless. The idea here is that you provide a "light" color, a "dark" color, and a "threshold." If the color we are contrasting is below the threshold, we return the "light" color; otherwise, we return the "dark" color. This allows fine-grained control of how different colors are picked, and there is no implication of direct comparison between the color being contrasted and the provided light/dark values.

The new code as of release 2.7.0 may be smarter about picking the mathematically best contrasting color (though I'm not convinced that picking black as the contrast for #619144 is right even in theory), but it is in any case not behaving according to the specification (assuming I'm interpreting the specification correctly). Being able to use the threshold value to adjust the algorithm does serve a purpose, and some people may actually be depending on it.

I should also note that I initially noticed this problem because I was comparing outputs between this compiler and a PHP-based one. The JS and PHP code agreed until this change was made... so at least one other developer seems to follow my interpretation of the spec.

In any case, compliance with spec is a whole separate question from "what is the best way to pick a contrasting color?" If this new logic is working better for some people, maybe a new autocontrast() function needs to be defined that has no threshold parameter, so people who want the new functionality can get it, but people depending on the historical functionality aren't unexpectedly broken.

@Synchro
Copy link
Member

Synchro commented Aug 9, 2016

The contrast ratio of white to #619144 is 3.72:1; the ratio of black to #619144 is 5.64:1. 5.64 is a passing grade for WCAG 2 AA, 3.72 is mostly not, and that matters to many. I checked this in several different WCAG contrast calculators (1, 2, 3, 4) and all produce the same result as Less, so I'm pretty certain the calc in Less is now correct. If you have a problem with that, I suggest you file a bug report with WCAG, or design a calculation that works demonstrably better across all possible colours, not just your pet combo.

This means (as has been discussed to death already) that the old Less behaviour was buggy and wrong. If you're saying you don't like how it looks, that's an entirely different proposition; this isn't really an aesthetic function, it's a technical one, implementing a specific calculation. If you want things to use specific colours, use specific colours.

The threshold value was only introduced for compatibility with SASS, and was dropped because they dropped it too. The threshold param didn't work as you describe - both colour choices (which could be supplied either way around - calling them light and dark was misleading) were compared with the base colour to give two ratio values, and the threshold allowed the choice to be biased one way or another, but it was very arbitrary - essentially you'd plug in some colours and use threshold to bodge the output for aesthetic reasons. It might be reasonable to reintroduce the threshold param to return strict WCAG results if it's not supplied, and ignore compliance issues if it is.

Meanwhile I am entirely in agreement that some are unhappy that it changes some specific combinations (though it means they are benefiting from a bug fix, not automatic art direction), so it should be pulled from 2.7 and pushed into 3.x as a BC break, as has been mentioned already. Can we just get on with that now as it's all getting a bit silly?

@matthew-dean
Copy link
Member

If you have a problem with that, I suggest you file a bug report with WCAG, or design a calculation that works demonstrably better across all possible colours, not just your pet combo.

Let's maybe be a bit more diplomatic. ;)

I agree that the problem is well-understood here, so more comments are not needed. #2754 has become more of a solution thread for this.

@demiankatz
Copy link

Apologies for my ignorance of the technical details, and thanks for the expansion/clarification/cross-references. All are appreciated! In any case, as you say, documenting this as a BC break is all it should take to make me (and likely everyone else involved) happy in the long run.

On a related note, would it be possible/feasible/worthwhile to add a note to the docs on lesscss.org about this issue so that the evolution of the function is documented? Hopefully that would help prevent more people from opening/amending GitHub issues about this in future. (Just a suggestion, though -- not trying to be a pest).

@jasonhibbs
Copy link

jasonhibbs commented Sep 26, 2016

+1

Contrast is not behaving as previously expected. All my use of contrast now compiles wrong as of 2.7.1. luma threshold parameter is not being taken into account, tried from 0% to 100%, no effect.

@seven-phases-max
Copy link
Member

seven-phases-max commented Sep 26, 2016

@jasonhibbs A few examples please. From previous examples (see also #2754) we've only seen that wrong expectations are quite common.

luma parameter is not being taken into account

There's no luma parameter.

@jasonhibbs
Copy link

jasonhibbs commented Sep 26, 2016

@seven-phases-max

There's no luma parameter.

Apologies, threshold. Will be happy to collect an example, but for now, I’ve had to switch the contrast function in 2.7.1 with the previous from 2.6.1 throughout our office.

@seven-phases-max
Copy link
Member

seven-phases-max commented Sep 26, 2016

threshold

I see. I'm afraid there's no threshold anymore too. It's unfortunate the breaking change was done in a minor release and haven't been documented, but it's just done. See #2754 (comment) "P.S.-part" for examples on achieving the same effect w/o any additional parameter (essentially setting threshold or whatever else artificial parameter is nothing but adjusting the "condition" color value).

@jasonhibbs
Copy link

// The threshold param is no longer used, in line with SASS.

I was hoping this was a joke 😅… thanks for the tip, though!

@roelvanduijnhoven
Copy link
Contributor

Seems to be related to #2754. Not too happy about it, so I gave my 2ct there.

@matthew-dean
Copy link
Member

Reverted in 2.7.2 and 3.0.0 alpha

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

No branches or pull requests

8 participants