Fixes #543 - better "comment in body" handling.

So instead of keeping comments in a property list we get them out
of every property.

It is a bit tricky as comments escaped at the beginning need to be
tracked and saved, but ones from the middle or end of the value has
to be saved for later (see `innerProperties`) and tracked after the
property even if they are moved before it.

See tests for a better picture.

diff --git a/History.md b/History.md
index 611b781..ba02a06 100644 (file)
--- a/History.md
+++ b/History.md
@@ -12,6 +12,7 @@
 [3.2.5 / 2015-xx-xx](https://github.com/jakubpawlowicz/clean-css/compare/v3.2.4...3.2)
 ==================
 
+* Fixed issue [#543](https://github.com/jakubpawlowicz/clean-css/issues/543) - better "comment in body" handling.
 * Fixed issue [#548](https://github.com/jakubpawlowicz/clean-css/issues/548) - regression in font minifying.
 
 [3.2.4 / 2015-04-24](https://github.com/jakubpawlowicz/clean-css/compare/v3.2.3...v3.2.4)

diff --git a/lib/tokenizer/extract-properties.js b/lib/tokenizer/extract-properties.js
index 10ea3d8..a0880d4 100644 (file)
--- a/lib/tokenizer/extract-properties.js
+++ b/lib/tokenizer/extract-properties.js
@@ -7,16 +7,48 @@ function selectorName(value) {
   return value[0];
 }
 
+function noop() {}
+
+function withoutComments(string, into, heading, context) {
+  var matcher = heading ? /^__ESCAPED_COMMENT_/ : /__ESCAPED_COMMENT_/;
+  var track = heading ? context.track : noop; // don't track when comment not in a heading as we do it later in `trackComments`
+
+  while (matcher.test(string)) {
+    var startOfComment = string.indexOf('__');
+    var endOfComment = string.indexOf('__', startOfComment + 1) + 2;
+    var comment = string.substring(startOfComment, endOfComment);
+    string = string.substring(0, startOfComment) + string.substring(endOfComment);
+
+    track(comment);
+    into.push(comment);
+  }
+
+  return string;
+}
+
+function withoutHeadingComments(string, into, context) {
+  return withoutComments(string, into, true, context);
+}
+
+function withoutInnerComments(string, into, context) {
+  return withoutComments(string, into, false, context);
+}
+
+function trackComments(comments, into, context) {
+  for (var i = 0, l = comments.length; i < l; i++) {
+    context.track(comments[i]);
+    into.push(comments[i]);
+  }
+}
+
 function extractProperties(string, selectors, context) {
   var list = [];
+  var innerComments = [];
   var splitter = new Splitter(/[ ,\/]/);
 
   if (typeof string != 'string')
     return [];
 
-  if (string.indexOf('__ESCAPED_COMMENT_') > -1)
-    string = string.replace(/(__ESCAPED_COMMENT_(SPECIAL_)?CLEAN_CSS[^_]+?__)/g, ';$1;');
-
   if (string.indexOf(')') > -1)
     string = string.replace(/\)([^\s_;:,\)])/g, context.sourceMap ? ') __ESCAPED_COMMENT_CLEAN_CSS(0,-1)__ $1' : ') $1');
 
@@ -32,7 +64,7 @@ function extractProperties(string, selectors, context) {
     if (firstColonAt == -1) {
       context.track(candidate);
       if (candidate.indexOf('__ESCAPED_COMMENT_SPECIAL') > -1)
-        list.push(candidate);
+        list.push(candidate.trim());
       continue;
     }
 
@@ -43,9 +75,20 @@ function extractProperties(string, selectors, context) {
 
     var body = [];
     var name = candidate.substring(0, firstColonAt);
+
+    innerComments = [];
+
+    if (name.indexOf('__ESCAPED_COMMENT') > -1)
+      name = withoutHeadingComments(name, list, context);
+
+    if (name.indexOf('__ESCAPED_COMMENT') > -1)
+      name = withoutInnerComments(name, innerComments, context);
+
     body.push([name.trim()].concat(context.track(name, true)));
     context.track(':');
 
+    trackComments(innerComments, list, context);
+
     var values = splitter.split(candidate.substring(firstColonAt + 1), true);
 
     if (values.length == 1 && values[0] === '') {
@@ -71,6 +114,19 @@ function extractProperties(string, selectors, context) {
         continue;
       }
 
+      innerComments = [];
+
+      if (trimmed.indexOf('__ESCAPED_COMMENT') > -1)
+        trimmed = withoutHeadingComments(trimmed, list, context);
+
+      if (trimmed.indexOf('__ESCAPED_COMMENT') > -1)
+        trimmed = withoutInnerComments(trimmed, innerComments, context);
+
+      if (trimmed.length === 0) {
+        trackComments(innerComments, list, context);
+        continue;
+      }
+
       var pos = body.length - 1;
       if (trimmed == 'important' && body[pos][0] == '!') {
         context.track(trimmed);
@@ -87,6 +143,8 @@ function extractProperties(string, selectors, context) {
 
       body.push([trimmed].concat(context.track(value, true)));
 
+      trackComments(innerComments, list, context);
+
       if (endsWithNonSpaceSeparator) {
         body.push([lastCharacter]);
         context.track(lastCharacter);

diff --git a/test/fixtures/issue-543-min.css b/test/fixtures/issue-543-min.css
new file mode 100644 (file)
index 0000000..580fa2c
--- /dev/null
+++ b/test/fixtures/issue-543-min.css
@@ -0,0 +1,5 @@
+@font-face{src:url(../font/myfont.eot);/*! IE9 */
+/*! IE6-IE8 */
+/*! chrome, firefox */
+/*! chrome, firefox, opera, Safari, Android, iOS 4.2+ */
+src:url(../font/myfont.eot?#iefix)format("embedded-opentype"),url(../font/myfont.woff)format("woff"),url(../font/myfont.ttf)format("truetype"),url(../font/myfont.svg#myfont)format("svg");font-style:normal;font-weight:400}
\ No newline at end of file

diff --git a/test/fixtures/issue-543.css b/test/fixtures/issue-543.css
new file mode 100644 (file)
index 0000000..b5e730f
--- /dev/null
+++ b/test/fixtures/issue-543.css
@@ -0,0 +1,9 @@
+@font-face {
+  src: url("../font/myfont.eot"); /*! IE9 */
+  src: url("../font/myfont.eot?#iefix") format("embedded-opentype"), /*! IE6-IE8 */
+  url("../font/myfont.woff") format("woff"), /*! chrome, firefox */
+  url("../font/myfont.ttf") format("truetype"), /*! chrome, firefox, opera, Safari, Android, iOS 4.2+ */
+  url("../font/myfont.svg#myfont") format("svg"); /* iOS 4.1- */
+  font-style: normal;
+  font-weight: normal;
+}

diff --git a/test/selectors/optimizers/simple-test.js b/test/selectors/optimizers/simple-test.js
index 2ff3663..c6314f4 100644 (file)
--- a/test/selectors/optimizers/simple-test.js
+++ b/test/selectors/optimizers/simple-test.js
@@ -580,7 +580,7 @@ vows.describe(SimpleOptimizer)
     propertyContext('comments', {
       'comment': [
         'a{__ESCAPED_COMMENT_SPECIAL_CLEAN_CSS0__color:red__ESCAPED_COMMENT_SPECIAL_CLEAN_CSS1__}',
-        ['__ESCAPED_COMMENT_SPECIAL_CLEAN_CSS0__', ['color', 'red'], '__ESCAPED_COMMENT_SPECIAL_CLEAN_CSS1__']
+        ['__ESCAPED_COMMENT_SPECIAL_CLEAN_CSS0__', '__ESCAPED_COMMENT_SPECIAL_CLEAN_CSS1__', ['color', 'red']]
       ]
     })
   )

diff --git a/test/tokenizer/tokenizer-test.js b/test/tokenizer/tokenizer-test.js
index 7a69f55..d0721fa 100644 (file)
--- a/test/tokenizer/tokenizer-test.js
+++ b/test/tokenizer/tokenizer-test.js
@@ -137,7 +137,31 @@ vows.describe(tokenize)
       'two properties wrapped between comments': [
         'div{__ESCAPED_COMMENT_SPECIAL_CLEAN_CSS0__color:red__ESCAPED_COMMENT_SPECIAL_CLEAN_CSS1__}',
         [
-          ['selector', [['div']], ['__ESCAPED_COMMENT_SPECIAL_CLEAN_CSS0__', [['color'], ['red']], '__ESCAPED_COMMENT_SPECIAL_CLEAN_CSS1__']]
+          ['selector', [['div']], ['__ESCAPED_COMMENT_SPECIAL_CLEAN_CSS0__', '__ESCAPED_COMMENT_SPECIAL_CLEAN_CSS1__', [['color'], ['red']]]]
+        ]
+      ],
+      'multiple values wrapped between comments #1': [
+        'div{background:__ESCAPED_URL_CLEAN_CSS0__,__ESCAPED_COMMENT_CLEAN_CSS0__red}',
+        [
+          ['selector', [['div']], ['__ESCAPED_COMMENT_CLEAN_CSS0__', [['background'], ['__ESCAPED_URL_CLEAN_CSS0__'], [','], ['red']]]]
+        ]
+      ],
+      'multiple values wrapped between comments #2': [
+        'div{background:__ESCAPED_URL_CLEAN_CSS0__,red__ESCAPED_COMMENT_CLEAN_CSS0__}',
+        [
+          ['selector', [['div']], ['__ESCAPED_COMMENT_CLEAN_CSS0__', [['background'], ['__ESCAPED_URL_CLEAN_CSS0__'], [','], ['red']]]]
+        ]
+      ],
+      'multiple values wrapped between comments #3': [
+        'div{background:__ESCAPED_URL_CLEAN_CSS0__,rgba(0,0,0,__ESCAPED_COMMENT_CLEAN_CSS0__0.1)}',
+        [
+          ['selector', [['div']], ['__ESCAPED_COMMENT_CLEAN_CSS0__', [['background'], ['__ESCAPED_URL_CLEAN_CSS0__'], [','], ['rgba(0,0,0,0.1)']]]]
+        ]
+      ],
+      'multiple values wrapped between comments #4': [
+        'div{background:__ESCAPED_URL_CLEAN_CSS0__,rgba(0,0,0,__ESCAPED_COMMENT_CLEAN_CSS0__0.1)}',
+        [
+          ['selector', [['div']], ['__ESCAPED_COMMENT_CLEAN_CSS0__', [['background'], ['__ESCAPED_URL_CLEAN_CSS0__'], [','], ['rgba(0,0,0,0.1)']]]]
         ]
       ],
       'pseudoselector after an argument one': [