Skip to content
This repository has been archived by the owner on Dec 28, 2021. It is now read-only.

Concatenative selectors #17

Closed
keithamus opened this issue Mar 14, 2016 · 21 comments
Closed

Concatenative selectors #17

keithamus opened this issue Mar 14, 2016 · 21 comments

Comments

@keithamus
Copy link

Hi @jonathantneal, thanks for making such a cool postcss plugin!

I feel like I've found a potential bug with this. The spec doesn't mention this behaviour specifically, but I'm quite sure it is undesired.

Input:

.foo {
  color: blue;
  &bar {
    color: red;
  }
}

This plugins output:

.foo {
  color: blue
}
.foobar {
  color: red;
}

I would expect to see an error, I think, as the spec suggests & is equivalent of :matches() - which, to my understanding, means they shouldn't concatenate like this.

Perhaps @tabatkins could clarify this?

@tabatkins
Copy link

It's wrong, but only because type selectors don't have an identifying prefix. For all other selectors, you can just naively concatenate them - .foo { &.bar {...}} goes to just .foo.bar {...}, for example. Same for cases where the & is immediately followed by a combinator - naive concatenation works just fine.

So this fails only in two cases:

  1. The inner selector puts the & at the beginning of a compound selector whose non-& part starts with a type selector, like .foo { &div {...}}.
  2. The inner selector puts the & inside of the compound selector, where the parent selector starts with a type selector, like div { .foo&.bar {...}}.

In either case, you just have to do some minor rewriting to make sure the type selector goes first - the first case should result in div.foo, the second in div.foo.bar.

as the spec suggests & is equivalent of :matches()

You're misreading a bit - if the parent selector uses commas (it's a selector list of length >1), you have to enclose it in a :matches() before substituting it into the child, if you're directly synthesizing an equivalent selector. So a, b { & c {...}} becomes :matches(a, b) c {...}. Same if the parent selector has >1 compound selectors and & isn't in the very first compound selector - a b { c & {...}} becomes c :matches(a b) {...}.

So & isn't equivalent to :matches(), but constructing equivalent selectors without & often requires :matches().

@taion
Copy link

taion commented May 5, 2016

Is it possible to add some sort of check for this? Right now this is prone to accidental use for cases like &_title, which is probably not great.

@tbremer
Copy link
Contributor

tbremer commented May 5, 2016

@taion, that's a good point. Could be a pretty quick RegEx check, using something like:

