fix(removeHiddenElems): remove hidden definitions usage (#1852)

svg · Nov 26, 2023 · e22d533 · e22d533
1 parent 7e259a2
commit e22d533
Show file tree

Hide file tree

Showing 6 changed files with 100 additions and 41 deletions.
diff --git a/docs/03-plugins/reuse-paths.mdx b/docs/03-plugins/reuse-paths.mdx
@@ -12,7 +12,7 @@ If the path contains other attributes, such as `style` or `transform`, they will
 
 :::tip
 
-If you only need SVG 2 or inline HTML compatibility, it's recommended to include the [Remove Xlink](/docs/plugins/remove-xlink/) plugin towards the end of your pipeline to convert references to `xlink:href` to the SVG 2 `href` attribute.
+If you only need SVG 2 or inline HTML compatibility, it's recommended to include the [Remove XLink](/docs/plugins/remove-xlink/) plugin towards the end of your pipeline to convert references to `xlink:href` to the SVG 2 `href` attribute.
 
 :::
 

diff --git a/lib/xast.js b/lib/xast.js
@@ -77,7 +77,8 @@ const visit = (node, visitor, parentNode) => {
 exports.visit = visit;
 
 /**
- * @type {(node: XastChild, parentNode: XastParent) => void}
+ * @param {XastChild} node
+ * @param {XastParent} parentNode
  */
 const detachNodeFromParent = (node, parentNode) => {
   // avoid splice to not break for loops

diff --git a/package.json b/package.json
@@ -1,7 +1,7 @@
 {
   "packageManager": "yarn@2.4.3",
   "name": "svgo",
-  "version": "3.0.4",
+  "version": "3.0.5",
   "description": "Nodejs-based tool for optimizing SVG vector graphics files",
   "license": "MIT",
   "keywords": [

diff --git a/plugins/removeAttrs.js b/plugins/removeAttrs.js
@@ -104,13 +104,11 @@ exports.fn = (root, params) => {
       enter: (node) => {
         for (let pattern of attrs) {
           // if no element separators (:), assume it's attribute name, and apply to all elements *regardless of value*
-          if (pattern.includes(elemSeparator) === false) {
-            pattern = ['.*', elemSeparator, pattern, elemSeparator, '.*'].join(
-              ''
-            );
+          if (!pattern.includes(elemSeparator)) {
+            pattern = ['.*', pattern, '.*'].join(elemSeparator);
             // if only 1 separator, assume it's element and attribute name, and apply regardless of attribute value
           } else if (pattern.split(elemSeparator).length < 3) {
-            pattern = [pattern, elemSeparator, '.*'].join('');
+            pattern = [pattern, '.*'].join(elemSeparator);
           }
 
           // create regexps for element, attribute name, and attribute value

diff --git a/plugins/removeHiddenElems.js b/plugins/removeHiddenElems.js
@@ -1,6 +1,7 @@
 'use strict';
 
 /**
+ * @typedef {import('../lib/types').XastChild} XastChild
  * @typedef {import('../lib/types').XastElement} XastElement
  * @typedef {import('../lib/types').XastParent} XastParent
  */
@@ -66,11 +67,40 @@ exports.fn = (root, params) => {
    */
   const nonRenderedNodes = new Map();
 
+  /**
+   * IDs for removed hidden definitions.
+   *
+   * @type {Set<string>}
+   */
+  const removedDefIds = new Set();
+
+  /**
+   * @param {XastChild} node
+   * @param {XastParent} parentNode
+   */
+  function removeElement(node, parentNode) {
+    if (
+      node.type === 'element' &&
+      node.attributes.id != null &&
+      parentNode.type === 'element' &&
+      parentNode.name === 'defs'
+    ) {
+      removedDefIds.add(node.attributes.id);
+    }
+
+    detachNodeFromParent(node, parentNode);
+  }
+
   visit(root, {
     element: {
       enter: (node, parentNode) => {
         // transparent non-rendering elements still apply where referenced
         if (nonRendering.includes(node.name)) {
+          if (node.attributes.id == null) {
+            detachNodeFromParent(node, parentNode);
+            return visitSkip;
+          }
+
           nonRenderedNodes.set(node, parentNode);
           return visitSkip;
         }
@@ -84,8 +114,7 @@ exports.fn = (root, params) => {
           computedStyle.opacity.type === 'static' &&
           computedStyle.opacity.value === '0'
         ) {
-          detachNodeFromParent(node, parentNode);
-          return;
+          removeElement(node, parentNode);
         }
       },
     },
@@ -105,7 +134,7 @@ exports.fn = (root, params) => {
           // keep if any descendant enables visibility
           querySelector(node, '[visibility=visible]') == null
         ) {
-          detachNodeFromParent(node, parentNode);
+          removeElement(node, parentNode);
           return;
         }
 
@@ -122,7 +151,7 @@ exports.fn = (root, params) => {
           // markers with display: none still rendered
           node.name !== 'marker'
         ) {
-          detachNodeFromParent(node, parentNode);
+          removeElement(node, parentNode);
           return;
         }
 
@@ -138,7 +167,7 @@ exports.fn = (root, params) => {
           node.children.length === 0 &&
           node.attributes.r === '0'
         ) {
-          detachNodeFromParent(node, parentNode);
+          removeElement(node, parentNode);
           return;
         }
 
@@ -154,7 +183,7 @@ exports.fn = (root, params) => {
           node.children.length === 0 &&
           node.attributes.rx === '0'
         ) {
-          detachNodeFromParent(node, parentNode);
+          removeElement(node, parentNode);
           return;
         }
 
@@ -170,7 +199,7 @@ exports.fn = (root, params) => {
           node.children.length === 0 &&
           node.attributes.ry === '0'
         ) {
-          detachNodeFromParent(node, parentNode);
+          removeElement(node, parentNode);
           return;
         }
 
@@ -186,7 +215,7 @@ exports.fn = (root, params) => {
           node.children.length === 0 &&
           node.attributes.width === '0'
         ) {
-          detachNodeFromParent(node, parentNode);
+          removeElement(node, parentNode);
           return;
         }
 
@@ -203,7 +232,7 @@ exports.fn = (root, params) => {
           node.children.length === 0 &&
           node.attributes.height === '0'
         ) {
-          detachNodeFromParent(node, parentNode);
+          removeElement(node, parentNode);
           return;
         }
 
@@ -218,7 +247,7 @@ exports.fn = (root, params) => {
           node.name === 'pattern' &&
           node.attributes.width === '0'
         ) {
-          detachNodeFromParent(node, parentNode);
+          removeElement(node, parentNode);
           return;
         }
 
@@ -233,7 +262,7 @@ exports.fn = (root, params) => {
           node.name === 'pattern' &&
           node.attributes.height === '0'
         ) {
-          detachNodeFromParent(node, parentNode);
+          removeElement(node, parentNode);
           return;
         }
 
@@ -248,7 +277,7 @@ exports.fn = (root, params) => {
           node.name === 'image' &&
           node.attributes.width === '0'
         ) {
-          detachNodeFromParent(node, parentNode);
+          removeElement(node, parentNode);
           return;
         }
 
@@ -263,7 +292,7 @@ exports.fn = (root, params) => {
           node.name === 'image' &&
           node.attributes.height === '0'
         ) {
-          detachNodeFromParent(node, parentNode);
+          removeElement(node, parentNode);
           return;
         }
 
@@ -274,12 +303,12 @@ exports.fn = (root, params) => {
         // <path d=""/>
         if (pathEmptyD && node.name === 'path') {
           if (node.attributes.d == null) {
-            detachNodeFromParent(node, parentNode);
+            removeElement(node, parentNode);
             return;
           }
           const pathData = parsePathData(node.attributes.d);
           if (pathData.length === 0) {
-            detachNodeFromParent(node, parentNode);
+            removeElement(node, parentNode);
             return;
           }
           // keep single point paths for markers
@@ -288,7 +317,7 @@ exports.fn = (root, params) => {
             computedStyle['marker-start'] == null &&
             computedStyle['marker-end'] == null
           ) {
-            detachNodeFromParent(node, parentNode);
+            removeElement(node, parentNode);
             return;
           }
           return;
@@ -304,7 +333,7 @@ exports.fn = (root, params) => {
           node.name === 'polyline' &&
           node.attributes.points == null
         ) {
-          detachNodeFromParent(node, parentNode);
+          removeElement(node, parentNode);
           return;
         }
 
@@ -318,31 +347,47 @@ exports.fn = (root, params) => {
           node.name === 'polygon' &&
           node.attributes.points == null
         ) {
-          detachNodeFromParent(node, parentNode);
-          return;
+          removeElement(node, parentNode);
         }
       },
 
       exit: (node, parentNode) => {
-        if (node.name !== 'svg' || parentNode.type !== 'root') {
+        if (node.name === 'defs' && node.children.length === 0) {
+          removeElement(node, parentNode);
           return;
         }
-        for (const [
-          nonRenderedNode,
-          nonRenderedParent,
-        ] of nonRenderedNodes.entries()) {
-          if (nonRenderedNode.attributes.id == null) {
-            detachNodeFromParent(node, nonRenderedParent);
-            continue;
+
+        if (node.name === 'use') {
+          const referencesRemovedDef = Object.entries(node.attributes).some(
+            ([attrKey, attrValue]) =>
+              (attrKey === 'href' || attrKey.endsWith(':href')) &&
+              removedDefIds.has(
+                attrValue.slice(attrValue.indexOf('#') + 1).trim()
+              )
+          );
+
+          if (referencesRemovedDef) {
+            detachNodeFromParent(node, parentNode);
           }
 
-          const selector = referencesProps
-            .map((attr) => `[${attr}="url(#${nonRenderedNode.attributes.id})"]`)
-            .join(',');
+          return;
+        }
+
+        if (node.name === 'svg' && parentNode.type === 'root') {
+          for (const [
+            nonRenderedNode,
+            nonRenderedParent,
+          ] of nonRenderedNodes.entries()) {
+            const selector = referencesProps
+              .map(
+                (attr) => `[${attr}="url(#${nonRenderedNode.attributes.id})"]`
+              )
+              .join(',');
 
-          const element = querySelector(root, selector);
-          if (element == null) {
-            detachNodeFromParent(node, nonRenderedParent);
+            const element = querySelector(root, selector);
+            if (element == null) {
+              detachNodeFromParent(node, nonRenderedParent);
+            }
           }
         }
       },

diff --git a/test/plugins/removeHiddenElems.13.svg b/test/plugins/removeHiddenElems.13.svg