From f3495ff6c9906f2b831f13b7ac36f2dc305fa37d Mon Sep 17 00:00:00 2001 From: Seth Falco Date: Sun, 22 Dec 2024 16:01:22 +0000 Subject: [PATCH] fix(removeEmptyContainers): skip if filter is applied via styles as well (#2089) Fixes a bug where we were too eager to remove empty containers. We already had logic to skip removing empty containers if it had the filter attribute, which is needed to apply a filter to the whole area. However, the filter can also be defined through CSS. We did not properly handle this case, and treated the node as if it had no filter at all. This computes the styles and checks the stylesheet as well. (We also move the logic down to avoid computing the styles eagerly.) --- plugins/removeEmptyContainers.js | 22 ++++++++++----- test/browser.js | 6 +++-- test/plugins/removeEmptyContainers.07.svg.txt | 27 +++++++++++++++++++ test/regression.js | 21 ++++++++------- yarn.lock | 9 +------ 5 files changed, 59 insertions(+), 26 deletions(-) create mode 100644 test/plugins/removeEmptyContainers.07.svg.txt diff --git a/plugins/removeEmptyContainers.js b/plugins/removeEmptyContainers.js index cb4ce0c0f..5eeeac2da 100644 --- a/plugins/removeEmptyContainers.js +++ b/plugins/removeEmptyContainers.js @@ -1,5 +1,6 @@ import { elemsGroups } from './_collections.js'; import { detachNodeFromParent } from '../lib/xast.js'; +import { collectStylesheet, computeStyle } from '../lib/style.js'; export const name = 'removeEmptyContainers'; export const description = 'removes empty container elements'; @@ -19,7 +20,9 @@ export const description = 'removes empty container elements'; * * @type {import('./plugins-types.js').Plugin<'removeEmptyContainers'>} */ -export const fn = () => { +export const fn = (root) => { + const stylesheet = collectStylesheet(root); + return { element: { exit: (node, parentNode) => { @@ -38,11 +41,7 @@ export const fn = () => { ) { return; } - // The may not have content, but the filter may cause a rectangle - // to be created and filled with pattern. - if (node.name === 'g' && node.attributes.filter != null) { - return; - } + // empty hides masked element if (node.name === 'mask' && node.attributes.id != null) { return; @@ -50,6 +49,17 @@ export const fn = () => { if (parentNode.type === 'element' && parentNode.name === 'switch') { return; } + + // The may not have content, but the filter may cause a rectangle + // to be created and filled with pattern. + if ( + node.name === 'g' && + (node.attributes.filter != null || + computeStyle(stylesheet, node).filter) + ) { + return; + } + detachNodeFromParent(node, parentNode); }, }, diff --git a/test/browser.js b/test/browser.js index 50560555f..05d66eb3e 100644 --- a/test/browser.js +++ b/test/browser.js @@ -5,6 +5,8 @@ import path from 'path'; import { fileURLToPath } from 'url'; import { chromium } from 'playwright'; +const PORT = 5001; + const __dirname = path.dirname(fileURLToPath(import.meta.url)); const pkgPath = path.join(__dirname, '../package.json'); const { version } = JSON.parse(await fs.readFile(pkgPath, 'utf-8')); @@ -58,7 +60,7 @@ const runTest = async () => { const browser = await chromium.launch(); const context = await browser.newContext(); const page = await context.newPage(); - await page.goto('http://localhost:5000'); + await page.goto(`http://localhost:${PORT}`); const actual = await page.evaluate(() => ({ version: globalThis.version, @@ -83,7 +85,7 @@ const runTest = async () => { await browser.close(); }; -server.listen(5000, async () => { +server.listen(PORT, async () => { try { await runTest(); console.info('Tested successfully'); diff --git a/test/plugins/removeEmptyContainers.07.svg.txt b/test/plugins/removeEmptyContainers.07.svg.txt new file mode 100644 index 000000000..d3e6f876d --- /dev/null +++ b/test/plugins/removeEmptyContainers.07.svg.txt @@ -0,0 +1,27 @@ +Empty nodes should not be removed if they contain a filter, including +filters applied via CSS. + +=== + + + + + + + + + •ᴗ• + + + +@@@ + + + + + + + + + •ᴗ• + diff --git a/test/regression.js b/test/regression.js index cf6cf9d78..aeefb8517 100644 --- a/test/regression.js +++ b/test/regression.js @@ -15,13 +15,14 @@ import { optimize } from '../lib/svgo.js'; const __dirname = path.dirname(fileURLToPath(import.meta.url)); -const width = 960; -const height = 720; +const PORT = 5001; +const WIDTH = 960; +const HEIGHT = 720; /** @type {PageScreenshotOptions} */ const screenshotOptions = { omitBackground: true, - clip: { x: 0, y: 0, width, height }, + clip: { x: 0, y: 0, width: WIDTH, height: HEIGHT }, animations: 'disabled', }; @@ -38,21 +39,21 @@ const runTests = async (list) => { * @param {string} name */ const processFile = async (page, name) => { - await page.goto(`http://localhost:5000/original/${name}`); + await page.goto(`http://localhost:${PORT}/original/${name}`); const originalBuffer = await page.screenshot(screenshotOptions); - await page.goto(`http://localhost:5000/optimized/${name}`); + await page.goto(`http://localhost:${PORT}/optimized/${name}`); const optimizedBufferPromise = page.screenshot(screenshotOptions); const writeDiffs = process.env.NO_DIFF == null; - const diff = writeDiffs && new PNG({ width, height }); + const diff = writeDiffs && new PNG({ width: WIDTH, height: HEIGHT }); const originalPng = PNG.sync.read(originalBuffer); const optimizedPng = PNG.sync.read(await optimizedBufferPromise); const matched = pixelmatch( originalPng.data, optimizedPng.data, diff ? diff.data : null, - width, - height, + WIDTH, + HEIGHT, ); // ignore small aliasing issues if (matched <= 4) { @@ -83,7 +84,7 @@ const runTests = async (list) => { const browser = await chromium.launch(); const context = await browser.newContext({ javaScriptEnabled: false, - viewport: { width, height }, + viewport: { width: WIDTH, height: HEIGHT }, }); await Promise.all( Array.from(new Array(os.cpus().length * 2), () => worker()), @@ -126,7 +127,7 @@ const runTests = async (list) => { throw new Error(`unknown path ${req.url}`); }); await new Promise((resolve) => { - server.listen(5000, resolve); + server.listen(PORT, resolve); }); const list = (await filesPromise).filter((name) => name.endsWith('.svg')); const passed = await runTests(list); diff --git a/yarn.lock b/yarn.lock index 0eaee2d9c..4072245aa 100644 --- a/yarn.lock +++ b/yarn.lock @@ -3830,14 +3830,7 @@ __metadata: languageName: node linkType: hard -"picocolors@npm:^1.0.0": - version: 1.0.0 - resolution: "picocolors@npm:1.0.0" - checksum: a2e8092dd86c8396bdba9f2b5481032848525b3dc295ce9b57896f931e63fc16f79805144321f72976383fc249584672a75cc18d6777c6b757603f372f745981 - languageName: node - linkType: hard - -"picocolors@npm:^1.1.1": +"picocolors@npm:^1.0.0, picocolors@npm:^1.1.1": version: 1.1.1 resolution: "picocolors@npm:1.1.1" checksum: e1cf46bf84886c79055fdfa9dcb3e4711ad259949e3565154b004b260cd356c5d54b31a1437ce9782624bf766272fe6b0154f5f0c744fb7af5d454d2b60db045