/^&(?: *[a-zA-Z\.#:>\[*+~]|(?:\|\|))/

However, there are pieces that (I think, maybe @tabatkins can confirm) this plugin and the spec diverge, the plugin allows for:

a {
    color: blue;
    & & {
        color: orange;
    }
    & b & {
        color: red;
    }
}

which compiles to:

a {
    color: blue;
}

a a {
    color: orange;
}

a b a {
    color: red;
}

But the Spec doesn't treat the & as selector replacement.

@tbremer
Copy link
Contributor

tbremer commented May 5, 2016

The @nest rules are a little less terse, so checking validity would have to be thought out a little more, however the spec does call this out specifically as invalid:

.foo {
  color: red;
  @nest & .bar, .baz {
    color: blue;
  }
}
/* Invalid because not all selectors in the list
   contain a nesting selector */

But the plugin compiles it to:

.foo {
    color: red;
}

.foo .bar, .baz {
    color: blue;
}

@tabatkins
Copy link

However, there are pieces that (I think, maybe @tabatkins can confirm) this plugin and the spec diverge, the plugin allows for:

Correct, the spec treats & as a real selector that refers to a specific element (whatever's matched by the parent rule), so & & never matches anything; an element can't be its own descendant.

@taion
Copy link

taion commented May 5, 2016

FWIW, I'm a little bit less worried about those edge cases – more as a defensive thing, preventing people from using the nesting syntax for concatenation in "obviously wrong" cases.

@jonathantneal
Copy link
Collaborator

It seems like this plugin cannot emulate what you are saying the specification defines.

For instance, are we saying that class selectors cannot be extended? I imagine almost anyone writing BEM-like CSS is probably using this plugin (alone or within cssnext) to do things like &-button, etc.

@taion
Copy link

taion commented May 5, 2016

👍 to that – I can't imagine many people are writing & &, selectors, but supporting something like &-button that people might actually use incorrectly would be bad.

@tbremer
Copy link
Contributor

tbremer commented May 5, 2016

Yeah, I believe this (see below) is invalid as the spec is written.

.button {
    &__primary {
        &--toggled {}
    }
}

I think @taion brings up a really valid point though if this plugin is to respect the spec then it should conform. Obviously the concern is that this type of change would be a major breaking type of change.

@jonathantneal
Copy link
Collaborator

jonathantneal commented May 6, 2016

Okay, I can update the plugin to effectively work more like :matches, allowing .foo { &div {} to be like .foo:matches(div) or div.foo. I can dismiss or warn out any selector concatenating more than one type selector, causing div { &span {} to break like div:matches(span) . Then, to work around this newly understood meaning of &, I can dismiss or warn out any selector using more than one &. Do I understand the spec a bit better? Would this make the plugin spec compliant?

This would definitely be a major release change, and, sadly, I’m certain many developers will, at some point, blindly pull the major release and discover that all of their BEM concatenations are broken.

@tbremer
Copy link
Contributor

tbremer commented May 6, 2016

There could be a "loose" option that allows selector concatenation.

@oalberdi
Copy link

oalberdi commented May 9, 2016

Missing semicolon on nesting selectors, as shown on the first example. It´s not a rendering issue but would be great to add or have it.

Input

.foo {
color: blue;
&bar {
color: red;
}
}

Output

.foo {
color: blue
}
.foobar {
color: red;
}

@jquense
Copy link

jquense commented May 25, 2016

bit confused does this mean that & + & is also invalid per the spec, its certainly allowed by the plugin?

@tabatkins
Copy link

It's not invalid in the spec, but it's meaningless and will never match anything, because & actually refers to an element, it's not just an alias for some selector text. An element can't be a sibling of itself.

@jquense
Copy link

jquense commented May 26, 2016

Hmm OK, so it's that this isn't actually polyfillable. If you don't mind me asking why not have it be the selector? that approach seems more useful am I just looking at it the wrong way?

@tabatkins
Copy link

Correct, it's not quite polyfillable exactly, without a ton of expensive effort in JS. You definitely can't faithfully polyfill it in all cases as a rewrite step. (You can in the vast majority of cases, tho. Just rejecting more than one & would help; that's actually valid in some cases (like multiple branches of a :matches() pseudo-class), but that's relatively rare.)

It's not text munging because text munging is weird and hacky and always ends up with unexpected consequences. We have real information in the browser; we can use that to do cool things that are more sophisticated than what you can do as a text-rewrite step.

@jonathantneal
Copy link
Collaborator

I think it's very possible to throw on selectors with multiple & and then use the selector parser to throw whenever both the parent rule's deepest selector and any selector chained to the nested selector use a tag.

This is equivalent to checking :matches for selectors like :matches(div):matches(div) before muxing.

@gaastonsr
Copy link

BEM user here. It would be bad if all of the sudden I couldn't do &__element in my code :)

@jonathantneal
Copy link
Collaborator

@gaastonsr, we’ll allow a “loose” option for such things, but it should be off by default, because we want to follow the spec. At the same time, having a separate plugin for loose mode seems excessive.

@jonathantneal
Copy link
Collaborator

I stopped documenting this usage in v3 and I’ve finally removed it in v4. For non-spec compliant nesting, we have the Sassy PostCSS Nested.

@Hypnosphi
Copy link

Correct, the spec treats & as a real selector that refers to a specific element (whatever's matched by the parent rule), so & & never matches anything; an element can't be its own descendant.

@tabatkins is there any reason I can't do && to increase selector specificity? It would still match the same element

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

No branches or pull requests

9 participants