sass / libsass

A C/C++ implementation of a Sass compiler
https://sass-lang.com/libsass
Other
4.33k stars 461 forks source link

Nested extend only selectors produce invalid CSS #137

Closed nc closed 10 years ago

nc commented 11 years ago
%button-style {
  background: red;

  a {
    color: white;
  }
}

button {
  @extend %button-style;
}

should produce

button {
  background: red; }
  button a {
    color: white; }

but instead produces

button {
  background: red; }
  %button-style a {
    color: white; }
akhleung commented 11 years ago

Thanks for catching this ... I thought I had handled this case, but I'll check again.

zakdances commented 11 years ago

Was a commit ever pushed to fix this?

jory commented 11 years ago

This nesting is also broken for non-placeholder selectors.

.foo {
  background-color: lime;
  a {
    color: white;
  }
}

.baz {
  @extend .foo;
}

should produce

.foo, .baz {
  background-color: lime; }
  .foo a, .baz a {
    color: white; }

but does produce

.foo, .baz {
  background-color: lime; }
  .foo a {
    color: white; }
jory commented 11 years ago

And mashing this one up with #145.

.foo {
  background-color: lime;
  a {
    color: white;
  }
}

%bar {
  @extend .foo;
}

.baz {
  @extend %bar;
}

should be

.foo, .baz {
  background-color: lime; }
  .foo a, .baz a {
    color: white; }

is

Edit: to be clear, sassc outputs nothing in this case.

akhleung commented 11 years ago

@extend is tricky to implement completely, and I can't get to it right away, so I can't give an ETA yet. Sorry!

HamptonMakes commented 11 years ago

Here is a new spec test for this issue...

https://github.com/hcatlin/sass-spec/commit/b26f84ed9703913e0c068f1640bef6af83ed2706

pkieltyka commented 11 years ago

hey guys, I was just wondering if you've had a chance to look into this issue yet?

jpdevries commented 11 years ago

I've also run into this issue. Here are my findings:

When extending silent classes from within another silent class, grunt-sass is producing bad selectors. grunt-contrib-sass generates correct selectors.

gist package that reproduces the issue here: https://gist.github.com/jpdevries/6350682

this:

%visuallyhidden {
    border: 0;
    clip: rect(0 0 0 0);
    height: 1px;
    margin: -1px;
    overflow: hidden;
    padding: 0;
    position: absolute;
    width: 1px;
}

%focusable:active,
%focusable:focus {
    clip: auto;
    height: auto;
    margin: 0;
    overflow: visible;
    position: static;
    width: auto;
    @extend %visuallyhidden;
}

#mydiv {
    @extend %visuallyhidden;
}

generates:

%focusable:active, %focusable:focus, #mydiv { /* bad selector generated by grunt-sass (grunt-contrib-sass works correclty) */
  border: 0;
  clip: rect(0 0 0 0);
  height: 1px;
  margin: -1px;
  overflow: hidden;
  padding: 0;
  position: absolute;
  width: 1px; }
LaurentGoderre commented 10 years ago

@jpdevries I have the same problem

kennethormandy commented 10 years ago

@LaurentGoderre Unfortunately, the feature isn’t implemented yet. I believe #80 is the original issue, but #146 seems to be the main one for it now. @benfrain kindly opened a bounty on it.

LaurentGoderre commented 10 years ago

@kennethormandy I did take a look at all of those related to extend. @jpdevries code ressembles most the one we are trying to achieve.

I would really love to help out on this since we need it but I'm afraid my C/C++ is rather weak

benfrain commented 10 years ago

@LaurentGoderre you can always chip in on the bounty ;) I can't help code wise either but I'm hoping the promise of a mighty tequila for their efforts might inspire those more capable!

akhleung commented 10 years ago

@hcatlin and I are set on getting selector inheritance finished by the end of the year, come hell or high water!

LaurentGoderre commented 10 years ago

@andrew, can we keep this on the radar to implement in node-sass when it lands?

andrew commented 10 years ago

@LaurentGoderre Yup, now you've mentioned me I'll get any further email updates.

akhleung commented 10 years ago

The example that @jpdevries posted should work now. I'll close this ticket since it's subsumed under #146.