Fix bad nodes cleaning to avoid exceptions (fixes #380)

lib/clean/Atrule.js

@@ -1,4 +1,5 @@
 var resolveKeyword = require('css-tree').keyword;
+var { hasNoChildren } = require('./utils');
 
 module.exports = function cleanAtrule(node, item, list) {
  if (node.block) {
@@ -7,15 +8,15 @@ module.exports = function cleanAtrule(node, item, list) {
  this.stylesheet.firstAtrulesAllowed = false;
  }
 
- if (node.block.children.isEmpty()) {
+ if (hasNoChildren(node.block)) {
  list.remove(item);
  return;
  }
  }
 
  switch (node.name) {
  case 'charset':
- if (!node.prelude || node.prelude.children.isEmpty()) {
+ if (hasNoChildren(node.prelude)) {
  list.remove(item);
  return;
  }
@@ -57,7 +58,7 @@ module.exports = function cleanAtrule(node, item, list) {
  name === 'supports') {
 
  // drop at-rule with no prelude
- if (!node.prelude || node.prelude.children.isEmpty()) {
+ if (hasNoChildren(node.prelude) || hasNoChildren(node.block)) {
  list.remove(item);
  }
  }

lib/clean/Raw.js

@@ -0,0 +1,9 @@
+var { isNodeChildrenList } = require('./utils');
+
+module.exports = function cleanRaw(node, item, list) {
+ // raw in stylesheet or block children
+ if (isNodeChildrenList(this.stylesheet, list) ||
+ isNodeChildrenList(this.block, list)) {
+ list.remove(item);
+ }
+};

lib/clean/Rule.js

@@ -1,5 +1,6 @@
 var hasOwnProperty = Object.prototype.hasOwnProperty;
 var walk = require('css-tree').walk;
+var { hasNoChildren } = require('./utils');
 
 function cleanUnused(selectorList, usageData) {
  selectorList.children.each(function(selector, item, list) {
@@ -73,15 +74,20 @@ function cleanUnused(selectorList, usageData) {
  return selectorList.children.isEmpty();
 }
 
-module.exports = function cleanRuleset(node, item, list, options) {
+module.exports = function cleanRule(node, item, list, options) {
+ if (hasNoChildren(node.prelude) || hasNoChildren(node.block)) {
+ list.remove(item);
+ return;
+ }
+
  var usageData = options.usage;
 
  if (usageData && (usageData.whitelist !== null || usageData.blacklist !== null)) {
  cleanUnused(node.prelude, usageData);
- }
 
- if (node.prelude.children.isEmpty() ||
- node.block.children.isEmpty()) {
- list.remove(item);
+ if (hasNoChildren(node.prelude)) {
+ list.remove(item);
+ return;
+ }
  }
 };

lib/clean/TypeSelector.js

@@ -1,5 +1,5 @@
 // remove useless universal selector
-module.exports = function cleanType(node, item, list) {
+module.exports = function cleanTypeSelector(node, item, list) {
  var name = item.data.name;
 
  // check it's a non-namespaced universal selector

lib/clean/WhiteSpace.js

@@ -1,18 +1,29 @@
+var { isNodeChildrenList } = require('./utils');
+
+function isSafeOperator(node) {
+ return node.type === 'Operator' && node.value !== '+' && node.value !== '-';
+}
+
 module.exports = function cleanWhitespace(node, item, list) {
  // remove when first or last item in sequence
  if (item.next === null || item.prev === null) {
  list.remove(item);
  return;
  }
 
- // remove when previous node is whitespace
- if (item.prev.data.type === 'WhiteSpace') {
+ // white space in stylesheet or block children
+ if (isNodeChildrenList(this.stylesheet, list) ||
+ isNodeChildrenList(this.block, list)) {
+ list.remove(item);
+ return;
+ }
+
+ if (item.next.data.type === 'WhiteSpace') {
  list.remove(item);
  return;
  }
 
- if ((this.stylesheet !== null && this.stylesheet.children === list) ||
- (this.block !== null && this.block.children === list)) {
+ if (isSafeOperator(item.prev.data) || isSafeOperator(item.next.data)) {
  list.remove(item);
  return;
  }

lib/clean/index.js

@@ -1,11 +1,11 @@
 var walk = require('css-tree').walk;
 var handlers = {
  Atrule: require('./Atrule'),
- Rule: require('./Rule'),
+ Comment: require('./Comment'),
  Declaration: require('./Declaration'),
+ Raw: require('./Raw'),
+ Rule: require('./Rule'),
  TypeSelector: require('./TypeSelector'),
- Comment: require('./Comment'),
- Operator: require('./Operator'),
  WhiteSpace: require('./WhiteSpace')
 };
 

lib/clean/utils.js

@@ -0,0 +1,8 @@
+module.exports = {
+ hasNoChildren: function(node) {
+ return !node || !node.children || node.children.isEmpty();
+ },
+ isNodeChildrenList: function(node, list) {
+ return node !== null && node.children === list;
+ }
+};

test/fixture/compress/atrules/media-merge/_nested-queries.min.css

@@ -1,23 +1 @@
-.foo {
- z-index: 1;
-}
-
-@supports (display: auto) {
- .foo {
- z-index: 2;
- }
-}
-
-@media (min-width: 1024px) {
- .foo {
- z-index: 3;
- }
- @supports (display: auto) {
- .foo {
- z-index: 4;
- }
- }
- .bar {
- z-index: 5;
- }
-}
+.foo{z-index:1}@media (min-width:1024px){.foo{z-index:3}.bar{z-index:5}}@supports (display:auto){.foo{z-index:2}@media (min-width:1024px){.foo{z-index:4}}}

test/fixture/compress/issue/39-27.css

@@ -0,0 +1,14 @@
+b, c, x:test {
+ color: blue;
+}
+b, a, x:test {
+ color: red
+}
+a, c, x:test {
+ color: green
+}
+
+/*
+ Correct solution (because `b` is now used without `:test` which can has limited support) is 
+ b,x:test{color:red}a,c,x:test{color:green}
+*/

test/fixture/compress/issue/39-27.min.css

@@ -0,0 +1 @@
+b{color:red}a,c,x:test{color:green}

test/fixture/compress/mess/4.css

@@ -0,0 +1,15 @@
+@media {
+ .foo { color: red; }
+}
+
+@media all {
+ .bar {
+ // bad comment
+ color: red;
+ }
+}
+
+.foo {
+ // bad comment
+ color: red;
+}

test/fixture/compress/restructure.merge/issue-384.css

@@ -1,3 +1,8 @@
+/*
+ Issue #384 should preserve order of declarations
+ when diff contains everride declarations
+*/
+
 .a {
  display: flex;
  box-sizing: border-box;