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

Custom properties inside builtin functions like rgba() throw error #2986

Closed
MikeKovarik opened this issue Nov 13, 2016 · 14 comments
Closed

Custom properties inside builtin functions like rgba() throw error #2986

MikeKovarik opened this issue Nov 13, 2016 · 14 comments

Comments

@MikeKovarik
Copy link

MikeKovarik commented Nov 13, 2016

Syntax of the content of CSS custom properties is very permissive but that causes problems when using such properties inside builtin functions like rgba()

Example:
My project allows user to define accent color on which the whole app UI and various custom components is based. And I am using rgb format because of the need to fade in certain places and CSS does not provide anything like fade(@color, 50%) in less. That way i can do rgba( var(--color), 0.5 ). This works in Chrome and is supported by the standart. However less throws the following error error evaluation function 'rgba': color functions take numbers as parameters.

Example code:

:root {
	--color-accent: 169,57,255;
}
button:hover {
	background-color: rgba(var(--color-accent), 0.2);
	color: rgb(var(--color-accent));
}

Note: This is a drop-in library and i don't want to force users to add another build step into their workflow only to build my less library with their colors. That's why i'm using the new and shiny custom properties (and also because of its scoping capability).

BTW: Is there some temporary workaround that'd skip such strict handling and build it anyway? It's not like I'm missing a bracket in a nested rules.

@seven-phases-max
Copy link
Member

seven-phases-max commented Nov 14, 2016

Just escape.


I put "Feature Request" label since it definitely becomes a problem when the custom properties are in TR. It's easy to fix by disabling any error detection for CSS functions args (though it's quite sad as it kills an important part of the language - i.e. we'll never find a error anymore until debug in a browser).

@seven-phases-max seven-phases-max changed the title using custom properties inside builtin functions like rgba() throws error Custom properties inside builtin functions like rgba() throw error Nov 14, 2016
@matthew-dean
Copy link
Member

Custom properties have support across all browsers except IE11. They'll probably hit mainstream within the next year, so I think support should be implemented soon.

Less, AFAIK, also doesn't have tests for the extremely permissive nature of custom property values. They can contain pretty much anything, even semi-colons, as long as they're not top-level tokens (not contained in bracket or curly-brace pairs). See: https://www.w3.org/TR/css-variables/#syntax

@seven-phases-max
Copy link
Member

Since this change requires an n-of-args checking anyway (to keep at least some error validation within the functions), it's also assumed that the newer rgba implementation also supports the two argument form for ordinal color values, e.g.:

