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

LibSass supports cross-media @extend #712

Closed
HugoGiraudel opened this Issue Dec 10, 2014 · 23 comments

Comments

Projects
None yet
8 participants
@HugoGiraudel

HugoGiraudel commented Dec 10, 2014

I feel kind of dirty reporting this, because this is not actually a bug per se... It turns out LibSass does support something Ruby Sass doesn't. Moreover, this feature has been asked for years in Sass, so I'm quite confused about the way to go here... Should this be removed? I'll leave you the only judge of this.

Test:

.selector {
  content: selector;
}

@media print {
  .other-selector {
    @extend .selector;
  }
}

Expect:

You may not @extend an outer selector from within @media.
You may only @extend selectors within the same directive.
From "@extend .selector" on line 7.

Result:

.selector,
.other-selector {
    content: selector;
}

Ref: http://sass-compatibility.github.io/#cross_media_extend.

@sturobson

This comment has been minimized.

Show comment
Hide comment
@sturobson

sturobson Dec 10, 2014

Wasn't this a feature that was available in Sass but was neutered in 3.2?

Anyways..... ssssshhhhhhhhhh

sturobson commented Dec 10, 2014

Wasn't this a feature that was available in Sass but was neutered in 3.2?

Anyways..... ssssshhhhhhhhhh

@davidkpiano

This comment has been minimized.

Show comment
Hide comment
@davidkpiano

davidkpiano Dec 10, 2014

Shouldn't this be a bug in LibSass? The expected behavior in @HugoGiraudel 's test was that .other-selector would extend the .selector ruleset only in @media print; whereas it's applied outside the media query in LibSass (unexpected behavior).

