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

wrong :before position #996

Closed
ahorek opened this issue Apr 30, 2020 · 8 comments
Closed

wrong :before position #996

ahorek opened this issue Apr 30, 2020 · 8 comments

Comments

@ahorek
Copy link

ahorek commented Apr 30, 2020

there's a behavioral difference in a new libsass version and I found out that dart-sass behaves the same way. Is it a bug?

links with sass/libsass#3084

input.scss

.red-icon {
  color: #e50026;
}

a.nested:before {
  &.error {
    @extend .red-icon;
  }
}

Actual results

.red-icon, a.nested:before.error {
  color: #e50026; }

in this case <div class="red-icon">text</div> isn't red.

Expected result

.red-icon, a.nested.error:before {
  color: #e50026; }
@Awjin
Copy link
Contributor

Awjin commented May 1, 2020

The parent selector & is working as intended. It does not make assumptions about the selector ordering of its parent.

With proper nesting, we can get the correct style:

a.nested {
  &:before {}

  &.error:before {}
}

@Awjin Awjin closed this as completed May 1, 2020
@ahorek
Copy link
Author

ahorek commented May 1, 2020

sass/libsass#3086

Hi, this is unfortunate. I know there's a workaround, but libsass made this change already, see related PR. So what should we do? Could such attempt become a warning? I would be happy with both approches, but this change caused a silent regression in my codebase ;-(

@nex3
Copy link
Contributor

nex3 commented May 2, 2020

LibSass shouldn't change this in a way that's incompatible with Dart Sass; I've asked LibSass to roll it back in sass/libsass#3086 (comment).

If you want to propose a language change around this behavior, you can follow the language proposal process to do so.

@mgreter
Copy link
Contributor

mgreter commented May 4, 2020

You are all aware that this basically means the following is not supported by sass?

A.foo:hover {
  &.other {
    foo: bar;
  }
}

I think this should at least produce a warning. And I still fail to see why we can't just ensure
correct output ordering. IMHO it seems the right thing to do to me (with or without warning) ...

@mgreter
Copy link
Contributor

mgreter commented May 4, 2020

Btw. I just checked ruby sass 3.7.4

For input.scss

.red-icon {
  color: #e50026;
}
a.nested:before {
  &.error {
    @extend .red-icon;
  }
}

It did produce correct css

.red-icon, a.nested.error:before {
  color: #e50026; }

While for the regular parentization, it also gives invalid css

A.foo:hover.other {
  foo: bar; }

So I believe the original bug report by @ahorek is valid!?

@nex3
Copy link
Contributor

nex3 commented May 13, 2020

Again, a full discussion of whether we want to change the existing behavior should happen in a language change proposal.

For some additional context: Sass has always had the philosophy of knowing as little about the underlying CSS semantics as possible while still making the transformations the user asks for. This is crucial for allowing us to avoid spending massive amounts of time updating Sass implementations every time CSS changes. It also means that Sass allows users to write syntactically-valid but semantically-invalid CSS. Other tools like linters are better all around at verifying semantic correctness, so it doesn't make sense for Sass to get into that game.

As to this particular case: & is not a particularly "smart" feature. It doesn't ever know anything about selector semantics beyond how to resolve nested selector lists; it just slams selectors together. Adding semantic intelligence just for this case would break that invariant, and thus be a confusing functional inconsistency. There is a long-standing feature request (sass/sass#490) to make parent selector resolution holistically more intelligent by piggy-backing on @extend's logic, which would cover this use-case as well as others. That would by valuable, but it would require a considerable amount of design work and analysis of which cases it would break, and it's not the sort of thing we should just add piecemeal ahead of time.

@mgreter
Copy link
Contributor

mgreter commented May 16, 2020

Thx for all the clarification, but why don't you see the original report as a valid report, since ruby sass did indeed produce correct output, but dart sass doesn't? I can somewhat see the reason to parse invalid css selector and output them as such, instead of fixing the order. But why is the original report not treated seriously?

@nex3
Copy link
Contributor

nex3 commented May 16, 2020

Thx for all the clarification, but why don't you see the original report as a valid report

@ahorek's report? As @Awjin said, this is how the specified behavior is expected to work.

since ruby sass did indeed produce correct output, but dart sass doesn't

Ruby Sass is no longer the reference implementation, and has no inherent bearing on the specified behavior of Sass.

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

No branches or pull requests

4 participants