Incorrectly handling of CSS Imports in @media

[xzyfer]

Discovered whilst working on #2234. Related to #2096.

@media all and (min-width: 100px) {
  @import "https://example.org"
}

LibSass

Ruby Sass

@media all and (min-width: 100px) {
  @import "https://example.org"; }

version info:

$ sassc --version
sassc: 3.3.3-3-ge054-dirty
libsass: 3.4.0-RC1
sass2scss: 1.1.0

Spec https://github.com/sass/sass-spec/pull/983

---

This actually expose two bugs. Firstly, we don't consider @imports in the isPrintable check for media blocks. Secondly, since we don't differentiate between Sass Imports and CSS Imports (#2096), fixing isPrintable causes the import node to hoisted to the top of the file.

[xzyfer]

The second issue can be seen when there is content in the media block

@media all and (min-width: 100px) {
  a { b: c }
  @import "https://example.org";
}

Ruby Sass

@media all and (min-width: 100px) {
  a {
    b: c; }

  @import "https://example.org"; }

LibSass

@import "https://example.org";
@media all and (min-width: 100px) {
  a {
    b: c; } }

[mgreter]

Since when did ruby sass start to support CSS3 nested imports? The empty output is just a "isPrintable" problem, but IMO sofar imports have always been "bubbled" up to the top of the document, as CSS2 requires!?

The @import CSS at-rule is used to import style rules from other style sheets. These rules must precede all other types of rules, except @charset rules; as it is not a nested statement, @import cannot be used inside conditional group at-rules.

from https://developer.mozilla.org/en-US/docs/Web/CSS/@import

[mgreter]

@@ -244,6 +244,9 @@ namespace Sass {
         if (dynamic_cast<Has_Block*>(stm)) {
           stm->perform(this);
         }
+        if (dynamic_cast<Import*>(stm)) {
+            stm->perform(this);
+        }
       }
       return;
     }

@@ -606,6 +606,7 @@ namespace Sass {
       incs_(std::vector<Include>()),
       media_queries_(0)
     { statement_type(IMPORT); }
+    virtual bool is_invisible() const { return false; }
     std::vector<Expression*>& urls() { return urls_; }
     std::vector<Include>& incs() { return incs_; }
     ATTACH_OPERATIONS()

results in

@import url("https://example.org");

https://github.com/sass/libsass/blob/master/src/output.cpp#L33-L36

[mgreter]

Even a simple

foo { 
  @import url("https://example.org/asdasd.css");
}

yields a different result in ruby sass and libsass!?

[mgreter]

I'm not even sure the output of ruby sass is valid CSS3? https://drafts.csswg.org/css-cascade-3/#at-ruledef-import

Any @import rules must precede all other at-rules and style rules in a style sheet (besides @charset, which must be the first thing in the style sheet if it exists), or else the @import rule is invalid.

From a quick glance it seems imports do support media queries, but still only on top and directly attached/appended to the import statement itself. Therefore it might be possible to extract these rules from parent media query at-rules. But I still read the specs the way that they must be on top of the document.

//CC @chriseppstein @nex3

[mgreter]

Without too much thinking I would expect the following:

@media all and (min-width: 100px) {
  @media all and (max-width: 800px) {
    @import url("test.css");
  }
}

Should become:

@import url("test.css") all and (min-width: 100px) and (max-width: 800px);

All other nesting cases (i.e. in a selector) should yield an error or be ignored (as the specs states).

Current ruby sass output:

@media all and (min-width: 100px) and (max-width: 800px) {
  @import url("test.css");
}

Me is confused ...

[nex3]

This just isn't a use-case we've considered—Ruby Sass is falling back on the default behavior of allowing everything in @media that's allowed at the top-level. We should probably just error out.

[mgreter]

I agree that we should error out on invalid cases (as per CSS specs). Can you already give the error message libsass should print in this case? I think it should be easy to implement the necessary checks on the libsass side.

I guess the case where a valid import in root scope(s) of media queries is a new feature. I would also argue that hoisting such imports may could lead to unexpectd behavior (specificity of selectors may not match to the intention, if the actual import is done before all other css styles). So I'm not sure if the example I posted in me previous comment makes much sense to implement after all!?

[nex3]

I guess the error message would be something like "@ import is not allowed within @ media"?

[mgreter]

I would agree (the error msg seems appropriate)! But I'm not sure if you mean that we would dissalow any @import inside @media. As I pointed out, we could hoist those to the top, but it could change expected behavior if any other ruleset preceed the import that would be hoisted! We have fallowed ruby sass in any decission so far, so just make up the rules and we'll follow! IMO erroring out would make the most sense, but as stated, we'll implement it as ruby/dart sass does!

[nex3]

Yes, all @import statements in @media queries should be disallowed, at least for now. You can feel free to disallow them in LibSass only for now; since they produce invalid CSS anyway, there's more flexibility for cross-implementation differences.