The lack of this "feature" seems like a necessary evil to prevent unexpected behavior like that. If I wanted to extend the .selector ruleset regardless of media query and then have custom rules inside a media query, I would just do this (which LibSass doesn't support yet):

.selector {
  content: selector;
}

@media print {
  .other-selector {
    @at-root (without: media) {
      @extend .selector;
    }
    background: red; // custom rule inside media query
  }
}

davidkpiano commented Dec 10, 2014

Shouldn't this be a bug in LibSass? The expected behavior in @HugoGiraudel 's test was that .other-selector would extend the .selector ruleset only in @media print; whereas it's applied outside the media query in LibSass (unexpected behavior).

The lack of this "feature" seems like a necessary evil to prevent unexpected behavior like that. If I wanted to extend the .selector ruleset regardless of media query and then have custom rules inside a media query, I would just do this (which LibSass doesn't support yet):

.selector {
  content: selector;
}

@media print {
  .other-selector {
    @at-root (without: media) {
      @extend .selector;
    }
    background: red; // custom rule inside media query
  }
}
@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

HugoGiraudel Dec 11, 2014

What is the status on this? Do you need me to write a spec?

HugoGiraudel commented Dec 11, 2014

What is the status on this? Do you need me to write a spec?

@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

HugoGiraudel Jan 12, 2015

Maintainers, what is the current status on this?

HugoGiraudel commented Jan 12, 2015

Maintainers, what is the current status on this?

@chriseppstein

This comment has been minimized.

Show comment
Hide comment
@chriseppstein

chriseppstein Jan 12, 2015

Member

This is a bug in libsass. Sass does have a feature planned to support cross media extends but it does not have the output you see here. The correct output requires Sass to keep the @media context when the extend is performed. The feature enhancement is being tracked here: sass/sass#1050

Member

chriseppstein commented Jan 12, 2015

This is a bug in libsass. Sass does have a feature planned to support cross media extends but it does not have the output you see here. The correct output requires Sass to keep the @media context when the extend is performed. The feature enhancement is being tracked here: sass/sass#1050

@chriseppstein

This comment has been minimized.

Show comment
Hide comment
@chriseppstein

chriseppstein Jan 12, 2015

Member

@HugoGiraudel The information at http://sass-compatibility.github.io/#cross_media_extend is not accurate. LibSass doesn't "get this right". It gets it very wrong just like sass got it very wrong back when we allowed it.

Member

chriseppstein commented Jan 12, 2015

@HugoGiraudel The information at http://sass-compatibility.github.io/#cross_media_extend is not accurate. LibSass doesn't "get this right". It gets it very wrong just like sass got it very wrong back when we allowed it.

@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

HugoGiraudel Jan 12, 2015

Sass Compatibility has no "get it right" or "get it wrong". It has support or no support.

HugoGiraudel commented Jan 12, 2015

Sass Compatibility has no "get it right" or "get it wrong". It has support or no support.

@chriseppstein

This comment has been minimized.

Show comment
Hide comment
@chriseppstein

chriseppstein Jan 13, 2015

Member

The presentation implies there is a feature that libsass has and other implementations don't. It's impossible for Ruby sass 3.4 anything other than 100% because it is the reference implementation.

Hunt & pecked on my iPhone... Sorry if it's brief!

On Jan 12, 2015, at 3:01 PM, Hugo Giraudel notifications@github.com wrote:

Sass Compatibility has no "get it right" or "get it wrong". It has support or no support.


Reply to this email directly or view it on GitHub.

Member

chriseppstein commented Jan 13, 2015

The presentation implies there is a feature that libsass has and other implementations don't. It's impossible for Ruby sass 3.4 anything other than 100% because it is the reference implementation.

Hunt & pecked on my iPhone... Sorry if it's brief!

On Jan 12, 2015, at 3:01 PM, Hugo Giraudel notifications@github.com wrote:

Sass Compatibility has no "get it right" or "get it wrong". It has support or no support.


Reply to this email directly or view it on GitHub.

@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

HugoGiraudel Jan 13, 2015

I could add a note about how LibSass gets it wrong in the description if you will but I won't be able to do much more. Sass Compatibility intends to list inconsistencies between Sass engines. It is an inconsistency, no matter if both are not supporting what you deem is right.

HugoGiraudel commented Jan 13, 2015

I could add a note about how LibSass gets it wrong in the description if you will but I won't be able to do much more. Sass Compatibility intends to list inconsistencies between Sass engines. It is an inconsistency, no matter if both are not supporting what you deem is right.

@chriseppstein

This comment has been minimized.

Show comment
Hide comment
@chriseppstein

chriseppstein Jan 13, 2015

Member

@HugoGiraudel Then the sense of the discrepency is wrong. The feature should be "forbids @extend in @media" and Sass should have it checked and libsass should not have it checked.

Member

chriseppstein commented Jan 13, 2015

@HugoGiraudel Then the sense of the discrepency is wrong. The feature should be "forbids @extend in @media" and Sass should have it checked and libsass should not have it checked.

@chriseppstein

This comment has been minimized.

Show comment
Hide comment
@chriseppstein

chriseppstein Jan 13, 2015

Member

@HugoGiraudel Anyways, I really like the presentation of your compatibility guide. It would be awesome if we could data-drive it from sass-spec. are you open to working with us on that?

Member

chriseppstein commented Jan 13, 2015

@HugoGiraudel Anyways, I really like the presentation of your compatibility guide. It would be awesome if we could data-drive it from sass-spec. are you open to working with us on that?

@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

HugoGiraudel Jan 13, 2015

The feature should be "forbids @extend in @media" and Sass should have it checked and libsass should not have it checked.

That seems reasonable.

we could data-drive it from sass-spec

It already is, except for the few tests that do not exist on sass-spec. I have been working with @xzyfer on this for several weeks now. As a result, I have written a lot of extra tests for sass-spec. :)

HugoGiraudel commented Jan 13, 2015

The feature should be "forbids @extend in @media" and Sass should have it checked and libsass should not have it checked.

That seems reasonable.

we could data-drive it from sass-spec

It already is, except for the few tests that do not exist on sass-spec. I have been working with @xzyfer on this for several weeks now. As a result, I have written a lot of extra tests for sass-spec. :)

@chriseppstein

This comment has been minimized.

Show comment
Hide comment
@chriseppstein

