From: Jakub Pawlowicz Date: Wed, 4 Jan 2017 17:43:50 +0000 (+0100) Subject: Fixes #806 - skip optimizing variable properties. X-Git-Url: https://git.ndcode.org/public/gitweb.cgi?a=commitdiff_plain;h=b013ab9ee8d29edbb602c8220ae419f2f1d5a2c2;p=clean-css.git Fixes #806 - skip optimizing variable properties. Why: * So far it's impossible to find out real property values so breaking up shorthands into components doesn't work. * In theory one could optimize cases where all component values are given and one of them is a variable, but it's an additional complexity probably not worth the hassle. --- diff --git a/History.md b/History.md index 90ea48eb..61881d71 100644 --- a/History.md +++ b/History.md @@ -25,6 +25,7 @@ * Fixed issue [#791](https://github.com/jakubpawlowicz/clean-css/issues/791) - resolves imports in-memory if possible. * Fixed issue [#796](https://github.com/jakubpawlowicz/clean-css/issues/796) - semantic merging for `@media` blocks. * Fixed issue [#801](https://github.com/jakubpawlowicz/clean-css/issues/801) - smarter `@import` inlining. +* Fixed issue [#806](https://github.com/jakubpawlowicz/clean-css/issues/806) - skip optimizing variable properties. * Fixed issue [#817](https://github.com/jakubpawlowicz/clean-css/issues/817) - makes `off` disable rounding. * Fixed issue [#818](https://github.com/jakubpawlowicz/clean-css/issues/818) - disables `px` rounding by default. * Fixed issue [#829](https://github.com/jakubpawlowicz/clean-css/issues/829) - adds more strict selector merging rules. diff --git a/lib/optimizer/basic.js b/lib/optimizer/basic.js index f06bbec8..fd98b09e 100644 --- a/lib/optimizer/basic.js +++ b/lib/optimizer/basic.js @@ -356,7 +356,7 @@ function optimizeBody(properties, context) { var property, name, type, value; var valueIsUrl; var propertyToken; - var _properties = wrapForOptimizing(properties); + var _properties = wrapForOptimizing(properties, true); for (var i = 0, l = _properties.length; i < l; i++) { property = _properties[i]; diff --git a/lib/properties/optimizer.js b/lib/properties/optimizer.js index f7057a53..fd44ca17 100644 --- a/lib/properties/optimizer.js +++ b/lib/properties/optimizer.js @@ -193,7 +193,7 @@ function optimize(selector, properties, mergeAdjacent, withCompacting, context) var validator = context.validator; var warnings = context.warnings; - var _properties = wrapForOptimizing(properties); + var _properties = wrapForOptimizing(properties, false); populateComponents(_properties, validator, warnings); _optimize(_properties, mergeAdjacent, context.options.aggressiveMerging, validator); diff --git a/lib/properties/wrap-for-optimizing.js b/lib/properties/wrap-for-optimizing.js index 22ad5462..2b9e1ca8 100644 --- a/lib/properties/wrap-for-optimizing.js +++ b/lib/properties/wrap-for-optimizing.js @@ -12,20 +12,28 @@ var Match = { IMPORTANT_WORD_PATTERN: new RegExp('important$', 'i'), STAR: '*', SUFFIX_BANG_PATTERN: /!$/, - UNDERSCORE: '_' + UNDERSCORE: '_', + VARIABLE_REFERENCE_PATTERN: /var\(--.+\)$/ }; -function wrapAll(properties) { +function wrapAll(properties, includeVariable) { var wrapped = []; var single; + var property; var i; for (i = properties.length - 1; i >= 0; i--) { - if (properties[i][0] != Token.PROPERTY) { + property = properties[i]; + + if (property[0] != Token.PROPERTY) { + continue; + } + + if (!includeVariable && someVariableReferences(property)) { continue; } - single = wrapSingle(properties[i]); + single = wrapSingle(property); single.all = properties; single.position = i; wrapped.unshift(single); @@ -34,6 +42,30 @@ function wrapAll(properties) { return wrapped; } +function someVariableReferences(property) { + var i, l; + var value; + + // skipping `property` and property name tokens + for (i = 2, l = property.length; i < l; i++) { + value = property[i]; + + if (value[0] != Token.PROPERTY_VALUE) { + continue; + } + + if (isVariableReference(value[1])) { + return true; + } + } + + return false; +} + +function isVariableReference(value) { + return Match.VARIABLE_REFERENCE_PATTERN.test(value); +} + function isMultiplex(property) { var value; var i, l; diff --git a/test/optimizer/advanced-test.js b/test/optimizer/advanced-test.js index d15e5bb0..42c3d2fc 100644 --- a/test/optimizer/advanced-test.js +++ b/test/optimizer/advanced-test.js @@ -102,4 +102,20 @@ vows.describe('advanced optimizer') ] }) ) + .addBatch( + optimizerContext('variables', { + 'skip processing properties with variable values - border - 1st value': [ + '.one{border:var(--color) solid 1px}', + '.one{border:var(--color) solid 1px}' + ], + 'skip processing properties with variable values - border - 2nd value': [ + '.one{border:red var(--style) 1px}', + '.one{border:red var(--style) 1px}' + ], + 'skip processing properties with variable values - border - 3rd value': [ + '.one{border:red solid var(--width)}', + '.one{border:red solid var(--width)}' + ] + }) + ) .export(module); diff --git a/test/properties/wrap-for-optimizing-test.js b/test/properties/wrap-for-optimizing-test.js index dc3f3fd3..56458b6d 100644 --- a/test/properties/wrap-for-optimizing-test.js +++ b/test/properties/wrap-for-optimizing-test.js @@ -14,7 +14,7 @@ vows.describe(wrapForOptimizing) ['property-value', '0'], ['property-value', '0'] ] - ]); + ], true); }, 'has one wrap': function (wrapped) { assert.lengthOf(wrapped, 1); @@ -64,7 +64,7 @@ vows.describe(wrapForOptimizing) ['property-name', 'color'], ['property-value', 'red'] ] - ]); + ], true); }, 'has two wraps': function (wrapped) { assert.lengthOf(wrapped, 2); @@ -82,7 +82,7 @@ vows.describe(wrapForOptimizing) ['property-name', 'color'], ['property-value', 'red'] ] - ]); + ], true); }, 'has one wrap': function (wrapped) { assert.lengthOf(wrapped, 1); @@ -101,7 +101,7 @@ vows.describe(wrapForOptimizing) ['property-value', '/'], ['property-value', '2px'] ] - ]); + ], true); }, 'has one wrap': function (wrapped) { assert.lengthOf(wrapped, 1); @@ -124,7 +124,7 @@ vows.describe(wrapForOptimizing) ['property-name', '--color'], ['property-value', 'red'] ] - ]); + ], true); }, 'has one wrap': function (wrapped) { assert.lengthOf(wrapped, 1); @@ -136,6 +136,40 @@ vows.describe(wrapForOptimizing) assert.isFalse(wrapped[0].block); } }, + 'variable reference': { + 'topic': function () { + return wrapForOptimizing([ + [ + 'property', + ['property-name', 'color'], + ['property-value', 'var(--red)'] + ] + ], true); + }, + 'has one wrap': function (wrapped) { + assert.lengthOf(wrapped, 1); + }, + 'has name': function (wrapped) { + assert.deepEqual(wrapped[0].name, 'color'); + }, + 'has value': function (wrapped) { + assert.deepEqual(wrapped[0].value, [['property-value', 'var(--red)']]); + } + }, + 'variable reference when variables are ignored': { + 'topic': function () { + return wrapForOptimizing([ + [ + 'property', + ['property-name', 'color'], + ['property-value', 'var(--red)'] + ] + ], false); + }, + 'has one wrap': function (wrapped) { + assert.lengthOf(wrapped, 0); + } + }, 'variable block': { 'topic': function () { return wrapForOptimizing([ @@ -158,7 +192,7 @@ vows.describe(wrapForOptimizing) ] ] ] - ]); + ], true); }, 'has one wrap': function (wrapped) { assert.lengthOf(wrapped, 1); @@ -183,7 +217,7 @@ vows.describe(wrapForOptimizing) ] ] ] - ]); + ], true); }, 'is a block': function (wrapped) { assert.isTrue(wrapped[0].block); @@ -196,7 +230,7 @@ vows.describe(wrapForOptimizing) 'property', ['property-name', 'margin'] ] - ]); + ], true); }, 'has one wrap': function (wrapped) { assert.lengthOf(wrapped, 1); @@ -216,7 +250,7 @@ vows.describe(wrapForOptimizing) ['property-name', 'margin'], ['property-value', '0!important'] ] - ]); + ], true); }, 'has one wrap': function (wrapped) { assert.lengthOf(wrapped, 1); @@ -237,7 +271,7 @@ vows.describe(wrapForOptimizing) ['property-value', '0'], ['property-value', '!important'] ] - ]); + ], true); }, 'has one wrap': function (wrapped) { assert.lengthOf(wrapped, 1); @@ -258,7 +292,7 @@ vows.describe(wrapForOptimizing) ['property-value', '0!'], ['property-value', 'important'] ] - ]); + ], true); }, 'has one wrap': function (wrapped) { assert.lengthOf(wrapped, 1); @@ -280,7 +314,7 @@ vows.describe(wrapForOptimizing) ['property-value', '!'], ['property-value', 'important'] ] - ]); + ], true); }, 'has one wrap': function (wrapped) { assert.lengthOf(wrapped, 1); @@ -303,7 +337,7 @@ vows.describe(wrapForOptimizing) ['property-name', '_color'], ['property-value', 'red'] ] - ]); + ], true); }, 'has one wrap': function (wrapped) { assert.lengthOf(wrapped, 1); @@ -323,7 +357,7 @@ vows.describe(wrapForOptimizing) ['property-name', '*color'], ['property-value', 'red'] ] - ]); + ], true); }, 'has one wrap': function (wrapped) { assert.lengthOf(wrapped, 1); @@ -343,7 +377,7 @@ vows.describe(wrapForOptimizing) ['property-name', 'margin'], ['property-value', '0\\9'] ] - ]); + ], true); }, 'has one wrap': function (wrapped) { assert.lengthOf(wrapped, 1); @@ -363,7 +397,7 @@ vows.describe(wrapForOptimizing) ['property-name', 'margin'], ['property-value', '0'] ] - ]); + ], true); }, 'has one wrap': function (wrapped) { assert.lengthOf(wrapped, 1); @@ -384,7 +418,7 @@ vows.describe(wrapForOptimizing) ['property-value', '0'], ['property-value', '\\9'] ] - ]); + ], true); }, 'has one wrap': function (wrapped) { assert.lengthOf(wrapped, 1); @@ -404,7 +438,7 @@ vows.describe(wrapForOptimizing) ['property-name', 'margin'], ['property-value', '0!ie'] ] - ]); + ], true); }, 'has one wrap': function (wrapped) { assert.lengthOf(wrapped, 1); @@ -427,7 +461,7 @@ vows.describe(wrapForOptimizing) ['property-name', 'margin'], ['property-value', '0 !ie'] ] - ]); + ], true); }, 'has one wrap': function (wrapped) { assert.lengthOf(wrapped, 1); @@ -451,7 +485,7 @@ vows.describe(wrapForOptimizing) ['property-value', '0'], ['property-value', '!ie'] ] - ]); + ], true); }, 'has one wrap': function (wrapped) { assert.lengthOf(wrapped, 1); @@ -474,7 +508,7 @@ vows.describe(wrapForOptimizing) ['property-name', 'color'], ['property-value', 'red\\9!important'] ] - ]); + ], true); }, 'has one wrap': function (wrapped) { assert.lengthOf(wrapped, 1); @@ -497,7 +531,7 @@ vows.describe(wrapForOptimizing) ['property-name', 'color', [[1, 2, undefined]]], ['property-value', 'red', [[1, 2, undefined]]] ] - ]); + ], true); }, 'has one wrap': function (wrapped) { assert.lengthOf(wrapped, 1);