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

specificity changes with recent changes to @extend selector rewriting #324

Closed
chriseppstein opened this Issue Mar 18, 2012 · 15 comments

Comments

Projects
None yet
4 participants
@chriseppstein
Copy link
Member

chriseppstein commented Mar 18, 2012

Consider this scss:

%base { color: red; }
body { @extend %base; }
body.testing { @extend %base; }

Until recently this would generate:

body, body.testing { color: red; }

But with recent selector rewriting optimizations it now generates:

body { color: red; }

I understand how this optimization is possible, the body selector is a superset of body.testing so why bother generating both?

But I have concerns because this changes the specificity of the selector. Consider the following sass:

body.something { color: blue; }
%base { color: red; }
body { @extend %base; }
body.testing { @extend %base; }

Before this change, a <body class="something testing"> element would have been red. After this change it will be blue.

Now, due to selector rewriting, the document order is changing which means that the following code may already violate user expectations (I have at least a couple instances of users being confused by this):

%base { color: red; }
body.something { color: blue; }
body { @extend %base; }
body.testing { @extend %base; }

But this is pretty easy to explain to users -- they might not be able to reason about output specificity changing by simply adding a new extend of a superselector of an existing selector extending the same selector.

@nex3

This comment has been minimized.

Copy link
Contributor

nex3 commented Mar 18, 2012

Ouch. My instinct is that changing specificity here is unacceptable. We may have to build in a specificity checker to resolve this in general.

@nex3

This comment has been minimized.

Copy link
Contributor

nex3 commented Mar 19, 2012

After thinking this over some more, I'd be pretty unhappy with just flatly removing this behavior. It cleans up a lot of really nasty output-explosion cases; see for example this presentation slide detailing a case where eighteen almost-certainly-unnecessary selectors are created. These selectors do have higher specificity than the smaller but almost-certainly-sufficient set of selectors that would be generated, so removing them is technically a behavioral change.

The extreme usefulness of this cleanup relative to the low likelihood of it causing serious problems in practice makes me tempted to put it in. Maybe the solution is to say that, for each rule, extensions of that rule are only guaranteed to have specificity greater than or equal to the specificity of the rule's selector, rather than the extending selector. Then the only problematic cases with the current implementation are those where the specificity of the original rule goes down, as in:

.testing { color: red; }
body#something { color: blue; }
body { @extend #something; }

In this example, body#something is replaced by just body, so <body class="testing" id="something"> will have red text instead of blue text. Interestingly, this compiles the same way with or without 8f4869e, but perhaps it should instead compile to:

.testing { color: red; }
body#something { color: blue; }
@nex3

This comment has been minimized.

Copy link
Contributor

nex3 commented Mar 19, 2012

Useless note: on browsers that support :not, we could theoretically explicitly manipulate the selector specificity without affecting the behavior. For example, body:not(#-s-does-not-exist) has the same specificity as body#something but matches the same elements as body (assuming no one uses the id="-s-does-not-exist"). Unfortunately, :not isn't widely implemented enough to make this practical.

@chriseppstein

This comment has been minimized.

Copy link
Member Author

chriseppstein commented Mar 19, 2012

I recently changed the blueprint reset to be almost entirely extend based. And I have to say that the output in our test files was really quite lovely. But I can't shake the fact that output specificity will change based on an unrelated delta and maybe to a page you're not even working on. The output change is wholly unpredictable by the user making the change. I find this to be a flatly unacceptable behavior that will place @extend firmly into the avoidance bucket for most developers. I will trade predictability for bloat as default behavior.

Now, that said, I am ok with users opting into this optimization once they have read and understand the caveats. So if we want to introduce a -O1 flag and start down the path of well documented optimizing output, I am ok with this.

@nex3

This comment has been minimized.

Copy link
Contributor

nex3 commented Mar 19, 2012

To be clear: do you think that even the following guarantee is unacceptable? "Given A { @extend B }, all selectors including B that affect A will have specificity between that of the original selector and that of A." It seems reasonable to me that the specificity of the original selector is used; in fact, that's how I'd expect a native implementation of @extend to work. For example, imagine that the following styles worked in native CSS:

body { color: red; }
.black { color: black; }
#pseudobody { @extend body; }

I would expect <div id="pseudobody" class="black"> to have black text, since .black has higher specificity than body, and body is the selector for the rule containing color: blue.

What do you think the output of the following snippet should be?

a#x { color: blue; }
a { @extend #x; }

b { color: blue; }
b#y { @extend b; }
@chriseppstein

This comment has been minimized.

Copy link
Member Author

chriseppstein commented Mar 20, 2012

I can see that being a reasonable design decision for a native implementation of @extend.

But this is not a native implementation. Our implementation is based on selector re-writing and so I expect this needs to work in a predictable fashion that allows the authors to easily reason about the behavior.

To be honest, I'm having a hard time understanding what you mean by:

"Given A { @extend B }, all selectors including B that affect A will have specificity between that of the original selector and that of A."

Regarding the snippet, I would expect the following output:

a#x { color: blue; }
b, b#y { color: blue; }
@chriseppstein

This comment has been minimized.

Copy link
Member Author

chriseppstein commented Mar 20, 2012

Now that I'm not as tired I realized that I got that example wrong. I would expect:

a#x, a { color: blue; }
b, b#y { color: blue; }
@nex3

This comment has been minimized.

Copy link
Contributor

nex3 commented Mar 20, 2012

I don't believe the behavior I'm advocating is particularly unpredictable... at least, not any less predictable than @extend by its nature has to be.

Let me see if I can explain it more clearly.

First of all, let's define the function extend(S, A, B) to be the result of taking a selector S and extending it by replacing all instances of A with A, B and resolving the result a la @extend. Here are some uncontroversial examples:

extend(a, a, b) = a, b
extend(a.foo, a, b) = a.foo, b.foo
extend(c, a, b) = c

Specificity of the Base Selector

Note that so far, it's always the case that extend(S, A, B)[0] = S. However, consider extend(a.foo, .foo, a). One interpretation of this would give the result as a.foo, a. However, a matches a strict superset of the elements that a.foo matches, so another interpretation could give the result as just a. a and a.foo, a are semantically identical except for specificity.

Let's define a new function to talk about this: spec(S) is the specificity of a selector S. So spec(a.foo) = 11, while spec(a) = 1. The nature of CSS means that differences in specificity can lead to practical differences in styling, so to some degree we clearly need to consider specificity as part of the semantics of the selectors we deal with. This is the broad point of this issue.

Let's get back to the example of extend(a.foo, .foo, a). The first selector in the result, extend(a.foo, .foo, a)[0], corresponds to the selector written by the user with the goal of directly styling a set of elements. Allowing the specificity of this selector to change because an @extend was added elsewhere in the stylesheet is semantic change at a distance, which is clearly something we shouldn't allow. Thus, it should be the case that extend(a.foo, .foo, a)[0] = a.foo and in general that spec(extend(S, A, B)[0]) = spec(S). Since any change to S will change its specificity, this shows that it should always be the case that extend(S, A, B)[0] = S.

First Law of Extend: extend(S, A, B)[0] = S

This is not always the behavior in Sass, either in master or in stable; this is clearly a bug that should be fixed.

Specificity of Generated Selectors

Now that we've established what spec(extend(S, A, B)[0]) should look like, it's time to think about what spec(extend(S, A, B)[1]) should look like as well. In order to allow our users to reason about the styling of their page, the specificity of the generated selectors should clearly be as consistent as possible. In an ideal world, if @extend were supported natively in the browser, the specificity would be equivalent to that of the original selector; that is, spec(extend(S, A, B)[1]) = spec(S). However, that's not always possible:

extend(a, a, b.foo) = a, b.foo
  spec(a) < spec(b.foo)
extend(a.foo, a.foo, b) = a.foo, b
  spec(a.foo) > spec(b)

Since consistency is desirable, we might be tempted instead to say that spec(extend(S, A, B)[1]) = spec(B). But that's not always possible either:

extend(a.foo, a, b) = a.foo, b.foo
  spec(b) < spec(b.foo)

There is one guarantee we can make, though: spec(extend(S, A, B)[1]) >= spec(B), since everything in S is either merged with or added to B.

Second Law of Extend: spec(extend(S, A, B)[1]) >= spec(B)

Implications for Optimization

The ultimate goal of this discussion is, of course, that we want to be able to perform certain optimizations on the generated selectors in order to reduce output size, but we don't want these optimizations to break the guarantees we offer our users. Which optimizations do the guarantees outlines above allow us, and which do they forbid?

One optimization that we've been doing for a long time is extend(a.foo, .foo, a) = a, as discussed above. This violates the first law, since a != a.foo.

Another optimization added in 8f4869e is extend(a, a, a.foo) = a. This violates the second law, since spec(a) < spec(a.foo).

However, many of the optimizations added in 8f4869e do still work. For example, extend(.bar a, a, a.foo) = .bar a works because spec(.bar a) = spec(a.foo).

Conclusion

As long as we make the @extend optimizer specificity-aware, we can retain a number of useful optimizations while still providing the same guarantees that they have without any optimizations. That's my proposal: that we support all the optimizations we can while still abiding by the two Laws of Extend outlined above.

@chriseppstein

This comment has been minimized.

Copy link
Member Author

chriseppstein commented Mar 21, 2012

<3 <3 <3

nex3 added a commit that referenced this issue Mar 28, 2012

nex3 added a commit that referenced this issue Mar 28, 2012

Follow the First Law of Extend (see #324).
Some tests still fail, since they rely on the old behavior which violated this
law.
@nex3

This comment has been minimized.

Copy link
Contributor

nex3 commented Mar 28, 2012

Closed by 6bbdec4.

Apparently merge commits can't auto-close issues.

@jslegers

This comment has been minimized.

Copy link

jslegers commented Jul 21, 2013

The current behavior doesn't make any sense.

Consider the following CSS code :

input, table, table input {
  border: 0 solid #ccc; }

input {
  border-width: 1px; }

Basicly, this code does the following :

  • It sets a default border-color and border-style for the 'table' and 'input' elements, but leaves the border-width at 0.
  • It then changes the border-width of the 'input' element to 1px.
  • It resets the border defaults of the 'input' element to the default for all input elements inside a table.

This is a pretty efficient design pattern with an optimal CSS footprint that I use throughout Cascade Framework.

Now, let's convert this code to SCSS:

%border-init {
  border:0 solid #ccc;
}

%border-1px {
  border-width:1px;
}

input {
  @extend %border-init;
  @extend %border-1px;
}

table {
  & {
    @extend %border-init;
  }

  & input {
    @extend %border-init;
  }
}

So far, so good. It generates the expected result.

Let's now abstract away the code for the table element into a separate component :

%border-init {
  border:0 solid #ccc;
}

%border-1px {
  border-width:1px;
}

%component-table {
  & {
    @extend %border-init;
  }

  & input {
    @extend %border-init;
  }
}

input {
  @extend %border-init;
  @extend %border-1px;
}

table {
  @extend %component-table;
}

Now, I no longer get the expected output. Now, Sass decides for me that I don't need the 'table input' in my selector anymore and removes it. So the output becomes this :

input, table {
  border: 0 solid #ccc; }

input {
  border-width: 1px; }

That means the behavior is changed to the following :

  • It sets a default border-color and border-style for the 'table' and 'input' elements, but leaves the border-width at 0.
  • It then changes the border-width of the 'input' element to 1px.
  • The border defaults of the 'input' element to the default for all input elements inside a table are no longer reset. Unlike what I expected, 'input' elements inside a 'table' now have a border.

I find this change of behavior very, very disturbing for the following reasons :

  • When abstracting away the rules of a specific selector to a component by defining a component as a placeholder and extending it, I expect the exact same behavior I would have without the abstraction. Otherwise, this technique becomes unpredictable.
  • It is not up to a preprocessor to assume that any selector containing both 'table' and 'input' should not contain 'table input', considering 'table input' has a higher specificity and may be required to overwrite something defined later on (as examplified here).
  • By ignoring specificity in some cases, you reduce the capabilities of CSS and the usefulness of SCSS.
@cimmanon

This comment has been minimized.

Copy link

cimmanon commented Jul 22, 2013

Your second set of Sass code compiles the way you're expecting for me:

/* line 13, ../sass/test.scss */
table, table input, input {
  border: 0 solid #cccccc;
}

/* line 17, ../sass/test.scss */
input {
  border-width: 1px;
}
@jslegers

This comment has been minimized.

Copy link

jslegers commented Jul 22, 2013

@cimmanon

What version of Sass are you using?

I've tried two different online compilers ( http://sass-lang.com/try.html and http://sassmeister.com/gist/5260832 ) and they both give the same incorrect result.

Also, @nex3 didn't just confirm this behavior but also stated that it was intentional. See #856 .

@cimmanon

This comment has been minimized.

Copy link

cimmanon commented Jul 22, 2013

I assume I was running the current stable version, but I upgraded to the edge version to confirm that it was the same before I commented.

Codepen is also generating the incorrect output.

@nex3

This comment has been minimized.

Copy link
Contributor

nex3 commented May 3, 2014

Just for posterity's sake, I want to mention that the First Law of Extend I described above needs to be revised to accommodate the ability to extend selectors within :not. For example,

:not(.foo) {...}
.bar {@extend .foo}

Because :not specifically declares selectors that the rule doesn't apply to, extending those selectors will necessarily increase the specificity of the base selector. The example above should compile to

:not(.foo):not(.bar) {...}

This new selector has higher specificity than the original. As such, the first law of extend must allow the generated selector to have higher specificity than the original in some cases.

See also #1100.

@nex3 nex3 referenced this issue May 28, 2017

Merged

Refactor extend. #143

nex3 added a commit to sass/language that referenced this issue Apr 14, 2018

Add a spec for extend and its specificity laws
The specificity text was copied/adapted from sass/sass#324.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.