chriseppstein Jan 13, 2015

Member

It already is

Excellent. Where is the mapping of tests to features?

Member

chriseppstein commented Jan 13, 2015

It already is

Excellent. Where is the mapping of tests to features?

@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

HugoGiraudel Jan 13, 2015

Hardly the place for it I suppose. Where should I reach you Chris?

HugoGiraudel commented Jan 13, 2015

Hardly the place for it I suppose. Where should I reach you Chris?

@matthewpavkov

This comment has been minimized.

Show comment
Hide comment
@matthewpavkov

matthewpavkov Feb 2, 2015

So, libsass does not support @extend within @media, and should not be used, correct?

matthewpavkov commented Feb 2, 2015

So, libsass does not support @extend within @media, and should not be used, correct?

@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment
@HugoGiraudel

HugoGiraudel Feb 2, 2015

LibSass does support a buggy version of it, hence should not be used.

HugoGiraudel commented Feb 2, 2015

LibSass does support a buggy version of it, hence should not be used.

@xzyfer

This comment has been minimized.

Show comment
Hide comment
@xzyfer

xzyfer Feb 16, 2015

Contributor

@HugoGiraudel could you please create a spec representing the correct behaviour?

Contributor

xzyfer commented Feb 16, 2015

@HugoGiraudel could you please create a spec representing the correct behaviour?

@HugoGiraudel

This comment has been minimized.

Show comment
Hide comment

HugoGiraudel commented Feb 16, 2015

@mgreter mgreter added this to the 3.2 milestone Mar 9, 2015

@mgreter mgreter self-assigned this Mar 9, 2015

@mgreter mgreter closed this in #927 Mar 9, 2015

@xzyfer

This comment has been minimized.

Show comment
Hide comment
@xzyfer

xzyfer Mar 9, 2015

Contributor

@mgreter have you confirmed the (unmerged) spec sass/sass-spec#261 passes?

Contributor

xzyfer commented Mar 9, 2015

@mgreter have you confirmed the (unmerged) spec sass/sass-spec#261 passes?

@xzyfer

This comment has been minimized.

Show comment
Hide comment
@xzyfer

xzyfer Mar 9, 2015

Contributor

Nevermind, that spec requires capturing errors.

Contributor

xzyfer commented Mar 9, 2015

Nevermind, that spec requires capturing errors.

@saper

This comment has been minimized.

Show comment
Hide comment
@saper

saper Sep 20, 2015

Member

I can confirm that for the sass/sass-spec#261 testcase libsass still works, i.e. produces output

.foo, .bar {
  content: 'foo'; }

without reporting an error.

This was:

sassc: 3.3.0-beta1
libsass: 3.3.0-beta2-1-g1efd
sass2scss: 1.0.3
Member

saper commented Sep 20, 2015

I can confirm that for the sass/sass-spec#261 testcase libsass still works, i.e. produces output

.foo, .bar {
  content: 'foo'; }

without reporting an error.

This was:

sassc: 3.3.0-beta1
libsass: 3.3.0-beta2-1-g1efd
sass2scss: 1.0.3
@xzyfer

This comment has been minimized.

Show comment
Hide comment
@xzyfer

xzyfer Sep 21, 2015

Contributor

Looks like a regression in 3.3. This was fixed in 3.2.0.

Error: You may not @extend an outer selector from within @media.
       You may only @extend selectors within the same directive.
       From "@extend .foo" on line 35 of test.scss
        on line 29 of test.scss
Contributor

xzyfer commented Sep 21, 2015

Looks like a regression in 3.3. This was fixed in 3.2.0.

Error: You may not @extend an outer selector from within @media.
       You may only @extend selectors within the same directive.
       From "@extend .foo" on line 35 of test.scss
        on line 29 of test.scss
@xzyfer

This comment has been minimized.

Show comment
Hide comment
@xzyfer

xzyfer Sep 21, 2015

Contributor

Tracking the regression in #1562

Contributor

xzyfer commented Sep 21, 2015

Tracking the regression in #1562

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment