From 05aac31b6703c9fec8b687f770bf0f70b90ba925 Mon Sep 17 00:00:00 2001 From: Jakub Pawlowicz Date: Sat, 25 Apr 2015 09:15:31 +0100 Subject: [PATCH] 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. --- History.md | 1 + lib/tokenizer/extract-properties.js | 66 ++++++++++++++++++++++-- test/fixtures/issue-543-min.css | 5 ++ test/fixtures/issue-543.css | 9 ++++ test/selectors/optimizers/simple-test.js | 2 +- test/tokenizer/tokenizer-test.js | 26 +++++++++- 6 files changed, 103 insertions(+), 6 deletions(-) create mode 100644 test/fixtures/issue-543-min.css create mode 100644 test/fixtures/issue-543.css diff --git a/History.md b/History.md index 611b7818..ba02a06c 100644 --- 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 10ea3d80..a0880d41 100644 --- 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 index 00000000..580fa2c0 --- /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 index 00000000..b5e730f2 --- /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 2ff3663b..c6314f48 100644 --- 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 7a69f55b..d0721fab 100644 --- 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': [ -- 2.34.1