From b179707c1955f8b4adda8be899364387f29ccc6f Mon Sep 17 00:00:00 2001 From: Kendell R Date: Sat, 9 Dec 2023 09:23:46 -0800 Subject: [PATCH 1/8] fix(removeHiddenElems): handle defs better --- plugins/removeHiddenElems.js | 72 ++++++++++++++++++++++-------------- 1 file changed, 44 insertions(+), 28 deletions(-) diff --git a/plugins/removeHiddenElems.js b/plugins/removeHiddenElems.js index ad58324d8..3942f3b27 100644 --- a/plugins/removeHiddenElems.js +++ b/plugins/removeHiddenElems.js @@ -74,6 +74,11 @@ exports.fn = (root, params) => { */ const removedDefIds = new Set(); + /** + * @type {Map>} + */ + const referencesById = new Map(); + /** * @param {XastChild} node * @param {XastParent} parentNode @@ -104,6 +109,23 @@ exports.fn = (root, params) => { nonRenderedNodes.set(node, parentNode); return visitSkip; } + + if (node.name == 'use') { + const reference = Object.keys(node.attributes).find( + (attr) => attr === 'href' || attr.endsWith('href') + ); + const referenceValue = reference && node.attributes[reference]; + const referenceId = referenceValue && referenceValue.slice(1); + if (referenceId) { + let refs = referencesById.get(referenceId); + if (!refs) { + refs = []; + referencesById.set(referenceId, refs); + } + refs.push({ node, parentNode }); + } + } + const computedStyle = computeStyle(stylesheet, node); // opacity="0" // @@ -356,38 +378,32 @@ exports.fn = (root, params) => { removeElement(node, parentNode); return; } - - 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); + }, + }, + root: { + exit: () => { + // Remove uses of deleted definitions + for (const id of removedDefIds) { + const refs = referencesById.get(id); + if (refs) { + for (const { node, parentNode } of refs) { + detachNodeFromParent(node, parentNode); + } } - - 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(','); + // Remove definitions that are unused + 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(nonRenderedNode, nonRenderedParent); } } }, From 2ad44953c6ddaa435518b82ecd86bd770b6ab241 Mon Sep 17 00:00:00 2001 From: Kendell R Date: Sat, 9 Dec 2023 09:56:55 -0800 Subject: [PATCH 2/8] Fix all regression tests --- plugins/removeHiddenElems.js | 74 +++++++++++++++++++++++------------- 1 file changed, 47 insertions(+), 27 deletions(-) diff --git a/plugins/removeHiddenElems.js b/plugins/removeHiddenElems.js index 3942f3b27..ef8f97ba8 100644 --- a/plugins/removeHiddenElems.js +++ b/plugins/removeHiddenElems.js @@ -15,6 +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 nonRendering = elemsGroups.nonRendering; @@ -79,6 +80,11 @@ exports.fn = (root, params) => { */ const referencesById = new Map(); + /** + * If styles are present, we can't be sure if a definition is unused or not + */ + let deoptimized = false; + /** * @param {XastChild} node * @param {XastParent} parentNode @@ -110,22 +116,6 @@ exports.fn = (root, params) => { return visitSkip; } - if (node.name == 'use') { - const reference = Object.keys(node.attributes).find( - (attr) => attr === 'href' || attr.endsWith('href') - ); - const referenceValue = reference && node.attributes[reference]; - const referenceId = referenceValue && referenceValue.slice(1); - if (referenceId) { - let refs = referencesById.get(referenceId); - if (!refs) { - refs = []; - referencesById.set(referenceId, refs); - } - refs.push({ node, parentNode }); - } - } - const computedStyle = computeStyle(stylesheet, node); // opacity="0" // @@ -145,6 +135,32 @@ exports.fn = (root, params) => { return { element: { enter: (node, parentNode) => { + if ( + (node.name === 'style' && node.children.length !== 0) || + hasScripts(node) + ) { + deoptimized = true; + return; + } + + // Store uses so that they can be removed if broken + // and used to determine if a definition is unused + if (node.name == 'use') { + const reference = Object.keys(node.attributes).find( + (attr) => attr === 'href' || attr.endsWith('href') + ); + const referenceValue = reference && node.attributes[reference]; + const referenceId = referenceValue && referenceValue.slice(1); + if (referenceId) { + let refs = referencesById.get(referenceId); + if (!refs) { + refs = []; + referencesById.set(referenceId, refs); + } + refs.push({ node, parentNode }); + } + } + // Removes hidden elements // https://www.w3schools.com/cssref/pr_class_visibility.asp const computedStyle = computeStyle(stylesheet, node); @@ -393,17 +409,21 @@ exports.fn = (root, params) => { } // Remove definitions that are unused - 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(nonRenderedNode, nonRenderedParent); + if (!deoptimized) { + for (const [ + nonRenderedNode, + 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) { + detachNodeFromParent(nonRenderedNode, nonRenderedParent); + } } } }, From 790767a76c8fed1fad694c4faf3a0cbe03cc5176 Mon Sep 17 00:00:00 2001 From: Kendell R Date: Sat, 9 Dec 2023 12:36:16 -0800 Subject: [PATCH 3/8] address review (cleanup code and look at all href) --- plugins/removeHiddenElems.js | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/plugins/removeHiddenElems.js b/plugins/removeHiddenElems.js index ef8f97ba8..22c4242eb 100644 --- a/plugins/removeHiddenElems.js +++ b/plugins/removeHiddenElems.js @@ -143,19 +143,16 @@ exports.fn = (root, params) => { return; } - // Store uses so that they can be removed if broken - // and used to determine if a definition is unused - if (node.name == 'use') { - const reference = Object.keys(node.attributes).find( - (attr) => attr === 'href' || attr.endsWith('href') - ); - const referenceValue = reference && node.attributes[reference]; - const referenceId = referenceValue && referenceValue.slice(1); - if (referenceId) { - let refs = referencesById.get(referenceId); + if (node.name === 'use') { + for (const attr of Object.keys(node.attributes)) { + if (attr !== 'href' && !attr.endsWith(':href')) continue; + const value = node.attributes[attr]; + const id = value.slice(1); + + let refs = referencesById.get(id); if (!refs) { refs = []; - referencesById.set(referenceId, refs); + referencesById.set(id, refs); } refs.push({ node, parentNode }); } From dcd805f3f3a8ccc01812b79319078a0f930b6c97 Mon Sep 17 00:00:00 2001 From: Kendell R Date: Sat, 9 Dec 2023 12:42:57 -0800 Subject: [PATCH 4/8] add tests (untested) --- test/plugins/removeHiddenElems.14.svg | 14 ++++++++++++++ test/plugins/removeHiddenElems.15.svg | 20 ++++++++++++++++++++ 2 files changed, 34 insertions(+) create mode 100644 test/plugins/removeHiddenElems.14.svg create mode 100644 test/plugins/removeHiddenElems.15.svg diff --git a/test/plugins/removeHiddenElems.14.svg b/test/plugins/removeHiddenElems.14.svg new file mode 100644 index 000000000..17def4b78 --- /dev/null +++ b/test/plugins/removeHiddenElems.14.svg @@ -0,0 +1,14 @@ +Remove unused defs + +=== + + + + + + + + +@@@ + + diff --git a/test/plugins/removeHiddenElems.15.svg b/test/plugins/removeHiddenElems.15.svg new file mode 100644 index 000000000..e469b102d --- /dev/null +++ b/test/plugins/removeHiddenElems.15.svg @@ -0,0 +1,20 @@ +Don't remove used defs + +=== + + + + + + + + + +@@@ + + + + + + + \ No newline at end of file From 70a2d93895dd0c5cedaf14d0e38817dc2157ecfd Mon Sep 17 00:00:00 2001 From: Kendell R Date: Sat, 9 Dec 2023 12:58:42 -0800 Subject: [PATCH 5/8] Remove empty defs without multipass --- plugins/removeHiddenElems.js | 41 +++++++++++++++++++++++++++--------- 1 file changed, 31 insertions(+), 10 deletions(-) diff --git a/plugins/removeHiddenElems.js b/plugins/removeHiddenElems.js index 22c4242eb..13826038a 100644 --- a/plugins/removeHiddenElems.js +++ b/plugins/removeHiddenElems.js @@ -75,6 +75,18 @@ exports.fn = (root, params) => { */ const removedDefIds = new Set(); + /** + * @type {Map} + */ + const allDefs = new Map(); + + /** + * Defs that may have had their children removed. + * + * @type {Set} + */ + const affectedDefs = new Set(); + /** * @type {Map>} */ @@ -91,12 +103,16 @@ exports.fn = (root, params) => { */ function removeElement(node, parentNode) { if ( - node.type === 'element' && - node.attributes.id != null && parentNode.type === 'element' && parentNode.name === 'defs' ) { - removedDefIds.add(node.attributes.id); + affectedDefs.add(parentNode) + if ( + node.type === 'element' && + node.attributes.id != null + ) { + removedDefIds.add(node.attributes.id); + } } detachNodeFromParent(node, parentNode); @@ -143,6 +159,10 @@ exports.fn = (root, params) => { return; } + if (node.name === 'defs') { + allDefs.set(node, parentNode); + } + if (node.name === 'use') { for (const attr of Object.keys(node.attributes)) { if (attr !== 'href' && !attr.endsWith(':href')) continue; @@ -385,13 +405,6 @@ exports.fn = (root, params) => { removeElement(node, parentNode); } }, - - exit: (node, parentNode) => { - if (node.name === 'defs' && node.children.length === 0) { - removeElement(node, parentNode); - return; - } - }, }, root: { exit: () => { @@ -423,6 +436,14 @@ exports.fn = (root, params) => { } } } + + // Remove empty defs + for (const node of affectedDefs) { + if (node.children.length === 0) { + const parentNode = allDefs.get(node); + detachNodeFromParent(node, parentNode) + } + } }, }, }; From 9e508ae3c1d7290600872ea3310adea3708c2bc4 Mon Sep 17 00:00:00 2001 From: Kendell R Date: Sat, 9 Dec 2023 13:23:27 -0800 Subject: [PATCH 6/8] fix a typo in the test, track all defs like before --- plugins/removeHiddenElems.js | 22 +++++----------------- test/plugins/removeHiddenElems.14.svg | 2 +- 2 files changed, 6 insertions(+), 18 deletions(-) diff --git a/plugins/removeHiddenElems.js b/plugins/removeHiddenElems.js index 13826038a..27a218b38 100644 --- a/plugins/removeHiddenElems.js +++ b/plugins/removeHiddenElems.js @@ -80,13 +80,6 @@ exports.fn = (root, params) => { */ const allDefs = new Map(); - /** - * Defs that may have had their children removed. - * - * @type {Set} - */ - const affectedDefs = new Set(); - /** * @type {Map>} */ @@ -103,16 +96,12 @@ exports.fn = (root, params) => { */ function removeElement(node, parentNode) { if ( + node.type === 'element' && + node.attributes.id != null && parentNode.type === 'element' && parentNode.name === 'defs' ) { - affectedDefs.add(parentNode) - if ( - node.type === 'element' && - node.attributes.id != null - ) { - removedDefIds.add(node.attributes.id); - } + removedDefIds.add(node.attributes.id); } detachNodeFromParent(node, parentNode); @@ -438,10 +427,9 @@ exports.fn = (root, params) => { } // Remove empty defs - for (const node of affectedDefs) { + for (const [node, parentNode] of allDefs.entries()) { if (node.children.length === 0) { - const parentNode = allDefs.get(node); - detachNodeFromParent(node, parentNode) + detachNodeFromParent(node, parentNode); } } }, diff --git a/test/plugins/removeHiddenElems.14.svg b/test/plugins/removeHiddenElems.14.svg index 17def4b78..a87a84bc2 100644 --- a/test/plugins/removeHiddenElems.14.svg +++ b/test/plugins/removeHiddenElems.14.svg @@ -11,4 +11,4 @@ Remove unused defs @@@ - + From 2b93feb647c7d1756aac5cccf236303298310c52 Mon Sep 17 00:00:00 2001 From: Kendell R Date: Sat, 9 Dec 2023 13:25:37 -0800 Subject: [PATCH 7/8] remove a out of scope change --- plugins/removeHiddenElems.js | 1 - 1 file changed, 1 deletion(-) diff --git a/plugins/removeHiddenElems.js b/plugins/removeHiddenElems.js index 27a218b38..5f28e6863 100644 --- a/plugins/removeHiddenElems.js +++ b/plugins/removeHiddenElems.js @@ -120,7 +120,6 @@ exports.fn = (root, params) => { nonRenderedNodes.set(node, parentNode); return visitSkip; } - const computedStyle = computeStyle(stylesheet, node); // opacity="0" // From 4298044cde5f18e2cd63f510ab6d5d2c88694a34 Mon Sep 17 00:00:00 2001 From: Kendell R Date: Sat, 9 Dec 2023 14:18:27 -0800 Subject: [PATCH 8/8] Apply suggestions from code review Co-authored-by: Seth Falco --- plugins/removeHiddenElems.js | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/plugins/removeHiddenElems.js b/plugins/removeHiddenElems.js index 5f28e6863..38b4f7c40 100644 --- a/plugins/removeHiddenElems.js +++ b/plugins/removeHiddenElems.js @@ -81,7 +81,7 @@ exports.fn = (root, params) => { const allDefs = new Map(); /** - * @type {Map>} + * @type {Map>} */ const referencesById = new Map(); @@ -396,7 +396,6 @@ exports.fn = (root, params) => { }, root: { exit: () => { - // Remove uses of deleted definitions for (const id of removedDefIds) { const refs = referencesById.get(id); if (refs) { @@ -406,7 +405,6 @@ exports.fn = (root, params) => { } } - // Remove definitions that are unused if (!deoptimized) { for (const [ nonRenderedNode, @@ -425,7 +423,6 @@ exports.fn = (root, params) => { } } - // Remove empty defs for (const [node, parentNode] of allDefs.entries()) { if (node.children.length === 0) { detachNodeFromParent(node, parentNode);