From 03223df9f7bcb6b2ee7364b84b3b39b0e745d0a6 Mon Sep 17 00:00:00 2001 From: Jakub Pawlowicz Date: Thu, 20 Apr 2017 16:09:19 +0200 Subject: [PATCH] Fixes #525 - restores `inherit`-based merging. Why: * Merging longhands into shorthand when `inherit` is present is possible and this commit restores this functionality; * it was previously removed after big level 2 optimizations refactoring in clean-css 3.2. --- History.md | 1 + lib/optimizer/level-2/compactable.js | 28 +- .../properties/merge-into-shorthands.js | 239 +++++++++++++++++- .../properties/merge-into-shorthands-test.js | 79 +++++- 4 files changed, 324 insertions(+), 23 deletions(-) diff --git a/History.md b/History.md index d23f8019..76830cbd 100644 --- a/History.md +++ b/History.md @@ -2,6 +2,7 @@ ================== * Fixed issue [#254](https://github.com/jakubpawlowicz/clean-css/issues/254) - adds `font` property optimizer. +* Fixed issue [#525](https://github.com/jakubpawlowicz/clean-css/issues/525) - restores `inherit`-based merging. * Fixed issue [#755](https://github.com/jakubpawlowicz/clean-css/issues/755) - adds custom handling of remote requests. * Fixed issue [#860](https://github.com/jakubpawlowicz/clean-css/issues/860) - adds `animation` property optimizer. * Fixed issue [#862](https://github.com/jakubpawlowicz/clean-css/issues/862) - allows removing unused at rules. diff --git a/lib/optimizer/level-2/compactable.js b/lib/optimizer/level-2/compactable.js index 097b6cf7..b076a0d7 100644 --- a/lib/optimizer/level-2/compactable.js +++ b/lib/optimizer/level-2/compactable.js @@ -359,6 +359,7 @@ var compactable = { 'border-width' ], defaultValue: 'medium', + oppositeTo: 'border-top-width', shortestValue: '0' }, 'border-collapse': { @@ -424,6 +425,7 @@ var compactable = { 'border-width' ], defaultValue: 'medium', + oppositeTo: 'border-right-width', shortestValue: '0' }, 'border-radius': { @@ -487,6 +489,7 @@ var compactable = { 'border-width' ], defaultValue: 'medium', + oppositeTo: 'border-left-width', shortestValue: '0' }, 'border-style': { @@ -571,6 +574,7 @@ var compactable = { 'border-width' ], defaultValue: 'medium', + oppositeTo: 'border-bottom-width', shortestValue: '0' }, 'border-width': { @@ -741,28 +745,32 @@ var compactable = { componentOf: [ 'margin' ], - defaultValue: '0' + defaultValue: '0', + oppositeTo: 'margin-top' }, 'margin-left': { canOverride: canOverride.generic.unit, componentOf: [ 'margin' ], - defaultValue: '0' + defaultValue: '0', + oppositeTo: 'margin-right' }, 'margin-right': { canOverride: canOverride.generic.unit, componentOf: [ 'margin' ], - defaultValue: '0' + defaultValue: '0', + oppositeTo: 'margin-left' }, 'margin-top': { canOverride: canOverride.generic.unit, componentOf: [ 'margin' ], - defaultValue: '0' + defaultValue: '0', + oppositeTo: 'margin-bottom' }, 'outline': { canOverride: canOverride.generic.components([ @@ -838,28 +846,32 @@ var compactable = { componentOf: [ 'padding' ], - defaultValue: '0' + defaultValue: '0', + oppositeTo: 'padding-top' }, 'padding-left': { canOverride: canOverride.generic.unit, componentOf: [ 'padding' ], - defaultValue: '0' + defaultValue: '0', + oppositeTo: 'padding-right' }, 'padding-right': { canOverride: canOverride.generic.unit, componentOf: [ 'padding' ], - defaultValue: '0' + defaultValue: '0', + oppositeTo: 'padding-left' }, 'padding-top': { canOverride: canOverride.generic.unit, componentOf: [ 'padding' ], - defaultValue: '0' + defaultValue: '0', + oppositeTo: 'padding-bottom' }, 'position': { canOverride: canOverride.property.position, diff --git a/lib/optimizer/level-2/properties/merge-into-shorthands.js b/lib/optimizer/level-2/properties/merge-into-shorthands.js index a75c4ed6..bce4ed8e 100644 --- a/lib/optimizer/level-2/properties/merge-into-shorthands.js +++ b/lib/optimizer/level-2/properties/merge-into-shorthands.js @@ -4,11 +4,15 @@ var populateComponents = require('./populate-components'); var compactable = require('../compactable'); var deepClone = require('../clone').deep; +var restoreWithComponents = require('../restore-with-components'); +var restoreFromOptimizing = require('../../restore-from-optimizing'); var wrapSingle = require('../../wrap-for-optimizing').single; +var serializeBody = require('../../../writer/one-time').body; var Token = require('../../../tokenizer/token'); + function mixedImportance(components) { var important; @@ -22,6 +26,49 @@ function mixedImportance(components) { return false; } +function overridable(components, name, validator) { + var descriptor = compactable[name]; + var newValuePlaceholder = [ + Token.PROPERTY, + [Token.PROPERTY_NAME, name], + [Token.PROPERTY_VALUE, descriptor.defaultValue] + ]; + var newProperty = wrapSingle(newValuePlaceholder); + var component; + var mayOverride; + var i, l; + + populateComponents([newProperty], validator, []); + + for (i = 0, l = descriptor.components.length; i < l; i++) { + component = components[descriptor.components[i]]; + mayOverride = compactable[component.name].canOverride; + + if (!everyValuesPair(mayOverride.bind(null, validator), newProperty.components[i], component)) { + return false; + } + } + + return true; +} + +function mixedInherit(components) { + var lastValue = null; + var currentValue; + + for (var name in components) { + currentValue = hasInherit(components[name]); + + if (lastValue !== null && lastValue !== currentValue) { + return true; + } + + lastValue = currentValue; + } + + return false; +} + function joinMetadata(components, at) { var metadata = []; var component; @@ -37,7 +84,180 @@ function joinMetadata(components, at) { Array.prototype.push.apply(metadata, componentMetadata); } - return metadata; + return metadata.sort(metadataSorter); +} + +function metadataSorter(metadata1, metadata2) { + var line1 = metadata1[0]; + var line2 = metadata2[0]; + var column1 = metadata1[1]; + var column2 = metadata2[1]; + + if (line1 < line2) { + return -1; + } else if (line1 === line2) { + return column1 < column2 ? -1 : 1; + } else { + return 1; + } +} + +function replaceWithInheritBestFit(properties, candidateComponents, name, validator) { + var viaLonghands = buildSequenceWithInheritLonghands(candidateComponents, name, validator); + var viaShorthand = buildSequenceWithInheritShorthand(candidateComponents, name, validator); + var longhandTokensSequence = viaLonghands[0]; + var shorthandTokensSequence = viaShorthand[0]; + var isLonghandsShorter = serializeBody(longhandTokensSequence).length < serializeBody(shorthandTokensSequence).length; + var newTokensSequence = isLonghandsShorter ? longhandTokensSequence : shorthandTokensSequence; + var newProperty = isLonghandsShorter ? viaLonghands[1] : viaShorthand[1]; + var newComponents = isLonghandsShorter ? viaLonghands[2] : viaShorthand[2]; + var all = candidateComponents[Object.keys(candidateComponents)[0]].all; + var componentName; + var oldComponent; + var newComponent; + var newToken; + + newProperty.position = all.length; + newProperty.shorthand = true; + newProperty.dirty = true; + newProperty.all = all; + newProperty.all.push(newTokensSequence[0]); + + properties.push(newProperty); + + for (componentName in candidateComponents) { + oldComponent = candidateComponents[componentName]; + oldComponent.unused = true; + + if (oldComponent.name in newComponents) { + newComponent = newComponents[oldComponent.name]; + newToken = findTokenIn(newTokensSequence, componentName); + + newComponent.position = all.length; + newComponent.all = all; + newComponent.all.push(newToken); + + properties.push(newComponent); + } + } +} + +function buildSequenceWithInheritLonghands(components, name, validator) { + var tokensSequence = []; + var inheritComponents = {}; + var nonInheritComponents = {}; + var descriptor = compactable[name]; + var shorthandToken = [ + Token.PROPERTY, + [Token.PROPERTY_NAME, name], + [Token.PROPERTY_VALUE, descriptor.defaultValue] + ]; + var newProperty = wrapSingle(shorthandToken); + var component; + var longhandToken; + var newComponent; + var nameMetadata; + var i, l; + + populateComponents([newProperty], validator, []); + + for (i = 0, l = descriptor.components.length; i < l; i++) { + component = components[descriptor.components[i]]; + + if (hasInherit(component)) { + longhandToken = component.all[component.position].slice(0, 2); + Array.prototype.push.apply(longhandToken, component.value); + tokensSequence.push(longhandToken); + + newComponent = deepClone(component); + newComponent.value = inferComponentValue(components, newComponent.name); + + newProperty.components[i] = newComponent; + inheritComponents[component.name] = deepClone(component); + } else { + newComponent = deepClone(component); + newComponent.all = component.all; + newProperty.components[i] = newComponent; + + nonInheritComponents[component.name] = component; + } + } + + nameMetadata = joinMetadata(nonInheritComponents, 1); + shorthandToken[1].push(nameMetadata); + + restoreFromOptimizing([newProperty], restoreWithComponents); + + shorthandToken = shorthandToken.slice(0, 2); + Array.prototype.push.apply(shorthandToken, newProperty.value); + + tokensSequence.unshift(shorthandToken); + + return [tokensSequence, newProperty, inheritComponents]; +} + +function inferComponentValue(components, propertyName) { + var descriptor = compactable[propertyName]; + + if ('oppositeTo' in descriptor) { + return components[descriptor.oppositeTo].value; + } else { + return [[Token.PROPERTY_VALUE, descriptor.defaultValue]]; + } +} + +function buildSequenceWithInheritShorthand(components, name, validator) { + var tokensSequence = []; + var inheritComponents = {}; + var nonInheritComponents = {}; + var descriptor = compactable[name]; + var shorthandToken = [ + Token.PROPERTY, + [Token.PROPERTY_NAME, name], + [Token.PROPERTY_VALUE, 'inherit'] + ]; + var newProperty = wrapSingle(shorthandToken); + var component; + var longhandToken; + var nameMetadata; + var valueMetadata; + var i, l; + + populateComponents([newProperty], validator, []); + + for (i = 0, l = descriptor.components.length; i < l; i++) { + component = components[descriptor.components[i]]; + + if (hasInherit(component)) { + inheritComponents[component.name] = component; + } else { + longhandToken = component.all[component.position].slice(0, 2); + Array.prototype.push.apply(longhandToken, component.value); + tokensSequence.push(longhandToken); + + nonInheritComponents[component.name] = deepClone(component); + } + } + + nameMetadata = joinMetadata(inheritComponents, 1); + shorthandToken[1].push(nameMetadata); + + valueMetadata = joinMetadata(inheritComponents, 2); + shorthandToken[2].push(valueMetadata); + + tokensSequence.unshift(shorthandToken); + + return [tokensSequence, newProperty, nonInheritComponents]; +} + +function findTokenIn(tokens, componentName) { + var i, l; + + for (i = 0, l = tokens.length; i < l; i++) { + if (tokens[i][1][1] == componentName) { + return tokens[i]; + } + } } function replaceWithShorthand(properties, candidateComponents, name, validator) { @@ -49,7 +269,6 @@ function replaceWithShorthand(properties, candidateComponents, name, validator) [Token.PROPERTY_NAME, name], [Token.PROPERTY_VALUE, descriptor.defaultValue] ]; - var mayOverride; var all; var newProperty = wrapSingle(newValuePlaceholder); @@ -61,13 +280,6 @@ function replaceWithShorthand(properties, candidateComponents, name, validator) for (var i = 0, l = descriptor.components.length; i < l; i++) { var component = candidateComponents[descriptor.components[i]]; - if (hasInherit(component)) - return; - - mayOverride = compactable[component.name].canOverride; - if (!everyValuesPair(mayOverride.bind(null, validator), newProperty.components[i], component)) - return; - newProperty.components[i] = deepClone(component); newProperty.important = component.important; @@ -108,7 +320,14 @@ function invalidateOrCompact(properties, position, candidates, validator) { if (mixedImportance(candidateComponents)) continue; - replaceWithShorthand(properties, candidateComponents, name, validator); + if (!overridable(candidateComponents, name, validator)) + continue; + + if (mixedInherit(candidateComponents)) { + replaceWithInheritBestFit(properties, candidateComponents, name, validator); + } else { + replaceWithShorthand(properties, candidateComponents, name, validator); + } } } diff --git a/test/optimizer/level-2/properties/merge-into-shorthands-test.js b/test/optimizer/level-2/properties/merge-into-shorthands-test.js index 03f3e113..f92c7890 100644 --- a/test/optimizer/level-2/properties/merge-into-shorthands-test.js +++ b/test/optimizer/level-2/properties/merge-into-shorthands-test.js @@ -506,12 +506,38 @@ vows.describe(optimizeProperties) ]); } }, - 'with inherit - pending #525': { + 'with inherit - one': { 'topic': function () { return _optimize('a{padding-top:10px;padding-left:5px;padding-bottom:3px;padding-right:inherit}'); }, 'into': function (properties) { assert.deepEqual(properties, [ + [ + 'property', + ['property-name', 'padding', [[1, 2, undefined], [1, 19, undefined], [1, 36, undefined]]], + ['property-value', '10px', [[1, 14, undefined]]], + ['property-value', '5px', [[1, 32, undefined]]], + ['property-value', '3px', [[1, 51, undefined]]] + ], + [ + 'property', + ['property-name', 'padding-right', [[1, 55, undefined]]], + ['property-value', 'inherit', [[1, 69, undefined]]] + ] + ]); + } + }, + 'with inherit - two': { + 'topic': function () { + return _optimize('a{padding-top:10px;padding-left:5px;padding-bottom:inherit;padding-right:inherit}'); + }, + 'into': function (properties) { + assert.deepEqual(properties, [ + [ + 'property', + ['property-name', 'padding', [[1, 36, undefined], [1, 59, undefined]]], + ['property-value', 'inherit', [[1, 51, undefined], [1, 73, undefined]]] + ], [ 'property', ['property-name', 'padding-top', [[1, 2, undefined]]], @@ -521,16 +547,59 @@ vows.describe(optimizeProperties) 'property', ['property-name', 'padding-left', [[1, 19, undefined]]], ['property-value', '5px', [[1, 32, undefined]]] + ] + ]); + } + }, + 'with inherit - three': { + 'topic': function () { + return _optimize('a{padding-top:inherit;padding-left:5px;padding-bottom:inherit;padding-right:inherit}'); + }, + 'into': function (properties) { + assert.deepEqual(properties, [ + [ + 'property', + ['property-name', 'padding', [[1, 2, undefined], [1, 39, undefined], [1, 62, undefined]]], + ['property-value', 'inherit', [[1, 14, undefined], [1, 54, undefined], [1, 76, undefined]]] ], [ 'property', - ['property-name', 'padding-bottom', [[1, 36, undefined]]], - ['property-value', '3px', [[1, 51, undefined]]] + ['property-name', 'padding-left', [[1, 22, undefined]]], + ['property-value', '5px', [[1, 35, undefined]]] + ] + ]); + } + }, + 'with inherit - four': { + 'topic': function () { + return _optimize('a{padding-top:inherit;padding-left:inherit;padding-bottom:inherit;padding-right:inherit}'); + }, + 'into': function (properties) { + assert.deepEqual(properties, [ + [ + 'property', + ['property-name', 'padding', [[1, 2, undefined], [1, 22, undefined], [1, 43, undefined], [1, 66, undefined]]], + ['property-value', 'inherit', [[1, 14, undefined]]] + ] + ]); + } + }, + 'with inherit - outline': { + 'topic': function () { + return _optimize('.block{outline-width:inherit;outline-style:solid;outline-color:red}'); + }, + 'into': function (properties) { + assert.deepEqual(properties, [ + [ + 'property', + ['property-name', 'outline', [[1, 29, undefined], [1, 49, undefined]]], + ['property-value', 'red', [[1, 63, undefined]]], + ['property-value', 'solid', [[1, 43, undefined]]] ], [ 'property', - ['property-name', 'padding-right', [[1, 55, undefined]]], - ['property-value', 'inherit', [[1, 69, undefined]]] + ['property-name', 'outline-width', [[1, 7, undefined]]], + ['property-value', 'inherit', [[1, 21, undefined]]], ] ]); } -- 2.34.1