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

selectors are not correctly parentized in respect to placeholders #1654

Closed
tomgenoni opened this issue Oct 28, 2015 · 18 comments
Closed

selectors are not correctly parentized in respect to placeholders #1654

tomgenoni opened this issue Oct 28, 2015 · 18 comments

Comments

@tomgenoni
Copy link

Using gulp/grunt-sass with libsass v3.3.1:

%foo {
  border: none;

  &__bar {
    display: block;
  }
}

.zoo {
  @extend %foo;
}

Outputs

.zoo {
  border: none; }
  __bar.zoo {
    display: block; }

Expected output

.zoo {
  border: none; }

cc @danoc

@mgreter
Copy link
Contributor

mgreter commented Oct 28, 2015

This looks like a ruby sass bug to me, please see this sample:

%foo {
  border: none;
  &bar {
    display: block;
  }
  &.bar {
    display: block;
  }
}

.zoo {
  @extend %foo;
  &baz {
    display: block;
  }
  &.baz {
    display: block;
  }
}

Results in ruby sass to:

.zoo {
  border: none; }
  .bar.zoo {
    display: block; }

.zoobaz {
  display: block; }
.zoo.baz {
  display: block; }

From the semantics I don't see why libsass should be wrong here?

//CC @nex3 @chriseppstein

@danoc
Copy link

danoc commented Oct 28, 2015

@mgreter – We only ran into this issue when using the BEM shorthand notation (&--) within a placeholder that is extended.

Ruby Sass behaved as we expected.

@mgreter
Copy link
Contributor

mgreter commented Oct 28, 2015

What's the reason for having ...

  &__bar {
    display: block;
  }

... when it is never output? I mean you seem to expect it to be never output?
Also you did not post anything with &-- in your sample, so not sure how that's relevant.
The ruby sass behavior is definitely not consistent, so I consider it a bug on their side ATM.
Lets see if anyone from their side can give some insight here ...

@danoc
Copy link

danoc commented Oct 28, 2015

@mgreter – I'll defer to @tomgenoni for the first question but I should have noted that both &__ and &-- produce similar results.

@mgreter
Copy link
Contributor

mgreter commented Oct 28, 2015

As you can see in my sample posted above, the "prefix" doesn't matter at all.

@tomgenoni
Copy link
Author

@mgreter This is just a reduced case to reproduce the issue. It's a change in behavior from the previous version. You can see the expected output with an earlier version of Libsass and Ruby Sass at http://sassmeister.com/gist/4c34daba378d7269af35

@xzyfer
Copy link
Contributor

xzyfer commented Oct 28, 2015

@mgreter IMHO this is a bug. It's work noting that &-- and &__ had their own semantics. This behaviour was introduced in Ruby Sass 3.3.0.

The parent selector, &, can be used with an identifier suffix. For example, &-suffix and &_suffix are now legal. The suffix will be added to the end of the parent selector, and will throw an error if this isn’t possible. & must still appear at the beginning of a compound selector – that is, .foo-& is still illegal.
Source

Maybe @chriseppstein can give more information on the exact semantics.

@mgreter
Copy link
Contributor

mgreter commented Oct 28, 2015

You all see that ruby sass is not consistent between extend, regular parent, type selector and class selector? @xzyfer from the quoted docs I don't see why in the sample above &__bar should be hidden? And as one can see it is shown when inside a "regular" ruleset. Also note that this interestingly changes when you use a class selectors (which is the bug I suspect to be on ruby side).

@chriseppstein
Copy link
Contributor

@extend is applied after nesting is resolved. &baz creates a new class built from the classname represented by & so @extend doesn't match against it. If the parent selector is a placeholder, it creates a new placeholder that never gets extended and that is then dropped from the output.

Does this resolve the confusion?

@mgreter
Copy link
Contributor

mgreter commented Oct 28, 2015

Not really, why does it work different for &.bar and &bar? And why is it only different for @at-root cases and not for regular rulesets?

@mgreter
Copy link
Contributor

mgreter commented Oct 28, 2015

OK, I think I need to repeat my sample from above, since I'm not sure you see the difference!?

.zoo {
  &baz {
    display: block;
  }
  &.baz {
    display: block;
  }
}
.zoobaz {
  display: block; }
.zoo.baz {
  display: block; }

vs.

%foo {
  &bar {
    display: block;
  }
  &.bar {
    display: block;
  }
}
zoo {
  @extend %foo;
}
zoo.bar {
  display: block;
}

This doesn't seem consistent!?

@mgreter
Copy link
Contributor

mgreter commented Oct 28, 2015

I think I get it now, so %foo is actually a selector in ruby sass. Too bad, this seems to be handled completely different in libsass. This will probably mean a complete refactor of how we do @extend. So what you are saying is: ruby sass resolved it to: %foobar, which is not found, while %foo.bar is.

@xzyfer AFAIK you made that implementation. Any thoughts on that one?

@mgreter
Copy link
Contributor

mgreter commented Oct 28, 2015

