From 5aad38bcc730d8a4257f8c69c0a8f1349e5a3a66 Mon Sep 17 00:00:00 2001
From: Seth Falco <seth@falco.fun>
Date: Tue, 7 Nov 2023 23:20:48 +0000
Subject: [PATCH] fix(inlineStyles): empty css block created empty attribute
 (#1823)

When running this plugin on an SVG with an empty block in the CSS, it
would apply it to the matched elements by adding an empty `style`
attribute. See the test for an example.

This resolves that by just dropping the declaration if it's empty.
---
 lib/style.js                         | 35 +++++++----------
 lib/svgo/plugins.js                  |  2 +-
 package.json                         |  4 +-
 plugins/inlineStyles.js              | 56 ++++++----------------------
 plugins/plugins-types.ts             | 28 +++++++++++---
 plugins/removeUnknownsAndDefaults.js |  8 +---
 test/plugins/inlineStyles.23.svg     | 20 ++++++++++
 test/regression.js                   |  8 +---
 yarn.lock                            | 14 +++----
 9 files changed, 80 insertions(+), 95 deletions(-)
 create mode 100644 test/plugins/inlineStyles.23.svg

diff --git a/lib/style.js b/lib/style.js
index eb8d93d8d..a8f3b9986 100644
--- a/lib/style.js
+++ b/lib/style.js
@@ -15,7 +15,7 @@
 
 const csstree = require('css-tree');
 const {
-  // @ts-ignore not defined in @types/csso
+  // @ts-ignore internal api
   syntax: { specificity },
 } = require('csso');
 const { visit, matches } = require('./xast.js');
@@ -47,9 +47,7 @@ const parseRule = (ruleNode, dynamic) => {
     }
   });
 
