Fixes #587 - too aggressive border reordering.

Not all border reordering cases were covered correctly.
clean-css · May 31, 2015 · a7ebb7b · a7ebb7b
1 parent 7d3b0d7
commit a7ebb7b
Show file tree

Hide file tree

Showing 8 changed files with 145 additions and 54 deletions.
diff --git a/History.md b/History.md
@@ -4,6 +4,7 @@
 * Fixed issue [#563](https://github.com/jakubpawlowicz/clean-css/issues/563) - `background:inherit` restoring.
 * Fixed issue [#582](https://github.com/jakubpawlowicz/clean-css/issues/582) - overriding with prefixed values.
 * Fixed issue [#583](https://github.com/jakubpawlowicz/clean-css/issues/583) - URL quoting for SVG data.
+* Fixed issue [#587](https://github.com/jakubpawlowicz/clean-css/issues/587) - too aggressive `border` reordering.
 
 [3.2.10 / 2015-05-14](https://github.com/jakubpawlowicz/clean-css/compare/v3.2.9...v3.2.10)
 ==================

diff --git a/lib/selectors/extractor.js b/lib/selectors/extractor.js
@@ -46,8 +46,12 @@ function findNameRoot(name) {
     return name;
   if (name.indexOf('-radius') > 0)
     return 'border-radius';
-  if (name.indexOf('border-') === 0)
+  if (name == 'border-collapse' || name == 'border-spacing' || name == 'border-image')
+    return name;
+  if (name.indexOf('border-') === 0 && /^border\-\w+\-\w+$/.test(name))
     return name.match(/border\-\w+/)[0];
+  if (name.indexOf('border-') === 0 && /^border\-\w+$/.test(name))
+    return 'border';
   if (name.indexOf('text-') === 0)
     return name;
 

diff --git a/lib/selectors/reorderable.js b/lib/selectors/reorderable.js
@@ -1,6 +1,7 @@
 // TODO: it'd be great to merge it with the other canReorder functionality
 
 var FLEX_PROPERTIES = /align\-items|box\-align|box\-pack|flex|justify/;
+var BORDER_PROPERTIES = /^border\-(top|right|bottom|left|color|style|width)/;
 
 function canReorder(left, right) {
   for (var i = right.length - 1; i >= 0; i--) {
@@ -31,6 +32,10 @@ function canReorderSingle(left, right) {
     return false;
   if (leftNameRoot == rightNameRoot && unprefixed(leftName) == unprefixed(rightName) && (vendorPrefixed(leftName) ^ vendorPrefixed(rightName)))
     return false;
+  if (leftNameRoot == 'border' && BORDER_PROPERTIES.test(rightNameRoot) && (leftName == 'border' || leftName == rightNameRoot))
+    return false;
+  if (rightNameRoot == 'border' && BORDER_PROPERTIES.test(leftNameRoot) && (rightName == 'border' || rightName == leftNameRoot))
+    return false;
   if (leftNameRoot != rightNameRoot)
     return true;
   if (leftName == rightName && leftNameRoot == rightNameRoot && (leftValue == rightValue || withDifferentVendorPrefix(leftValue, rightValue)))

diff --git a/test/fixtures/big-min.css b/test/fixtures/big-min.css
diff --git a/test/fixtures/bootstrap-min.css b/test/fixtures/bootstrap-min.css
diff --git a/test/selectors/extractor-test.js b/test/selectors/extractor-test.js
@@ -93,18 +93,36 @@ vows.describe(extractor)
           assert.deepEqual(tokens, [['-webkit-border-top-left-radius', 'none', 'border-radius', [['-webkit-border-top-left-radius'], ['none']], '-webkit-border-top-left-radius:none', [['a']], true]]);
         }
       },
-      'border-image': {
+      'border-image-width': {
         'topic': extractor(buildToken('a{border-image-width:2px}')),
         'has no properties': function (tokens) {
           assert.deepEqual(tokens, [['border-image-width', '2px', 'border-image', [['border-image-width'], ['2px']], 'border-image-width:2px', [['a']], true]]);
         }
       },
-      'border-top': {
+      'border-color': {
+        'topic': extractor(buildToken('a{border-color:red}')),
+        'has no properties': function (tokens) {
+          assert.deepEqual(tokens, [['border-color', 'red', 'border', [['border-color'], ['red']], 'border-color:red', [['a']], true]]);
+        }
+      },
+      'border-top-style': {
         'topic': extractor(buildToken('a{border-top-style:none}')),
         'has no properties': function (tokens) {
           assert.deepEqual(tokens, [['border-top-style', 'none', 'border-top', [['border-top-style'], ['none']], 'border-top-style:none', [['a']], true]]);
         }
       },
+      'border-top': {
+        'topic': extractor(buildToken('a{border-top:none}')),
+        'has no properties': function (tokens) {
+          assert.deepEqual(tokens, [['border-top', 'none', 'border', [['border-top'], ['none']], 'border-top:none', [['a']], true]]);
+        }
+      },
+      'border-collapse': {
+        'topic': extractor(buildToken('a{border-collapse:collapse}')),
+        'has no properties': function (tokens) {
+          assert.deepEqual(tokens, [['border-collapse', 'collapse', 'border-collapse', [['border-collapse'], ['collapse']], 'border-collapse:collapse', [['a']], true]]);
+        }
+      },
       'text-shadow': {
         'topic': extractor(buildToken('a{text-shadow:none}')),
         'has no properties': function (tokens) {

diff --git a/test/selectors/optimizer-test.js b/test/selectors/optimizer-test.js
@@ -129,6 +129,10 @@ vows.describe(SelectorsOptimizer)
         'div{margin-top:0}.one{margin:5px}.two{display:block;margin-top:0}.three{color:red}.four{margin-top:0}',
         'div{margin-top:0}.one{margin:5px}.four,.two{margin-top:0}.two{display:block}.three{color:red}'
       ],
+      'over shorthand - border': [
+        '.one{border-color:red}.two{border:1px solid}.three{color:#fff;border-color:red}',
+        '.one{border-color:red}.two{border:1px solid}.three{color:#fff;border-color:red}'
+      ],
       'granuar over granular': [
         'div{margin-top:0}.one{margin-bottom:2px}.two{display:block;margin-top:0}',
         '.two,div{margin-top:0}.one{margin-bottom:2px}.two{display:block}'

diff --git a/test/selectors/reorderable-test.js b/test/selectors/reorderable-test.js
@@ -129,6 +129,62 @@ vows.describe(canReorderSingle)
         assert.isTrue(result);
       }
     },
+    'different properties with same root - border #1': {
+      'topic': function () {
+        return canReorderSingle(propertiesIn('a{border:none}')[0], propertiesIn('a{border-top-color:red}')[0]);
+      },
+      'must be false': function (result) {
+        assert.isFalse(result);
+      }
+    },
+    'different properties with same root - border #2': {
+      'topic': function () {
+        return canReorderSingle(propertiesIn('a{border-top:1px solid red}')[0], propertiesIn('a{border-bottom:1px solid blue}')[0]);
+      },
+      'must be true': function (result) {
+        assert.isTrue(result);
+      }
+    },
+    'different properties with same root - border #3': {
+      'topic': function () {
+        return canReorderSingle(propertiesIn('a{border-top-color:red}')[0], propertiesIn('a{border-bottom:1px solid blue}')[0]);
+      },
+      'must be true': function (result) {
+        assert.isTrue(result);
+      }
+    },
+    'different properties with same root - border #4': {
+      'topic': function () {
+        return canReorderSingle(propertiesIn('a{border-bottom:none}')[0], propertiesIn('a{border-bottom:1px solid blue}')[0]);
+      },
+      'must be false': function (result) {
+        assert.isFalse(result);
+      }
+    },
+    'different properties with same root - border #5': {
+      'topic': function () {
+        return canReorderSingle(propertiesIn('a{border-bottom:none}')[0], propertiesIn('a{border-bottom:none}')[0]);
+      },
+      'must be true': function (result) {
+        assert.isTrue(result);
+      }
+    },
+    'different properties with same root - border #6': {
+      'topic': function () {
+        return canReorderSingle(propertiesIn('a{border-radius:3px}')[0], propertiesIn('a{border:0}')[0]);
+      },
+      'must be true': function (result) {
+        assert.isTrue(result);
+      }
+    },
+    'different properties with same root - border #7': {
+      'topic': function () {
+        return canReorderSingle(propertiesIn('a{border-radius:3px}')[0], propertiesIn('a{border-style:solid}')[0]);
+      },
+      'must be true': function (result) {
+        assert.isTrue(result);
+      }
+    },
     'shorhand and longhand with different value': {
       'topic': function () {
         return canReorderSingle(propertiesIn('a{margin:3px}')[0], propertiesIn('a{margin-bottom:5px}')[0]);