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

warn when invalid values are passed to functions and clamped? #821

Closed
lunelson opened this issue Jun 4, 2012 · 17 comments
Closed

warn when invalid values are passed to functions and clamped? #821

lunelson opened this issue Jun 4, 2012 · 17 comments

Comments

@lunelson
Copy link

lunelson commented Jun 4, 2012

If you define a color like this:

@color: hsla(0,100,100,1);

...it should produce 100% red, but it produces 100% cyan.

Also, the corresponding readout function of hue() is producing the incorrect value too; although if you use the two functions together the errors cancel each other out.

Still hsla() should be fixed to work correctly for defining new colors.

@dmcass
Copy link
Contributor

dmcass commented Aug 6, 2012

I just want to verify your problem here, because your code example and expected result are both incorrect. HSL is not the same as HSV (aka HSB), and it seems you expect they are. You're also missing percentage signs from your Saturation and Lightness values, and the fact that LESS was still able to process that surprises me since it's not the correct type.

hsla(0, 100%, 100%, 1) will produce white, since the lightness is set at 100%.
hsla(0, 100%, 50%, 1 ) is red.

Chances are the reason you're experiencing this problem is because you're passing numbers instead of percentages to these functions.

@lunelson
Copy link
Author

lunelson commented Aug 6, 2012

Yes, you're right I wrote the lightness at 100 in my example, which is wrong; I should have wrote it at 50.

However I think you've located the bug with the units:

@color: hsl(0,100,50); produces cyan, while
@color: hsl(0,100%,50%); produces red, which is the precise opposite --

and there's no particular reason why the function should require the units. If it were strictly so, then the proper syntax should also require the addition of deg for the hue value. So I would maintain that this is still a bug and the function should be fixed to produce the correct values with or without deg and % units.

@dmcass
Copy link
Contributor

dmcass commented Aug 6, 2012

HSL is implemented within LESS the same way it is implemented in CSS. In CSS, using HSL requires units on the Saturation and Lightness values, and no unit on the Hue value. If anything, LESS should throw an error if units are not used correctly within the HSL or HSLA functions.

@lunelson
Copy link
Author

lunelson commented Aug 6, 2012

OK, well I didn't think you could use hsl() directly in CSS. When you use it in LESS it returns an rgb hex value doesn't it? So the LESS implementation of hsl() doesn't really need to observe the same limitations as CSS even if it provides the same capability and supports the same input.

But an error is better than silently returning the wrong color, so I think that's a good minimal solution; I just personally feel that one of the advantages of using a preprocessor like LESS is you can improve the functionality and usability of simple things like this...

Thanks for the feedback, glad to see this issue got some attention.

@lukeapage
Copy link
Member

Actually the function takes values 0-1 or a value with a percentage
e.g. 50% or 0.5

So I don't think it is a bug.

I have raised a documentation bug
less/old-lesscss.org#28

Does that close the issue for you?

@lunelson
Copy link
Author

Thanks Luke,

that's good to know; but FWIW I still believe that if the requirements for the arguments -- be what they may -- are not met, the function should return an error rather than silently return the wrong color.

Debugging one's own code for a false value is one thing, but a missing unit which causes a function to behave totally differently can be much harder to catch

@lukeapage
Copy link
Member

It's difficult because some people may depend on passing invalid values and
it being clamped.. so do you error on 1.1.. 2... Or something else?

@dmcass
Copy link
Contributor

dmcass commented Aug 22, 2012

Maybe this is a case where a warning would be appropriate instead of an error? At least you get an idea of what's going on in that case.

Either way, I'm not sure we should cater to a case where people are passing invalid values to the function.

@timkindberg
Copy link

After a 16 hour day trying to do a very custom mixin, I am foiled by the fact that the hsla function does not clamp the input values, can we please move the priority up on this?!!!

I need this feature so badly it hurts. I've resorted to forking and editing less.js.

I'm creating an application that will allow admins to set 4 core theme colors to anything they want. So I need EVERY color in my application to based off of those 4 colors. I coded up a sweet (what I thought was sweet) mixin that will let me write:

