fix(removeEmptyContainers): skip if filter is applied via styles as w…

…ell (#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

@@ -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,18 +41,25 @@ export const fn = () => {
         ) {
           return;
         }
-        // The <g> 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 <mask> hides masked element
         if (node.name === 'mask' && node.attributes.id != null) {
           return;
         }
         if (parentNode.type === 'element' && parentNode.name === 'switch') {
           return;
         }
+
+        // The <g> 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);
       },
     },

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');

test/plugins/removeEmptyContainers.07.svg.txt

@@ -0,0 +1,27 @@
+Empty <g> nodes should not be removed if they contain a filter, including
+filters applied via CSS.
+
+===
+
+<svg viewBox="0 0 50 50" xmlns="http://www.w3.org/2000/svg">
+    <filter id="a" x="0" y="0" width="50" height="50" filterUnits="userSpaceOnUse">
+        <feFlood flood-color="#aaa"/>
+    </filter>
+    <mask id="b" x="0" y="0" width="50" height="50">
+        <g style="filter: url(#a)"/>
+    </mask>
+    <text x="16" y="16" style="mask: url(#b)">•ᴗ•</text>
+</svg>
+
+
+@@@
+
+<svg viewBox="0 0 50 50" xmlns="http://www.w3.org/2000/svg">
+    <filter id="a" x="0" y="0" width="50" height="50" filterUnits="userSpaceOnUse">
+        <feFlood flood-color="#aaa"/>
+    </filter>
+    <mask id="b" x="0" y="0" width="50" height="50">
+        <g style="filter: url(#a)"/>
+    </mask>
+    <text x="16" y="16" style="mask: url(#b)">•ᴗ•</text>
+</svg>

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);

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