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

Parsing Error "Unrecognised input" for color operations with color names #2124 #2775

Merged
merged 4 commits into from
Jan 15, 2016

Conversation

SomMeri
Copy link
Member

@SomMeri SomMeri commented Jan 14, 2016

Named colors inside expressions are will be parsed as colors.

@SomMeri
Copy link
Member Author

SomMeri commented Jan 14, 2016

Linking original issue: #2124

@seven-phases-max
Copy link
Member

Btw., would not it make sense to add more tests for ambiguous (especially with -sm=off) expressions like:

foo {
    0: 0 -red;  
    1: 1 - red; 
    2: -red * 2;
   @3: -red;

    &-bar@{3} {x: y}
}

@SomMeri
Copy link
Member Author

SomMeri commented Jan 14, 2016

I was confident, but -red * 2; really failed. So, do not merge yet.

@seven-phases-max
Copy link
Member

It's just also similar quite not always predictable story with hex values, for example. So I'm just a bit concerned of more side-effects this could add.


Technically for -red * 2 I'd say it should fail as it can be considered as -webkit-fnord * 2. Thus it's not possible to decide if -red is actually a negated red or just a regular CSS identifier. It's tricky, obviously we do know there's no (and never will be?) such thing as -aqua in CSS, so we could probably assume it as an expression. On the other hand I suspect this would also mean that every identifier (in values/variables) beginning with - (e.g. -webkit-gradient) first has to be tried to be parsed as -color, and only if this fails it falls back to -ident, but this -> bad performance.

Thus I probably would left -red as-is (i.e. -red * 2 -> error) especially since for such ambiguous expressions a workaround will be as simple as red * -2 (or color(red) * -2 as it is now for #2124 in general).


P.S. e.g. just like -round(2.3) results in -round(2.3) currently, not -2.

@SomMeri
Copy link
Member Author

SomMeri commented Jan 15, 2016

Since color-0: 0 -red;compiles into color-0: 0 -red;, then I think -red * 2 should compile into -red * 2 instead of failing.

@SomMeri
Copy link
Member Author

SomMeri commented Jan 15, 2016

Never mind, identifier multiplied by number fails no matter what, so it is better to keep it that way. If it wants to be changed, it would be different issue anyway. So I will just add test you suggested without that.

@seven-phases-max
Copy link
Member

Never mind, identifier multiplied by number fails no matter what, so it is better to keep it that way. If it wants to be changed, it would be different issue anyway.

Yep.

SomMeri added a commit that referenced this pull request Jan 15, 2016
Parsing Error "Unrecognised input" for color operations with color names #2124
@SomMeri SomMeri merged commit 4ce7914 into less:master Jan 15, 2016
namedColor: function () {
parserInput.save();
var autoCommentAbsorb = parserInput.autoCommentAbsorb;
parserInput.autoCommentAbsorb = false;
Copy link
Member

Choose a reason for hiding this comment

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

Investigating #2802. Indeed that's mysterious. W/o autoCommentAbsorb = false;
this property: yes /* comment */; causes infinite parser loop (why?),
but otherwise property: red /* comment */; is obviously expected to fail.
No ideas how to fix this so far.

Copy link
Member

Choose a reason for hiding this comment

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

Never mind, I found another workaround on top of that. Now just polishing a performance for the fix.

Copy link
Member Author

Choose a reason for hiding this comment

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

I added parserInput.autoCommentAbsorb = false because the comments were either doubled or swallowed without it (I do not remember which one).

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

Successfully merging this pull request may close these issues.

2 participants