-  /**
-   * @type {StylesheetRule[]}
-   */
+  /** @type {StylesheetRule[]} */
   const rules = [];
   csstree.walk(ruleNode.prelude, (node) => {
     if (node.type === 'Selector') {
@@ -78,9 +76,7 @@ const parseRule = (ruleNode, dynamic) => {
  * @type {(css: string, dynamic: boolean) => Array<StylesheetRule>}
  */
 const parseStylesheet = (css, dynamic) => {
-  /**
-   * @type {Array<StylesheetRule>}
-   */
+  /** @type {Array<StylesheetRule>} */
   const rules = [];
   const ast = csstree.parse(css, {
     parseValue: false,
@@ -111,9 +107,7 @@ const parseStylesheet = (css, dynamic) => {
  * @type {(css: string) => Array<StylesheetDeclaration>}
  */
 const parseStyleDeclarations = (css) => {
-  /**
-   * @type {Array<StylesheetDeclaration>}
-   */
+  /** @type {Array<StylesheetDeclaration>} */
   const declarations = [];
   const ast = csstree.parse(css, {
     context: 'declarationList',
@@ -135,9 +129,7 @@ const parseStyleDeclarations = (css) => {
  * @type {(stylesheet: Stylesheet, node: XastElement) => ComputedStyles}
  */
 const computeOwnStyle = (stylesheet, node) => {
-  /**
-   * @type {ComputedStyles}
-   */
+  /** @type {ComputedStyles} */
   const computedStyle = {};
   const importantStyles = new Map();
 
@@ -197,10 +189,12 @@ const computeOwnStyle = (stylesheet, node) => {
 };
 
 /**
- * Compares two selector specificities.
- * extracted from https://github.com/keeganstreet/specificity/blob/main/specificity.js#L211
+ * Compares selector specificities.
+ * Derived from https://github.com/keeganstreet/specificity/blob/8757133ddd2ed0163f120900047ff0f92760b536/specificity.js#L207
  *
- * @type {(a: Specificity, b: Specificity) => number}
+ * @param {Specificity} a
+ * @param {Specificity} b
+ * @returns {number}
  */
 const compareSpecificity = (a, b) => {
   for (let i = 0; i < 4; i += 1) {
@@ -213,18 +207,15 @@ const compareSpecificity = (a, b) => {
 
   return 0;
 };
+exports.compareSpecificity = compareSpecificity;
 
 /**
  * @type {(root: XastRoot) => Stylesheet}
  */
 const collectStylesheet = (root) => {
-  /**
-   * @type {Array<StylesheetRule>}
-   */
+  /** @type {Array<StylesheetRule>} */
   const rules = [];
-  /**
-   * @type {Map<XastElement, XastParent>}
-   */
+  /** @type {Map<XastElement, XastParent>} */
   const parents = new Map();
   visit(root, {
     element: {
diff --git a/lib/svgo/plugins.js b/lib/svgo/plugins.js
index 5719d1633..efeb01b14 100644
--- a/lib/svgo/plugins.js
+++ b/lib/svgo/plugins.js
@@ -14,7 +14,7 @@ const { visit } = require('../xast.js');
  */
 const invokePlugins = (ast, info, plugins, overrides, globalOverrides) => {
   for (const plugin of plugins) {
-    const override = overrides == null ? null : overrides[plugin.name];
+    const override = overrides?.[plugin.name];
     if (override === false) {
       continue;
     }
diff --git a/package.json b/package.json
index 63bb49ec9..c193de684 100644
--- a/package.json
+++ b/package.json
@@ -115,14 +115,14 @@
     "commander": "^7.2.0",
     "css-select": "^5.1.0",
     "css-tree": "^2.2.1",
-    "csso": "^5.0.5",
+    "csso": "5.0.5",
     "picocolors": "^1.0.0"
   },
   "devDependencies": {
     "@rollup/plugin-commonjs": "^22.0.2",
     "@rollup/plugin-node-resolve": "^14.1.0",
     "@types/css-tree": "^2.0.0",
-    "@types/csso": "^5.0.1",
+    "@types/csso": "~5.0.3",
     "@types/jest": "^29.5.5",
     "del": "^6.0.0",
     "eslint": "^8.24.0",
diff --git a/plugins/inlineStyles.js b/plugins/inlineStyles.js
index 323286f75..81245edba 100644
--- a/plugins/inlineStyles.js
+++ b/plugins/inlineStyles.js
@@ -8,7 +8,7 @@
 
 const csstree = require('css-tree');
 const {
-  // @ts-ignore not defined in @types/csso
+  // @ts-ignore internal api
   syntax: { specificity },
 } = require('csso');
 const {
@@ -16,49 +16,16 @@ const {
   querySelectorAll,
   detachNodeFromParent,
 } = require('../lib/xast.js');
+const { compareSpecificity } = require('../lib/style');
 
 exports.name = 'inlineStyles';
 exports.description = 'inline styles (additional options)';
 
 /**
- * Compares two selector specificities.
- * extracted from https://github.com/keeganstreet/specificity/blob/main/specificity.js#L211
- *
- * @type {(a: Specificity, b: Specificity) => number}
- */
-const compareSpecificity = (a, b) => {
-  for (var i = 0; i < 4; i += 1) {
-    if (a[i] < b[i]) {
-      return -1;
-    } else if (a[i] > b[i]) {
-      return 1;
-    }
-  }
-  return 0;
-};
-
-/**
- * Moves + merges styles from style elements to element styles
- *
- * Options
- *   onlyMatchedOnce (default: true)
- *     inline only selectors that match once
- *
- *   removeMatchedSelectors (default: true)
- *     clean up matched selectors,
- *     leave selectors that hadn't matched
- *
- *   useMqs (default: ['', 'screen'])
- *     what media queries to be used
- *     empty string element for styles outside media queries
- *
- *   usePseudos (default: [''])
- *     what pseudo-classes/-elements to be used
- *     empty string element for all non-pseudo-classes and/or -elements
- *
- * @author strarsis <strarsis@gmail.com>
+ * Merges styles from style nodes into inline styles.
  *
  * @type {import('./plugins-types').Plugin<'inlineStyles'>}
+ * @author strarsis <strarsis@gmail.com>
  */
 exports.fn = (root, params) => {
   const {
@@ -205,9 +172,7 @@ exports.fn = (root, params) => {
         for (const selector of sortedSelectors) {
           // match selectors
           const selectorText = csstree.generate(selector.item.data);
-          /**
-           * @type {Array<XastElement>}
-           */
+          /** @type {Array<XastElement>} */
           const matchedElements = [];
           try {
             for (const node of querySelectorAll(root, selectorText)) {
@@ -232,9 +197,7 @@ exports.fn = (root, params) => {
           // apply <style/> to matched elements
           for (const selectedEl of matchedElements) {
             const styleDeclarationList = csstree.parse(
-              selectedEl.attributes.style == null
-                ? ''
-                : selectedEl.attributes.style,
+              selectedEl.attributes.style ?? '',
               {
                 context: 'declarationList',
                 parseValue: false,
@@ -280,8 +243,11 @@ exports.fn = (root, params) => {
                 }
               },
             });
-            selectedEl.attributes.style =
-              csstree.generate(styleDeclarationList);
+
+            const newStyles = csstree.generate(styleDeclarationList);
+            if (newStyles.length !== 0) {
+              selectedEl.attributes.style = newStyles;
+            }
           }
 
           if (
diff --git a/plugins/plugins-types.ts b/plugins/plugins-types.ts
index daf5ff0ad..a3f8852f0 100644
--- a/plugins/plugins-types.ts
+++ b/plugins/plugins-types.ts
@@ -73,10 +73,28 @@ type DefaultPlugins = {
   };
   mergeStyles: void;
   inlineStyles: {
+    /**
+     * Inlines selectors that match once only.
+     *
+     * @default true
+     */
     onlyMatchedOnce?: boolean;
+    /**
+     * Clean up matched selectors. Unused selects are left as-is.
+     *
+     * @default true
+     */
     removeMatchedSelectors?: boolean;
-    useMqs?: Array<string>;
-    usePseudos?: Array<string>;
+    /**
+     * Media queries to use. An empty string indicates all selectors outside of
+     * media queries.
+     */
+    useMqs?: string[];
+    /**
+     * Pseudo-classes and elements to use. An empty string indicates all
+     * all non-pseudo-classes and elements.
+     */
+    usePseudos?: string[];
   };
   mergePaths: {
     force?: boolean;
@@ -89,13 +107,13 @@ type DefaultPlugins = {
      * Disable or enable a structure optimisations.
      * @default true
      */
-    restructure?: boolean | undefined;
+    restructure?: boolean;
     /**
      * Enables merging of @media rules with the same media query by splitted by other rules.
      * The optimisation is unsafe in general, but should work fine in most cases. Use it on your own risk.
      * @default false
      */
-    forceMediaMerge?: boolean | undefined;
+    forceMediaMerge?: boolean;
     /**
      * Specify what comments to leave:
      * - 'exclamation' or true – leave all exclamation comments
@@ -103,7 +121,7 @@ type DefaultPlugins = {
      * - false – remove all comments
      * @default true
      */
-    comments?: string | boolean | undefined;
+    comments?: string | boolean;
     /**
      * Advanced optimizations
      */
diff --git a/plugins/removeUnknownsAndDefaults.js b/plugins/removeUnknownsAndDefaults.js
index c67845cd5..410b36554 100644
--- a/plugins/removeUnknownsAndDefaults.js
+++ b/plugins/removeUnknownsAndDefaults.js
@@ -182,16 +182,12 @@ exports.fn = (root, params) => {
             attributesDefaults.get(name) === value
           ) {
             // keep defaults if parent has own or inherited style
-            if (
-              computedParentStyle == null ||
-              computedParentStyle[name] == null
-            ) {
+            if (computedParentStyle?.[name] == null) {
               delete node.attributes[name];
             }
           }
           if (uselessOverrides && node.attributes.id == null) {
-            const style =
-              computedParentStyle == null ? null : computedParentStyle[name];
+            const style = computedParentStyle?.[name];
             if (
               presentationNonInheritableGroupAttrs.includes(name) === false &&
               style != null &&
diff --git a/test/plugins/inlineStyles.23.svg b/test/plugins/inlineStyles.23.svg
new file mode 100644
index 000000000..f24756224
--- /dev/null
+++ b/test/plugins/inlineStyles.23.svg
@@ -0,0 +1,20 @@
+Empty selectors should just be dropped.
+
+===
+
+<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 45 35">
+  <style>
+    .a {}
+  </style>
+  <g class="a">
+    <circle class="b" cx="42.97" cy="24.92" r="1.14"/>
+  </g>
+</svg>
+
+@@@
+
+<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 45 35">
+    <g>
+        <circle class="b" cx="42.97" cy="24.92" r="1.14"/>
+    </g>
+</svg>
diff --git a/test/regression.js b/test/regression.js
index ade0928e8..81b6ac5d7 100644
--- a/test/regression.js
+++ b/test/regression.js
@@ -30,17 +30,11 @@ const runTests = async ({ list }) => {
       name.startsWith('w3c-svg-11-test-suite/svg/animate-') ||
       // messed gradients
       name === 'w3c-svg-11-test-suite/svg/pservers-grad-18-b.svg' ||
-      // animated filter
-      name === 'w3c-svg-11-test-suite/svg/filters-light-04-f.svg' ||
-      // animated filter
-      name === 'w3c-svg-11-test-suite/svg/filters-composite-05-f.svg' ||
       // removing wrapping <g> breaks :first-child pseudo-class
       name === 'w3c-svg-11-test-suite/svg/styling-pres-04-f.svg' ||
       // rect is converted to path which matches wrong styles
       name === 'w3c-svg-11-test-suite/svg/styling-css-08-f.svg' ||
-      // external image
-      name === 'w3c-svg-11-test-suite/svg/struct-image-02-b.svg' ||
-      // complex selectors are messed becase of converting shapes to paths
+      // complex selectors are messed because of converting shapes to paths
       name === 'w3c-svg-11-test-suite/svg/struct-use-10-f.svg' ||
       name === 'w3c-svg-11-test-suite/svg/struct-use-11-f.svg' ||
       name === 'w3c-svg-11-test-suite/svg/styling-css-01-b.svg' ||
diff --git a/yarn.lock b/yarn.lock
index 4291a4d15..bf9ad077b 100644
--- a/yarn.lock
+++ b/yarn.lock
@@ -958,12 +958,12 @@ __metadata:
   languageName: node
   linkType: hard
 
-"@types/csso@npm:^5.0.1":
-  version: 5.0.1
-  resolution: "@types/csso@npm:5.0.1"
+"@types/csso@npm:~5.0.3":
+  version: 5.0.3
+  resolution: "@types/csso@npm:5.0.3"
   dependencies:
     "@types/css-tree": "*"
-  checksum: 62c7e534dfde79c1bf3b3761baec06ac2aa2b568da9be4a548b95e709b65b3944b8ad4cd4d8926de2b4bb301b4f102fb0f8c1354cb203a92ebccf59e58c957a6
+  checksum: 3d299ea755732f9b913cfb3d94849e173cd1019559058af0a372aa1ca8f48a3c63aa74932fdfa2f2f25ee78255a115feaaec01ae4fe9578e76b7c4acd8ae3f2a
   languageName: node
   linkType: hard
 
@@ -1695,7 +1695,7 @@ __metadata:
   languageName: node
   linkType: hard
 
-"csso@npm:^5.0.5":
+"csso@npm:5.0.5":
   version: 5.0.5
   resolution: "csso@npm:5.0.5"
   dependencies:
@@ -4563,12 +4563,12 @@ __metadata:
     "@rollup/plugin-node-resolve": ^14.1.0
     "@trysound/sax": 0.2.0
     "@types/css-tree": ^2.0.0
-    "@types/csso": ^5.0.1
+    "@types/csso": ~5.0.3
     "@types/jest": ^29.5.5
     commander: ^7.2.0
     css-select: ^5.1.0
     css-tree: ^2.2.1
-    csso: ^5.0.5
+    csso: 5.0.5
     del: ^6.0.0
     eslint: ^8.24.0
     jest: ^29.5.5