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

Function variables are changed when using numbers and underscores together #2462

Closed
bradcornford opened this issue Feb 21, 2015 · 13 comments · Fixed by #2485
Closed

Function variables are changed when using numbers and underscores together #2462

bradcornford opened this issue Feb 21, 2015 · 13 comments · Fixed by #2485

Comments

@bradcornford
Copy link

When placing numbers before underscores in function variables, these seem to be changed to include a space character between the two.

Take the following less snippet:

@type: 5_large;

.icon-@{type} {
    background-image: ~"url(/img/icon/@{type}.svg)";
}

The output is as follows, notice the space between 5 and the underscore:

.icon-5 _large {
  background-image: url(../img/icon/5 _large.svg);
}
@seven-phases-max
Copy link
Member

Confirmed.
A minimal example:

@type: 5_large;
foo {
    bar: @type; // -> 5 _large
}

@lukeapage
Copy link
Member

I think it is a difficult one - if keywords began with numbers then we cannot read 5px as a dimension. I'm not a fan of trying to work out if a variable is

  • a string (as in this case - the correct fix is @type: "5_large";
  • a selector
  • something meaningfull in css (e.g. dimension, keyword, expression and a million other things)

I think @type: 5_large; should give a syntax error - but we don't check we have whitespace after interpretting a node - and it would probably cause issues if we did - but I think we should do that.

What do you think @seven-phases-max

@seven-phases-max
Copy link
Member

@lukeapage

I think it's #2485, i.e. just missing _ in the regexp. I.e. since 5foobar is parsed as a dimension (with foobar as unit) I guess it's OK to parse 5_foobar as a dimension too, so it's just missing _ in the re.


Though now I can see the same issue applies to values like 5-foo-bar (which technically could also be parsed as a single dimension value), but that's more difficult case since then it needs to distinguish between 5-foo-bar and 5-foo-6 thus requiring more complex regexp there (so I did nothing for it in the patch).

P.S. Doh! And then obviously the same applies to values like 5foo7bar (which also technically possible to be a single dimension with foo7bar as a unit but changes for that start to be even more disturbing requiring too many new tests).

@lukeapage
Copy link
Member

I'm in favour of saying its not a bug and adding a check whitespace function to the parser so it throws an error (perhaps mentioning, should this be a string?)

@rjgotten
Copy link
Contributor

rjgotten commented Apr 3, 2015

You could always just have the parser accept only those strings that are defined as actual unit identifiers in the official CSS3 Values and Units Module, as literal dimensions.

Those are the only units that have sane defined behavior in CSS anyway. And if a user really, really wants a 'custom unit' ... well; there's the unit(scalar, unit) function to make that happen. Dimensions with custom units is really not something that the actual parser itself should have any concerns over.

Make this part of strictUnits if people need the fix and you're afraid of breaking back-compat.

@seven-phases-max
Copy link
Member

I still can't see why 5whatever and 5_whatever should behave differently. Yes, I realize that formally both are more like of Anonymous type rather than of Dimension (not counting though that the CSS spec. probably never mentions that some new units they hypothetically invent may not start with _). But since Dimension can handle it just fine, why especially forbid it? (not counting those additional 5-foo-bar examples though).

@seven-phases-max
Copy link
Member

(not counting though that the CSS spec. probably never mentions that some new units they hypothetically invent may not start with _ ).

Hmm, curiously they actually directly allow this (not counting there's no such units currently):
dimension-token is number-token->ident-token and ident-token may begin with _.

So purely nominally 5_large is valid CSS dimension value :)

@rjgotten
Copy link
Contributor

Hmm, curiously they actually directly allow this

Not that curious, actually.

An ident-token is, iirc, also used for property declaration names. The leading underscore on property names was a browser hack for targeting old IE versions. Establishing it as valid syntax that browsers should process is a form of paving the cowpath. ;)

@seven-phases-max
Copy link
Member

An ident-token is, iirc, also used for property declaration names.

Underscores in property, class, keyword etc. etc. names are expected since those are identifiers... Units (e.g. px or %) contrary are never mentioned in the spec. as kind of an ident token hence it was quite surprising to see this in the grammar.

@matthew-dean
Copy link
Member

The spec is actually surprisingly permissive for the purpose of consistency. So yes, a unit can be pretty much any identifier, for the reason that the parsing does not change as units are added. I don't think it's so much about being permissive to IE. CSS gives specific instructions on how things are to be parsed, so I think they considered that in forming the grammar.

@seven-phases-max
Copy link
Member

So just formally, despite of the original example, the following minimal example:

@type: 5_large;
foo {
    bar: @type; // -> now `5 _large` but should be `5_large`
}

is about to be fixed even if we know for sure there's no such unit... is it?

@SomMeri
Copy link
Member

SomMeri commented Dec 7, 2015

Fixed by #2485

@bradcornford
Copy link
Author

Thanks for this! Seems to fix the issue!

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

Successfully merging a pull request may close this issue.

7 participants
@lukeapage @matthew-dean @rjgotten @SomMeri @bradcornford @seven-phases-max and others