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

& + '' needs to be wrapped in parentheses #1170

Closed
kaelig opened this issue May 7, 2015 · 6 comments · Fixed by #1176
Closed

& + '' needs to be wrapped in parentheses #1170

kaelig opened this issue May 7, 2015 · 6 comments · Fixed by #1176

Comments

@kaelig
Copy link
Contributor

kaelig commented May 7, 2015

el {
  @if (& + '' == 'el') {
    content: "It works!";
  }
}

Throws:

invalid selector after '' on line 2 at column 14

See: http://sassmeister.com/gist/f19f7aa4e0fbacde9549

The simplest fix (as suggested by @hugogiraudel) is to wrap & + '' in parenthesis: (& + '').

@KittyGiraudel
Copy link

Also commented Gist: http://sassmeister.com/gist/bd0834fdae880f7a65ea.

@xzyfer
Copy link
Contributor

xzyfer commented May 7, 2015

I can see why this fails. The & + '' is an attempt to make Sass coerce the Selector into a String. It would this isn't happening in LibSass.

The reason it works with the () is because it is now a List. The LibSass engine will evaluate the list values individually, the as a list. That second look at the value is likely where we turn it into a string.

@KittyGiraudel
Copy link

Of all the possible reasons for this bug, I would not have guessed that. :D

@mgreter
Copy link
Contributor

mgreter commented May 8, 2015

IMO & doesn't work in this context, since even when inside a list, the expected result is not correct (given the test above). So I think we actually do not handle Parent_Selector correctly in Binary_Operation (which is what if will be using). But that really is just a wild guess ...

debug_ast from Expression* Eval::operator()(Binary_Expression* b)

Binary_Expression 0x2a03d40 (0@[1:8]-[1:9]) [EQ]
 left:  Parent_Selector 0x2f734f0 (0@[1:8]-[1:9]) <>
 left:  ->Selector_List 0x23c8820 (0@[1:9]-[1:10]) [block:0x2fa5df0] [@media:0] - - -
 left:  -> Complex_Selector 0x2a03b90 (0@[1:12]-[1:12]) [block:0x2fa5df0] [weight:100] [@media:0] - - - -> {+} <>
 left:  ->  Compound_Selector 0x2d87490 (0@[1:12]-[1:15]) [block:0x2fa5df0] [weight:0] [@media:0] - - - <>
 left:  ->   Selector_Reference 0x2ce8cb0 (0@[1:12]-[1:15]) @ref 0
 left:  -> -Complex_Selector 0x2a03b00 (0@[1:12]-[1:12]) [block:0x2fa5df0] [weight:100] [@media:0] - - - -> { } <>
 left:  -> - Compound_Selector 0x2d873c0 (0@[1:10]-[1:11]) [block:0x2fa5df0] [weight:100] [@media:0] - - - <>
 left:  -> -  Type_Selector 0x2ce8c30 (0@[1:12]-[1:15]) <<'a'>> - <>
 right: Parent_Selector 0x2f73560 (0@[1:21]-[1:22]) <>
 right: ->Selector_List 0x23c88e0 (0@[1:22]-[1:23]) [block:0x2fa5df0] [@media:0] - - -
 right: -> Complex_Selector 0x2a03cb0 (0@[1:25]-[1:25]) [block:0x2fa5df0] [weight:100] [@media:0] - - - -> {+} <>
 right: ->  Compound_Selector 0x2d87630 (0@[1:25]-[1:31]) [block:0x2fa5df0] [weight:0] [@media:0] - - - <>
 right: ->   Selector_Reference 0x2ce8db0 (0@[1:25]-[1:31]) @ref 0
 right: -> -Complex_Selector 0x2a03c20 (0@[1:25]-[1:25]) [block:0x2fa5df0] [weight:100] [@media:0] - - - -> { } <>
 right: -> - Compound_Selector 0x2d87560 (0@[1:23]-[1:24]) [block:0x2fa5df0] [weight:100] [@media:0] - - - <>
 right: -> -  Type_Selector 0x2ce8d30 (0@[1:25]-[1:31]) <<'aqwe'>> - <>

@xzyfer
Copy link
Contributor

xzyfer commented May 13, 2015

This fix was a false positive due to sass/sass-spec#380. The spec still fails.

@xzyfer xzyfer reopened this May 13, 2015
@xzyfer xzyfer modified the milestones: 3.2.5, 3.2.3 May 13, 2015
@mgreter mgreter removed their assignment May 13, 2015
@mgreter mgreter self-assigned this May 28, 2015
@mgreter mgreter modified the milestones: 3.3, 3.2.5 May 28, 2015
@mgreter
Copy link
Contributor

mgreter commented Jul 13, 2015

We now have a passing spec test since #1249 was merged 🎈

@mgreter mgreter closed this as completed Jul 13, 2015
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.

4 participants