rgba(var(--some), .42); // -> rgba(var(--some), .42)
rgba(#010101, .42); // -> rgba(1, 1, 1, .42)

@matthew-dean
Copy link
Member

How should we handle custom properties in args in general? Technically, you can use var(--some-property) just about anywhere, and if Less is trying to interpret args for built-in CSS functions like rgba(), that's an issue. Although I'm not sure why Less is throwing strict value errors for a built-in CSS function? Just to support expressions?

@matthew-dean
Copy link
Member

I see a whole bunch of built-in CSS functions at https://github.com/less/less.js/blob/master/lib/less/functions/color.js#L34, and IMO most of that shouldn't be there. Less isn't a linter, and those aren't Less functions. If the parser tries to get super-clever at CSS functions, it's going to fail. So I think that's the source of the problem, that Less currently is monkey-patching regular CSS color functions rather than letting them pass-through.

I'm not sure if that was added for browsers that didn't yet support rgb() rgba()? Looks like it was added 4 years ago. Maybe the rationale was to add balance to newer color functions that aren't yet supported in browsers? 🤔 @lukeapage are you still around to answer?

@seven-phases-max
Copy link
Member

seven-phases-max commented Jun 2, 2018

Considering my comment at #3214, for this issue the quick fix would be just: "if (nargs < 3/4) fallback to css string (by returning nothing)" in the corresponding functions implementations.

(And a more strict fix is in putting in checks for every arg in every such func to decide if it's evaluable and returning either a color object or a CSS-string fallback).

@matthew-dean
Copy link
Member

Considering my comment at #3214, for this issue the quick fix would be just: "if (nargs < 3/4) fallback to css string (by returning nothing)" in the corresponding functions implementations.

And a similar check for var() ?

darken(rgba(var(--red-value), 2, 3, .4), 5%)

That is: should we do the following:

  1. Allow pass-through of functions if they don't match n-args? I assume we would need to specially flag functions since there's nothing special about function registry (unless we do something crazy like toString() the function and literally count args with regex -- AFAIK that's the only type of reflection in JS; you may be able to do it with TypeScript but that doesn't help us)
  2. Throw error if non-matching function is used in another Less function evaluation.

This is an interesting problem because the introduction of custom properties has made CSS functions un-resolvable at compile time. To some degree we may be at the end of an era of static pre-processing. But that's a whole different discussion.

@seven-phases-max
Copy link
Member

seven-phases-max commented Jun 4, 2018

This is an interesting problem because the introduction of custom properties has made CSS functions un-resolvable at compile time.

Nothing new really. All necessary facility to handle this with Less is there since its v1.x - see below. The only challenge is to code it in a non-bloating way.

I assume we would need to specially flag functions since there's nothing special about function registry

No, the functions should just return undefined (or null) if they find they can't evaluate their args: like this (unless they also detect the args are 100% erratic and it's better to trigger a error). .
And the undefined return value makes the function evaluator to fallback to the initial string representation: Demo.

And a similar check for var()

Also no, there's no need to check for var or anything specific (basically unknown things from a CSS future). That's the point. The code should check for what is possible to evaluate (known things) instead of what is not possible to evaluate (unknowns).
(though the details may very depending on the specific function - that's not so important).


Btw., notice that --var is also a built-in Less function (as well as any ident(...) thing) that is simply not implemented explicitly (hence it simply falls back to self-string-representation) because there's no need for it to be. But a plugin (for example) may override it with its own implementation that may return potentially evaluable value.

@matthew-dean
Copy link
Member

@seven-phases-max Oh okay, so you think the simple fix is for these CSS/Less functions to return undefined rather than throw an error (to render as-is)? That seems reasonable, then if function no. 2 (like darken()) can't interpret an arg (which at this point would be eval'd to an rgba() anonymous value?), it should still throw?

@seven-phases-max
Copy link
Member

seven-phases-max commented Jun 4, 2018

Arguing with myself:

Also no, there's no need to check for var() or anything specific (basically unknown things from a CSS future).

On the other hand, it would not be a problem (and even tempting) to detect var specifically so that the function can distinguish between rgba(var(...), ...) and rgba(foo, ...) and still trigger a error for the latter, but that would mean a similar issue to raise each time they add something new to CSS.
(Both variants are fine I guess, and it's more about finding the balance and/or predicting the less burden for maintainers...).

@matthew-dean

That seems reasonable, then if function no. 2 (like darken()) can't interpret an arg (which at this point would be eval'd to an rgba() anonymous value?), it should still throw?

Yes, exactly - we don't even need any new code for this (though ideally they (functions like darken) should have more friendly error messages in this case because current "a.toHSL is not a function"-like messages are quite confusing).

@matthew-dean
Copy link
Member

Well, the other thing not mentioned is that something like rgba(calc(1),1,1) is treated by browsers as valid (note both calc and 3 vs. 4 arguments), so we probably shouldn't get too clever about it. I like the idea of just outputting as is as the general rule, if possible.

@seven-phases-max
Copy link
Member

seven-phases-max commented Jun 5, 2018

Yes, this is what I mean with

The code should check for what is possible to evaluate (known things) instead of what is not possible to evaluate (unknowns).

The only valid args for Less rgba are either a number or a color object (if we also consider the "neo"-"CSS4"-like forms like rgba(#123, .42)).
Anything else is either a error or a (unknown but potentially valid) CSS value.

@weivea
Copy link

weivea commented Aug 13, 2019

Syntax of the content of CSS custom properties is very permissive but that causes problems when using such properties inside builtin functions like rgba()

Example:
My project allows user to define accent color on which the whole app UI and various custom components is based. And I am using rgb format because of the need to fade in certain places and CSS does not provide anything like fade(@color, 50%) in less. That way i can do rgba( var(--color), 0.5 ). This works in Chrome and is supported by the standart. However less throws the following error error evaluation function 'rgba': color functions take numbers as parameters.

Example code:

:root {
	--color-accent: 169,57,255;
}
button:hover {
	background-color: rgba(var(--color-accent), 0.2);
	color: rgb(var(--color-accent));
}

Note: This is a drop-in library and i don't want to force users to add another build step into their workflow only to build my less library with their colors. That's why i'm using the new and shiny custom properties (and also because of its scoping capability).

BTW: Is there some temporary workaround that'd skip such strict handling and build it anyway? It's not like I'm missing a bracket in a nested rules.

write like this~

:root {
	--color-accent: 169,57,255;
}
button:hover {
	background-color: ~"rgba(var(--color-accent), 0.2)";
	color: ~"rgb(var(--color-accent))";
}

@matthew-dean
Copy link
Member

@weivea On the latest version of Less 3.x, this is not necessary, just write rgba(var(--color-accent))

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

4 participants