/* #appcolor is mixin group, it has various mixins: 
//           color(), border(), background-color(), etc
// In this example I use .color()
//
// .color() takes two parameters. 
// 1. The style color input
// 2. The core theme color to compute it against
// let's assume that @mainBgColor is hsl(0, 100%, 50%) by default 
*/

#appcolor > .color(hsl(0, 80%, 80%), @mainBgColor);

CSS Output would be:

/* if @mainBgColor is set to default there is no change from input*/
color: hsl(0, 80%, 80%)

/* but if admin sets @mainBgColor to say hsl(50, 90%, 60%) 
// it does calculations so that the output color is the same relation to mainBgColor.
// It does this by looking at the difference between h, s, and l.
// The differences in original input and @mainBgColor were h: 0, s: -20%, 30%.
// It does a lot more than this, but that's the gist.
*/
color: hsl(50, 70%, 90%)

Now, in my real code I don't really use hsl colors too much, I just used them in this example so you could see the computed channels.

The result overall is simply stunning. Every color, every nuance can be controlled by those four colors. EXCEPT I ran into a huge issue when ever the code ends up passing an "out of range" value to hsla(). It's absolutely heartbreaking. I've tried everything I can think of: javascript interpolation (only returns strings, can never get them back to a tree.Dimension), store variables into variables, use darken and other functions but then there is not way to merge the h, s, and l back together.

Can we move this up?!

I would expect that all functions do clamping if it makes any sense to do it.

@lukeapage
Copy link
Member

@timkindberg can you give me an example of something that doesn't work. I'll add in clamping. I noticed that when the color is output as a hex it doesn't seem to be a problem, e.g. hsl(380, 150%, 150%); outputs hsl-clamp: #bfffff; but when it is rgba we get values > 255

This issue is about warning when clamping - if a function is causing problems by not clamping then that is higher priority.

@lukeapage
Copy link
Member

ah and clamped it is hsl-clamp: #ffffff; will be in 1.4.0 beta.

@timkindberg
Copy link

So you just fixed it or it is already fixed? Can you point to where in the src it is now clamped? FYI, I'm working with less 1.3.3.

So here's an example of where I'm running into an issue. So I'm doing calculations and storing a hue number, sat percentage, lightness percentage and alpha number into variables like this:

@h:hue(@src_user)-(hue(@src_def) - hue(@def));
@s:saturation(@src_user)-(saturation(@src_def) - saturation(@def));
@l:lightness(@src_user)-(lightness(@src_def) - lightness(@def));
@a:alpha(@src_user)-(alpha(@src_def) - alpha(@def));

In this particular case I end up with the following values for my variables, because performing calculations like above doesn't keep your values from going "out of range", my @s var is a negative number now.
@h: 196
@s: -20%
@l: 62%
@A: 1

When I pass those values into hsla() I get these results:

@c: hsla(@h, @s, @l, @a);
color: @c;
/* next line works similar to console.log for css, it appears in web tools as invalid css */
output-info: hue hue(@c), saturation saturation(@c), lightness lightness(@c), alpha alpha(@c);

CSS Output:

color: #B1958B //sort of dark orang, expecting desaturated blue
output-info: hue 16, saturation 20%, lightness 62%, alpha 1; 
/* hue and saturation values are not as expected */

@lukeapage
Copy link
Member

@timkindberg I fixed it on master so it will be in the next version of less - the beta is on npm and for download in the dist folder.

@lukeapage
Copy link
Member

To clarify anyone looking at this bug now 1.4.0 is out - the last thing is to allow warnings and to give a warning when a variable is clamped.

@seven-phases-max
Copy link
Member

Changing label to "feature-request" (since this is definitely not a bug but intended behaviour).

@matthew-dean
Copy link
Member

I think generating "warnings" is a separate issue, yes? Can someone find which issue this is dependant on?

@seven-phases-max
Copy link
Member

I'm not sure if there's corresponding issue. The specific no % warnings for colour function were discussed at #1665/#1669.

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

No branches or pull requests

7 participants
@timkindberg @lukeapage @dmcass @matthew-dean @lunelson @seven-phases-max and others