IMO that's pretty funky and not as I intuitively expected it to be. I always thought of it like "add the optional placeholder postfix and the block where the @extend happens". Didn't think the placeholder block would be "parentized" in its own context first. Here another test case:

%foo {
  &bar {
    display: inner;
  }
  &.bar {
    display: outer;
  }
}
zoo {
  @extend %foobar;
}

Ruby Sass result:

zoo {
  display: inner; }

@xzyfer
Copy link
Contributor

xzyfer commented Oct 28, 2015

I didn't know that about %placeholders. @mgreter the extends implementation
was written by a third party just before I joined.

IMHO it would be worth while doing a line by line reimplementation of the
Ruby version. The Ruby extend code is relatively self contained. I believe
this is how the original implementation was done.
On 29 Oct 2015 6:20 am, "Marcel Greter" notifications@github.com wrote:

IMO that's pretty funky and not as I intuitively expected it to be. Here
another test case:

%foo {
&bar {
display: inner;
}
&.bar {
display: outer;
}
}
zoo {
@extend %foobar;
}

Ruby Sass result:

zoo {
display: inner; }


Reply to this email directly or view it on GitHub
#1654 (comment).

@tomgenoni
Copy link
Author

It appears that with the BEM selector the .class--modifier is being reversed to --modifier.class

%foo,
.foo {
 display:block; 

  &--up {
   border: none;
 }
}

.zoo {
  @extend %foo;

  &--up {
    @extend %foo--up;
  }
}

expected:

.zoo,
.foo {
  display: block;
}

.zoo--up,
.foo--up {
  border: none;
}

with Libsass v3.3.1:

.zoo,
.foo {
  display: block; 
}

 --up.zoo,
 .foo--up {
    border: none; 
}

--up.zoo should be .zoo--up. I think that's the extent of this issue.

http://sassmeister.com/gist/0d5e1935dfb0538f908e

@mgreter
Copy link
Contributor

mgreter commented Oct 28, 2015

The actual reason is that we parentize selectors after the evaluation step. Ruby sass seems to do it before, but I still try to get together more spec test, so we don't implement it wrongly again. I wonder why this sample does not give any errors nor can I figure out what the correct extend name should be:

moo {
  &%foo {
    &bar {
      display: inner;
    }
    &.bar {
      display: outer;
    }
  }
}
zoo {
  &foo {
    @extend %moofoobar;
  }
}

@chriseppstein any suggestions if this should be extendable at all or if it should error?

@mgreter
Copy link
Contributor

mgreter commented Oct 28, 2015

OK, got some something:

moo {
  &%foo {
    &bar {
      display: inner;
    }
    &.bar {
      display: outer;
    }
  }
}
zoo {
  &foo {
    @extend moo%foobar;
  }
}

results in

zoofoo {
  display: inner; }

Is this the expected behavior? Still not very intuitive IMHO!

@chriseppstein
Copy link
Contributor

@chriseppstein any suggestions if this should be extendable at all or if it should error?

It should error. But the reason it is erroring is that you have two element selectors and so there is no way to extend one element by another because every selector can match at most one element name.

I think I get it now, so %foo is actually a selector in ruby sass.

Bingo. Before we called them placeholders we called them "silent classes".

When in doubt, replace the % with a . because placeholders have the exact semantics as class names except selectors containing a placeholder are stripped from the final output.

This will probably mean a complete refactor of how we do @extend.

I don't see why. Just remove any special handling of placeholder selectors. Treat them exactly like class names. After extend, you remove all selectors that have a placeholder as any selector component.

Is this the expected behavior? Still not very intuitive IMHO!

Yes, this is expected. If we did the operation in the order you suggest, then @extend would not be stable against refactoring the selector hierarchy. This should never happen, nesting is just another way of forming CSS selectors and should never change the semantics of those final selectors. E.g. if someone has written .foobar { ... } .foobaz { ... } and later refactors this to .foo { &bar {...} &baz {...}} why should these suddenly become extended by some .something { @extend .foo}? In both cases, there is not any such selector .foo. Now, I get that people who are writing with BEM methodology do see something special about that parent selector in that it forms a namespace for them. Since they conceptualize it as "special" they expect Sass to see it as special as well, but Sass is a general authoring tool and we cannot assume that the user is writing with BEM or some other namespacing methodology.

You're not the only one who is confused by this. A lot of people have a hard time understanding @extend. They think it is a kind of "search and replace" but it's not. Extend has very specific meaning. A { @extend B; } says "style everything matching selector A as if it matched selector B".

@mgreter mgreter self-assigned this Nov 1, 2015
@mgreter mgreter changed the title @extend using BEM style syntax with placeholders outputs unexpected CSS selectors are not correctly parentized in respect to placeholders Nov 1, 2015
mgreter added a commit to mgreter/sass-spec that referenced this issue Nov 1, 2015
mgreter added a commit to mgreter/libsass that referenced this issue Nov 1, 2015
@mgreter mgreter added this to the 3.3.2 milestone Nov 1, 2015
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

5 participants