From 4c2cc1bbdba714df7a45ce3fe3de957d463a98bc Mon Sep 17 00:00:00 2001
From: Seth Falco <seth@falco.fun>
Date: Sat, 11 Nov 2023 12:43:13 +0000
Subject: [PATCH] feat(inlineStyles): remove redundant presentation attrs
 (#1829)

SVGs can have the same presentation attribute declared redundantly in
both the node attributes and `<style>` tag.

This wouldn't break anything, but we can shave off a few more bytes by
dropping the attribute in this case.
---
 docs/03-plugins/inline-styles.mdx             |  2 +-
 .../remove-attributes-by-selector.mdx         |  2 +-
 docs/03-plugins/remove-comments.mdx           |  4 ++
 docs/03-plugins/remove-empty-text.mdx         |  6 +-
 docs/03-plugins/remove-hidden-elems.mdx       |  2 +-
 .../remove-useless-stroke-and-fill.mdx        |  2 +-
 lib/style.js                                  | 19 ++++++
 lib/style.test.js                             | 29 +++++++++
 plugins/inlineStyles.js                       | 62 ++++++++++---------
 test/plugins/inlineStyles.24.svg              | 21 +++++++
 test/plugins/inlineStyles.25.svg              | 32 ++++++++++
 11 files changed, 144 insertions(+), 37 deletions(-)
 create mode 100644 test/plugins/inlineStyles.24.svg
 create mode 100644 test/plugins/inlineStyles.25.svg

diff --git a/docs/03-plugins/inline-styles.mdx b/docs/03-plugins/inline-styles.mdx
index 323050ebf..764acb418 100644
--- a/docs/03-plugins/inline-styles.mdx
+++ b/docs/03-plugins/inline-styles.mdx
@@ -16,7 +16,7 @@ svgo:
       description: What pseudo-classes and pseudo-elements to use. An empty string signifies all non-pseudo-classes and non-pseudo-elements. 
 ---
 
-Move and merge styles from `<style>` elements to a respective elements `style` attributes.
+Merges styles from `<style>` elements to the `style` attribute of matching elements.
 
 ## Usage
 
diff --git a/docs/03-plugins/remove-attributes-by-selector.mdx b/docs/03-plugins/remove-attributes-by-selector.mdx
index dd8b0389e..4e26c90b1 100644
--- a/docs/03-plugins/remove-attributes-by-selector.mdx
+++ b/docs/03-plugins/remove-attributes-by-selector.mdx
@@ -4,7 +4,7 @@ svgo:
   pluginId: removeAttributesBySelector
   parameters:
     selectors:
-      description: This is an array of objects with two properties, <code>selector</code>, and <code>attributes</code>, which represent a CSS selector and the attributes to remove respectively.
+      description: An array of objects with two properties, <code>selector</code>, and <code>attributes</code>, which represent a CSS selector and the attributes to remove respectively.
       default: null
 ---
 
diff --git a/docs/03-plugins/remove-comments.mdx b/docs/03-plugins/remove-comments.mdx
index 3c2a6fe8f..c1bb83fdb 100644
--- a/docs/03-plugins/remove-comments.mdx
+++ b/docs/03-plugins/remove-comments.mdx
@@ -28,6 +28,10 @@ Removing a comment like this may be considered a breach of the license terms, as
 
 <PluginUsage/>
 
+### Parameters
+
+<PluginParams/>
+
 ## Demo
 
 <PluginDemo/>
diff --git a/docs/03-plugins/remove-empty-text.mdx b/docs/03-plugins/remove-empty-text.mdx
index 389d4df9f..67080a7fa 100644
--- a/docs/03-plugins/remove-empty-text.mdx
+++ b/docs/03-plugins/remove-empty-text.mdx
@@ -5,13 +5,13 @@ svgo:
   defaultPlugin: true
   parameters:
     text:
-      description: Removes empty <a href="https://developer.mozilla.org/docs/Web/SVG/Element/text" target="_blank"><code>&lt;text&gt;</code></a> elements.
+      description: If to remove empty <a href="https://developer.mozilla.org/docs/Web/SVG/Element/text" target="_blank"><code>&lt;text&gt;</code></a> elements.
       default: true
     tspan:
-      description: Removes empty <a href="https://developer.mozilla.org/docs/Web/SVG/Element/tspan" target="_blank"><code>&lt;tspan&gt;</code></a> elements.
+      description: If to remove empty <a href="https://developer.mozilla.org/docs/Web/SVG/Element/tspan" target="_blank"><code>&lt;tspan&gt;</code></a> elements.
       default: true
     tref:
-      description: Removes empty <a href="https://developer.mozilla.org/docs/Web/SVG/Element/tref" target="_blank"><code>&lt;tref&gt;</code></a> elements.
+      description: If to remove empty <a href="https://developer.mozilla.org/docs/Web/SVG/Element/tref" target="_blank"><code>&lt;tref&gt;</code></a> elements.
       default: true
 ---
 
diff --git a/docs/03-plugins/remove-hidden-elems.mdx b/docs/03-plugins/remove-hidden-elems.mdx
index dc6a64a2e..a61a886d6 100644
--- a/docs/03-plugins/remove-hidden-elems.mdx
+++ b/docs/03-plugins/remove-hidden-elems.mdx
@@ -55,7 +55,7 @@ Remove hidden or invisible elements from the document. This can be elements with
 
 This plugin ignores non-rendering elements, such as [`<clipPath>`](https://developer.mozilla.org/docs/Web/SVG/Element/clipPath) and [`<linearGradient>`](https://developer.mozilla.org/docs/Web/SVG/Element/linearGradient), which still apply regardless of styles, unless they are unused.
 
-Refer to the paremeters for the conditions this plugin looks for. All checks enabled by default.
+Refer to the parameters for the conditions this plugin looks for. All checks enabled by default.
 
 ## Usage
 
diff --git a/docs/03-plugins/remove-useless-stroke-and-fill.mdx b/docs/03-plugins/remove-useless-stroke-and-fill.mdx
index 4e884f978..159b514b0 100644
--- a/docs/03-plugins/remove-useless-stroke-and-fill.mdx
+++ b/docs/03-plugins/remove-useless-stroke-and-fill.mdx
@@ -17,7 +17,7 @@ svgo:
 
 Removes useless `stroke` and `fill` attributes.
 
-Assigning these attributes can sometimes change nothing in the document. For example, in most cases assigning a `stoke` color is redundant if the elements [`stroke-width`](https://developer.mozilla.org/docs/Web/SVG/Attribute/stroke-width) or [`stroke-opacity`](https://developer.mozilla.org/docs/Web/SVG/Attribute/stroke-opacity) is `0`.
+Assigning these attributes can sometimes change nothing in the document. For example, in most cases assigning a `stroke` color is redundant if the elements [`stroke-width`](https://developer.mozilla.org/docs/Web/SVG/Attribute/stroke-width) or [`stroke-opacity`](https://developer.mozilla.org/docs/Web/SVG/Attribute/stroke-opacity) is `0`.
 
 ## Usage
 
diff --git a/lib/style.js b/lib/style.js
index a0469b50e..865daf299 100644
--- a/lib/style.js
+++ b/lib/style.js
@@ -276,3 +276,22 @@ const computeStyle = (stylesheet, node) => {
   return computedStyles;
 };
 exports.computeStyle = computeStyle;
+
+/**
+ * Determines if the CSS selector references the given attribute.
+ *
+ * @param {csstree.ListItem<csstree.CssNode>|string} selector
+ * @param {string} attr
+ * @returns {boolean}
+ * @see https://developer.mozilla.org/docs/Web/CSS/Attribute_selectors
+ */
+const includesAttrSelector = (selector, attr) => {
+  const attrSelectorPattern = new RegExp(`\\[\\s*${attr}\\s*[\\]=~|^$*]`, 'i');
+
+  if (typeof selector === 'string') {
+    return attrSelectorPattern.test(attr);
+  }
+
+  return attrSelectorPattern.test(csstree.generate(selector.data));
+};
+exports.includesAttrSelector = includesAttrSelector;
diff --git a/lib/style.test.js b/lib/style.test.js
index c4f63c4f2..1aaeb998b 100644
--- a/lib/style.test.js
+++ b/lib/style.test.js
@@ -306,3 +306,32 @@ it('ignores keyframes atrule', () => {
     },
   });
 });
+
+it('ignores @-webkit-keyframes atrule', () => {
+  const root = parseSvg(`
+      <svg>
+        <style>
+          .a {
+            animation: loading 4s linear infinite;
+          }
+          @-webkit-keyframes loading {
+            0% {
+              stroke-dashoffset: 440;
+            }
+            50% {
+              stroke-dashoffset: 0;
+            }
+          }
+        </style>
+        <rect id="element" class="a" />
+      </svg>
+    `);
+  const stylesheet = collectStylesheet(root);
+  expect(computeStyle(stylesheet, getElementById(root, 'element'))).toEqual({
+    animation: {
+      type: 'static',
+      inherited: false,
+      value: 'loading 4s linear infinite',
+    },
+  });
+});
diff --git a/plugins/inlineStyles.js b/plugins/inlineStyles.js
index 81245edba..db6bd2350 100644
--- a/plugins/inlineStyles.js
+++ b/plugins/inlineStyles.js
@@ -1,7 +1,6 @@
 'use strict';
 
 /**
- * @typedef {import('../lib/types').Specificity} Specificity
  * @typedef {import('../lib/types').XastElement} XastElement
  * @typedef {import('../lib/types').XastParent} XastParent
  */
@@ -16,7 +15,8 @@ const {
   querySelectorAll,
   detachNodeFromParent,
 } = require('../lib/xast.js');
-const { compareSpecificity } = require('../lib/style');
+const { compareSpecificity, includesAttrSelector } = require('../lib/style');
+const { attrsGroups } = require('./_collections');
 
 exports.name = 'inlineStyles';
 exports.description = 'inline styles (additional options)';
@@ -52,15 +52,12 @@ exports.fn = (root, params) => {
   return {
     element: {
       enter: (node, parentNode) => {
-        // skip <foreignObject /> content
         if (node.name === 'foreignObject') {
           return visitSkip;
         }
-        // collect only non-empty <style /> elements
         if (node.name !== 'style' || node.children.length === 0) {
           return;
         }
-        // values other than the empty string or text/css are not used
         if (
           node.attributes.type != null &&
           node.attributes.type !== '' &&
@@ -68,16 +65,14 @@ exports.fn = (root, params) => {
         ) {
           return;
         }
-        // parse css in style element
-        let cssText = '';
-        for (const child of node.children) {
-          if (child.type === 'text' || child.type === 'cdata') {
-            cssText += child.value;
-          }
-        }
-        /**
-         * @type {?csstree.CssNode}
-         */
+
+        const cssText = node.children
+          .filter((child) => child.type === 'text' || child.type === 'cdata')
+          // @ts-ignore
+          .map((child) => child.value)
+          .join('');
+
+        /** @type {?csstree.CssNode} */
         let cssAst = null;
         try {
           cssAst = csstree.parse(cssText, {
@@ -102,14 +97,14 @@ exports.fn = (root, params) => {
             }
 
             // skip media queries not included into useMqs param
-            let mq = '';
+            let mediaQuery = '';
             if (atrule != null) {
-              mq = atrule.name;
+              mediaQuery = atrule.name;
               if (atrule.prelude != null) {
-                mq += ` ${csstree.generate(atrule.prelude)}`;
+                mediaQuery += ` ${csstree.generate(atrule.prelude)}`;
               }
             }
-            if (useMqs.includes(mq) === false) {
+            if (!useMqs.includes(mediaQuery)) {
               return;
             }
 
@@ -138,7 +133,8 @@ exports.fn = (root, params) => {
                 pseudos.map((pseudo) => pseudo.item.data)
               ),
             });
-            if (usePseudos.includes(pseudoSelectors) === false) {
+
+            if (!usePseudos.includes(pseudoSelectors)) {
               return;
             }
 
@@ -160,8 +156,8 @@ exports.fn = (root, params) => {
         if (styles.length === 0) {
           return;
         }
-        // stable sort selectors
-        const sortedSelectors = [...selectors]
+        const sortedSelectors = selectors
+          .slice()
           .sort((a, b) => {
             const aSpecificity = specificity(a.item.data);
             const bSpecificity = specificity(b.item.data);
@@ -221,9 +217,18 @@ exports.fn = (root, params) => {
                 // no inline styles, external styles,                                    external styles used
                 // inline styles,    external styles same   priority as inline styles,   inline   styles used
                 // inline styles,    external styles higher priority than inline styles, external styles used
-                const matchedItem = styleDeclarationItems.get(
-                  ruleDeclaration.property
-                );
+                const property = ruleDeclaration.property;
+
+                if (
+                  attrsGroups.presentation.includes(property) &&
+                  !selectors.some((selector) =>
+                    includesAttrSelector(selector.item, property)
+                  )
+                ) {
+                  delete selectedEl.attributes[property];
+                }
+
+                const matchedItem = styleDeclarationItems.get(property);
                 const ruleDeclarationItem =
                   styleDeclarationList.children.createItem(ruleDeclaration);
                 if (matchedItem == null) {
@@ -236,10 +241,7 @@ exports.fn = (root, params) => {
                     matchedItem,
                     ruleDeclarationItem
                   );
-                  styleDeclarationItems.set(
-                    ruleDeclaration.property,
-                    ruleDeclarationItem
-                  );
+                  styleDeclarationItems.set(property, ruleDeclarationItem);
                 }
               },
             });
@@ -262,7 +264,7 @@ exports.fn = (root, params) => {
         }
 
         // no further processing required
-        if (removeMatchedSelectors === false) {
+        if (!removeMatchedSelectors) {
           return;
         }
 
diff --git a/test/plugins/inlineStyles.24.svg b/test/plugins/inlineStyles.24.svg
new file mode 100644
index 000000000..e88b74001
--- /dev/null
+++ b/test/plugins/inlineStyles.24.svg
@@ -0,0 +1,21 @@
+If we're going to inline styles for property that is also a presentation
+attribute, and that presentation attribute was already defined in the node, we
+can just drop the presentation attribute as it would be overridden by the style
+anyway.
+
+===
+
+<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 269 349">
+  <style type="text/css">
+    .a {
+      fill: #059669;
+    }
+  </style>
+  <path class="a" d="M191.5,324.1V355l9.6-31.6A77.49,77.49,0,0,1,191.5,324.1Z" fill="#059669" transform="translate(-57.17 -13.4)"/>
+</svg>
+
+@@@
+
+<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 269 349">
+    <path d="M191.5,324.1V355l9.6-31.6A77.49,77.49,0,0,1,191.5,324.1Z" transform="translate(-57.17 -13.4)" style="fill:#059669"/>
+</svg>
diff --git a/test/plugins/inlineStyles.25.svg b/test/plugins/inlineStyles.25.svg
new file mode 100644
index 000000000..04e02878a
--- /dev/null
+++ b/test/plugins/inlineStyles.25.svg
@@ -0,0 +1,32 @@
+Don't remove the redundant presentation attribute if it's used in a CSS
+selector in a `<style> tag.
+
+===
+
+<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 50 50">
+  <style>
+    .a {
+      stroke: red;
+    }
+
+    [stroke] + path {
+      stroke: purple;
+    }
+  </style>
+  <path class="a" d="M10 10h20" stroke="red"/>
+  <path d="M10 20h20"/>
+  <path d="M10 30h20" stroke="yellow"/>
+  <path d="M10 40h20"/>
+</svg>
+
+@@@
+
+<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 50 50">
+    <style>
+        [stroke]+path{stroke:purple}
+    </style>
+    <path d="M10 10h20" stroke="red" style="stroke:red"/>
+    <path d="M10 20h20"/>
+    <path d="M10 30h20" stroke="yellow"/>
+    <path d="M10 40h20"/>
+</svg>