fix: improve handling of url references in reference attributes (#1881)

Author: SethFalco

lib/svgo/tools.js

@@ -6,7 +6,11 @@
  * @typedef {import('../types').DataUri} DataUri
  */
 
-const { attrsGroups } = require('../../plugins/_collections');
+const { attrsGroups, referencesProps } = require('../../plugins/_collections');
+
+const regReferencesUrl = /\burl\((["'])?#(.+?)\1\)/g;
+const regReferencesHref = /^#(.+?)$/;
+const regReferencesBegin = /(\w+)\.[a-zA-Z]/;
 
 /**
  * Encode plain SVG data string into Data URI string.
@@ -188,6 +192,39 @@ exports.hasScripts = hasScripts;
  * @returns {boolean} If the given string includes a URL reference.
  */
 const includesUrlReference = (body) => {
-  return /\burl\((["'])?#(.+?)\1\)/g.test(body);
+  return new RegExp(regReferencesUrl).test(body);
 };
 exports.includesUrlReference = includesUrlReference;
+
+/**
+ * @param {string} attribute
+ * @param {string} value
+ * @returns {string[]}
+ */
+const findReferences = (attribute, value) => {
+  const results = [];
+
+  if (referencesProps.includes(attribute)) {
+    const matches = value.matchAll(regReferencesUrl);
+    for (const match of matches) {
+      results.push(match[2]);
+    }
+  }
+
+  if (attribute === 'href' || attribute.endsWith(':href')) {
+    const match = regReferencesHref.exec(value);
+    if (match != null) {
+      results.push(match[1]);
+    }
+  }
+
+  if (attribute === 'begin') {
+    const match = regReferencesBegin.exec(value);
+    if (match != null) {
+      results.push(match[1]);
+    }
+  }
+
+  return results.map((body) => decodeURI(body));
+};
+exports.findReferences = findReferences;

plugins/cleanupIds.js

@@ -5,15 +5,11 @@
  */
 
 const { visitSkip } = require('../lib/xast.js');
-const { hasScripts } = require('../lib/svgo/tools');
-const { referencesProps } = require('./_collections.js');
+const { hasScripts, findReferences } = require('../lib/svgo/tools');
 
 exports.name = 'cleanupIds';
 exports.description = 'removes unused IDs and minifies used';
 
-const regReferencesUrl = /\burl\((["'])?#(.+?)\1\)/g;
-const regReferencesHref = /^#(.+?)$/;
-const regReferencesBegin = /(\w+)\.[a-zA-Z]/;
 const generateIdChars = [
   'a',
   'b',
@@ -191,35 +187,12 @@ exports.fn = (_root, params) => {
               nodeById.set(id, node);
             }
           } else {
-            // collect all references
-            /**
-             * @type {string[]}
-             */
-            let ids = [];
-            if (referencesProps.includes(name)) {
-              const matches = value.matchAll(regReferencesUrl);
-              for (const match of matches) {
-                ids.push(match[2]); // url() reference
-              }
-            }
-            if (name === 'href' || name.endsWith(':href')) {
-              const match = value.match(regReferencesHref);
-              if (match != null) {
-                ids.push(match[1]); // href reference
-              }
-            }
-            if (name === 'begin') {
-              const match = value.match(regReferencesBegin);
-              if (match != null) {
-                ids.push(match[1]); // href reference
-              }
-            }
+            const ids = findReferences(name, value);
             for (const id of ids) {
-              const decodedId = decodeURI(id);
-              let refs = referencesById.get(decodedId);
+              let refs = referencesById.get(id);
               if (refs == null) {
                 refs = [];
-                referencesById.set(decodedId, refs);
+                referencesById.set(id, refs);
               }
               refs.push({ element: node, name });
             }

plugins/prefixIds.js

@@ -237,8 +237,8 @@ exports.fn = (_root, params, info) => {
             node.attributes[name].length !== 0
           ) {
             node.attributes[name] = node.attributes[name].replace(
-              /url\((.*?)\)/gi,
-              (match, url) => {
+              /\burl\((["'])?(#.+?)\1\)/gi,
+              (match, _, url) => {
                 const prefixed = prefixReference(prefixGenerator, url);
                 if (prefixed == null) {
                   return match;

plugins/removeHiddenElems.js

@@ -6,7 +6,7 @@
  * @typedef {import('../lib/types').XastParent} XastParent
  */
 
-const { elemsGroups, referencesProps } = require('./_collections.js');
+const { elemsGroups } = require('./_collections.js');
 const {
   visit,
   visitSkip,
@@ -15,7 +15,7 @@ const {
 } = require('../lib/xast.js');
 const { collectStylesheet, computeStyle } = require('../lib/style.js');
 const { parsePathData } = require('../lib/path.js');
-const { hasScripts } = require('../lib/svgo/tools.js');
+const { hasScripts, findReferences } = require('../lib/svgo/tools.js');
 
 const nonRendering = elemsGroups.nonRendering;
 
@@ -80,6 +80,9 @@ exports.fn = (root, params) => {
    */
   const allDefs = new Map();
 
+  /** @type {Set<string>} */
+  const allReferences = new Set();
+
   /**
    * @type {Map<string, Array<{ node: XastElement, parentNode: XastParent }>>}
    */
@@ -363,7 +366,6 @@ exports.fn = (root, params) => {
             removeElement(node, parentNode);
             return;
           }
-          return;
         }
 
         // Polyline with empty points
@@ -391,6 +393,15 @@ exports.fn = (root, params) => {
           node.attributes.points == null
         ) {
           removeElement(node, parentNode);
+          return;
+        }
+
+        for (const [name, value] of Object.entries(node.attributes)) {
+          const ids = findReferences(name, value);
+
+          for (const id of ids) {
+            allReferences.add(id);
+          }
         }
       },
     },
@@ -411,13 +422,8 @@ exports.fn = (root, params) => {
             nonRenderedParent,
           ] of nonRenderedNodes.entries()) {
             const id = nonRenderedNode.attributes.id;
-            const selector = referencesProps
-              .map((attr) => `[${attr}="url(#${id})"]`)
-              .concat(`[href="#${id}"]`, `[xlink\\:href="#${id}"]`)
-              .join(',');
 
-            const element = querySelector(root, selector);
-            if (element == null) {
+            if (!allReferences.has(id)) {
               detachNodeFromParent(nonRenderedNode, nonRenderedParent);
             }
           }