From: Jakub Pawlowicz <contact@jakubpawlowicz.com>
Date: Sun, 18 Jan 2015 19:06:24 +0000 (+0000)
Subject: Fixes #351 - remote `@import`s after content.
X-Git-Url: https://git.ndcode.org/public/gitweb.cgi?a=commitdiff_plain;h=01eff2b816c4e9a70e399f8d2610567baf11ee86;p=clean-css.git

Fixes #351 - remote `@import`s after content.

Changes behavior of importing remote styles after other content,
when minifying without a callback (requirement for remote `@import`s).

So far such `@import`s were kept intact and no warnings were reported.
From now on they are removed and a warning is raised, one per `@import`.

It is due to a fact that all browsers ignore `@import`s when they appear
not at the beginning of a stylesheet.

In case if a remote `@import` appears after other remote or local ones
it won't be dropped as it may be resolved at runtime (in the browser).
---

diff --git a/History.md b/History.md
index e22287da..189755ba 100644
--- a/History.md
+++ b/History.md
@@ -5,6 +5,7 @@
 * Adds better non-adjacent selector merging when body is the same.
 * Fixed issue [#158](https://github.com/GoalSmashers/clean-css/issues/158) - adds body-based selectors reduction.
 * Fixed issue [#182](https://github.com/GoalSmashers/clean-css/issues/182) - removing space after closing brace.
+* Fixed issue [#351](https://github.com/GoalSmashers/clean-css/issues/351) - remote `@import`s after content.
 * Fixed issue [#357](https://github.com/GoalSmashers/clean-css/issues/357) - non-standard but valid URLs.
 * Fixed issue [#416](https://github.com/GoalSmashers/clean-css/issues/416) - accepts hash as `minify` argument.
 
diff --git a/lib/imports/inliner.js b/lib/imports/inliner.js
index 67afa16d..e7babd73 100644
--- a/lib/imports/inliner.js
+++ b/lib/imports/inliner.js
@@ -47,7 +47,6 @@ function importFrom(data, context) {
   var nextEnd = 0;
   var cursor = 0;
   var isComment = commentScanner(data);
-  var afterContent = contentScanner(data);
 
   for (; nextEnd < data.length;) {
     nextStart = nextImportAt(data, cursor);
@@ -66,12 +65,11 @@ function importFrom(data, context) {
       break;
     }
 
-    context.done.push(data.substring(0, nextStart));
+    var noImportPart = data.substring(0, nextStart);
+    context.done.push(noImportPart);
     context.left.unshift([data.substring(nextEnd + 1), context]);
-
-    return afterContent(nextStart) ?
-      processNext(context) :
-      inline(data, nextStart, nextEnd, context);
+    context.afterContent = hasContent(noImportPart);
+    return inline(data, nextStart, nextEnd, context);
   }
 
   // no @import matched in current data
@@ -154,7 +152,7 @@ function commentScanner(data) {
   };
 }
 
-function contentScanner(data) {
+function hasContent(data) {
   var isComment = commentScanner(data);
   var firstContentIdx = -1;
   while (true) {
@@ -163,11 +161,7 @@ function contentScanner(data) {
       break;
   }
 
-  return function(idx) {
-    return firstContentIdx > -1 ?
-      idx > firstContentIdx :
-      false;
-  };
+  return firstContentIdx > -1;
 }
 
 function inline(data, nextStart, nextEnd, context) {
@@ -199,9 +193,16 @@ function inline(data, nextStart, nextEnd, context) {
   var isRemote = context.isRemote || REMOTE_RESOURCE.test(importedFile);
 
   if (context.localOnly && isRemote) {
-    context.warnings.push('Ignoring remote @import declaration of "' + importedFile + '" as no callback given.');
-    restoreImport(importedFile, mediaQuery, context);
+    if (context.afterContent || hasContent(context.done.join('')))
+      context.warnings.push('Ignoring remote @import of "' + importedFile + '" as no callback given.');
+    else
+      restoreImport(importedFile, mediaQuery, context);
+
+    return processNext(context);
+  }
 
+  if (!isRemote && context.afterContent) {
+    context.warnings.push('Ignoring local @import of "' + importedFile + '" as after other CSS content.');
     return processNext(context);
   }
 
diff --git a/test/fixtures/partials/remote.css b/test/fixtures/partials/remote.css
new file mode 100644
index 00000000..63669d17
--- /dev/null
+++ b/test/fixtures/partials/remote.css
@@ -0,0 +1 @@
+@import url(http://jakubpawlowicz.com/styles.css);
diff --git a/test/integration-test.js b/test/integration-test.js
index a850c2dd..a00c1bfc 100644
--- a/test/integration-test.js
+++ b/test/integration-test.js
@@ -1452,6 +1452,18 @@ title']{display:block}",
     'with double underscore': [
       '@import url(test/fixtures/partials/with__double_underscore.css);',
       '.one{color:green}'
+    ],
+    'remote inside local': [
+      '@import url(test/fixtures/partials/remote.css);',
+      '@import url(http://jakubpawlowicz.com/styles.css);'
+    ],
+    'remote inside local after content': [
+      'a{color:red}@import url(test/fixtures/partials/remote.css);',
+      'a{color:red}'
+    ],
+    'remote inside local after imported content': [
+      '@import url(test/fixtures/partials/one.css);@import url(test/fixtures/partials/remote.css);',
+      '.one{color:red}'
     ]
   }, { root: process.cwd() }),
   'malformed but still valid @import': cssContext({
diff --git a/test/module-test.js b/test/module-test.js
index 28ae768d..ab4a8234 100644
--- a/test/module-test.js
+++ b/test/module-test.js
@@ -209,6 +209,64 @@ vows.describe('module tests').addBatch({
       });
     }
   },
+  'external imports and no callback': {
+    'without content': {
+      'topic': function () {
+        return new CleanCSS().minify('@import url(http://jakubpawlowicz.com/styles.css);');
+      },
+      'has right output': function (minified) {
+        assert.equal(minified.styles, '@import url(http://jakubpawlowicz.com/styles.css);');
+      },
+      'has no errors': function (minified) {
+        assert.isEmpty(minified.errors);
+      },
+      'has a warning': function (minified) {
+        assert.deepEqual(minified.warnings, []);
+      }
+    },
+    'after content': {
+      'topic': function () {
+        return new CleanCSS().minify('a{color:red}@import url(http://jakubpawlowicz.com/styles.css);');
+      },
+      'has right output': function (minified) {
+        assert.equal(minified.styles, 'a{color:red}');
+      },
+      'has no errors': function (minified) {
+        assert.isEmpty(minified.errors);
+      },
+      'has a warning': function (minified) {
+        assert.deepEqual(minified.warnings, ['Ignoring remote @import of "http://jakubpawlowicz.com/styles.css" as no callback given.']);
+      }
+    },
+    'after local import': {
+      'topic': function () {
+        return new CleanCSS().minify('@import url(test/fixtures/partials/one.css);@import url(http://jakubpawlowicz.com/styles.css);');
+      },
+      'has right output': function (minified) {
+        assert.equal(minified.styles, '.one{color:red}');
+      },
+      'has no errors': function (minified) {
+        assert.isEmpty(minified.errors);
+      },
+      'has a warning': function (minified) {
+        assert.deepEqual(minified.warnings, ['Ignoring remote @import of "http://jakubpawlowicz.com/styles.css" as no callback given.']);
+      }
+    },
+    'after remote import': {
+      'topic': function () {
+        return new CleanCSS().minify('@import url(http://jakubpawlowicz.com/reset.css);@import url(http://jakubpawlowicz.com/styles.css);');
+      },
+      'has right output': function (minified) {
+        assert.equal(minified.styles, '@import url(http://jakubpawlowicz.com/reset.css);@import url(http://jakubpawlowicz.com/styles.css);');
+      },
+      'has no errors': function (minified) {
+        assert.isEmpty(minified.errors);
+      },
+      'has a warning': function (minified) {
+        assert.deepEqual(minified.warnings, []);
+      }
+    }
+  },
   'buffer passed in': {
     'topic': function() {
       return new CleanCSS().minify(new Buffer('@import url(test/fixtures/partials/one.css);'));
diff --git a/test/protocol-imports-test.js b/test/protocol-imports-test.js
index 3fcb62e9..5d5ba764 100644
--- a/test/protocol-imports-test.js
+++ b/test/protocol-imports-test.js
@@ -403,7 +403,7 @@ vows.describe('protocol imports').addBatch({
   },
   'of a remote resource mixed with local ones but no callback': {
     topic: function() {
-      var source = '@import url(http://127.0.0.1/remote.css);@import url(test/fixtures/partials/one.css);';
+      var source = '@import url(test/fixtures/partials/one.css);@import url(http://127.0.0.1/remote.css);';
       this.reqMocks = nock('http://127.0.0.1')
         .get('/remote.css')
         .reply(200, 'div{padding:0}');
@@ -418,7 +418,7 @@ vows.describe('protocol imports').addBatch({
       assert.match(minified.warnings[0], /no callback given/);
     },
     'should process @import': function (error, minified) {
-      assert.equal(minified.styles, '@import url(http://127.0.0.1/remote.css);.one{color:red}');
+      assert.equal(minified.styles, '.one{color:red}');
     },
     teardown: function() {
       assert.isFalse(this.reqMocks.isDone());