Refactor SVG stroke width/bounds calculation

package.json

@@ -39,6 +39,7 @@
     "json": "^9.0.6",
     "lodash.defaultsdeep": "4.6.1",
     "mkdirp": "^1.0.3",
+    "mock-require": "^3.0.3",
     "rimraf": "^3.0.1",
     "tap": "^11.0.1",
     "webpack": "^4.8.0",

src/svg-renderer.js

@@ -305,18 +305,27 @@ class SvgRenderer {
      * @return {number} The largest stroke width in the SVG.
      */
     _findLargestStrokeWidth (rootNode) {
+        // Per SVG spec, the 'stroke' attribute only applies to shapes and text content elements:
+        // https://www.w3.org/TR/SVG11/painting.html#StrokeProperty
+        const STROKABLE_ELEMENTS = new Set([
+            // Shape elements (https://www.w3.org/TR/SVG11/intro.html#TermShape)
+            'path', 'rect', 'circle', 'ellipse', 'line', 'polyline', 'polygon',
+            // Text content elements (https://www.w3.org/TR/SVG11/intro.html#TermTextContentElement)
+            'altGlyph', 'textPath', 'text', 'tref', 'tspan'
+        ]);
+
         let largestStrokeWidth = 0;
         const collectStrokeWidths = domElement => {
-            if (domElement.getAttribute) {
-                if (domElement.getAttribute('stroke')) {
-                    largestStrokeWidth = Math.max(largestStrokeWidth, 1);
-                }
-                if (domElement.getAttribute('stroke-width')) {
-                    largestStrokeWidth = Math.max(
-                        largestStrokeWidth,
-                        Number(domElement.getAttribute('stroke-width')) || 0
-                    );
-                }
+            if (
+                STROKABLE_ELEMENTS.has(domElement.localName) &&
+                domElement.getAttribute &&
+                domElement.getAttribute('stroke') &&
+                domElement.getAttribute('stroke') !== 'none'
+            ) {
+                const strokeWidthAttr = domElement.getAttribute('stroke-width');
+                // Stroke width is 1 if unset.
+                const strokeWidth = Number(strokeWidthAttr) || 1;
+                largestStrokeWidth = Math.max(largestStrokeWidth, strokeWidth);
             }
             for (let i = 0; i < domElement.childNodes.length; i++) {
                 collectStrokeWidths(domElement.childNodes[i]);
@@ -381,14 +390,8 @@ class SvgRenderer {
         // Enlarge the bbox from the largest found stroke width
         // This may have false-positives, but at least the bbox will always
         // contain the full graphic including strokes.
-        // If the width or height is zero however, don't enlarge since
-        // they won't have a stroke width that needs to be enlarged.
-        let halfStrokeWidth;
-        if (bbox.width === 0 || bbox.height === 0) {
-            halfStrokeWidth = 0;
-        } else {
-            halfStrokeWidth = this._findLargestStrokeWidth(this._svgTag) / 2;
-        }
+        const halfStrokeWidth = this._findLargestStrokeWidth(this._svgTag) / 2;
+
         const width = bbox.width + (halfStrokeWidth * 2);
         const height = bbox.height + (halfStrokeWidth * 2);
         const x = bbox.x - halfStrokeWidth;

test/stroke-width.js

@@ -0,0 +1,118 @@
+const test = require('tap').test;
+const jsdom = require('jsdom');
+const {JSDOM} = jsdom;
+
+const mockRequire = require('mock-require');
+// The font inliner uses Webpack loader require syntax, which doesn't work in Node.
+mockRequire('../src/font-inliner', () => {});
+
+const SvgRenderer = require('../src/svg-renderer');
+
+const {window} = new JSDOM();
+// The SvgRenderer constructor tries to get a canvas' context, which doesn't work in JSDOM
+window.HTMLCanvasElement.prototype.getContext = () => {};
+global.window = window;
+global.document = window.document;
+global.DOMParser = window.DOMParser;
+const parser = new window.DOMParser();
+
+const renderer = new SvgRenderer();
+
+const parseSVGString = svgString => parser.parseFromString(svgString, 'image/svg+xml').documentElement;
+
+test('stroke-width set to maximum', t => {
+    const svg = parseSVGString(`
+    <svg xmlns="http://www.w3.org/2000/svg" version="1.1" viewBox="0 0 256 256" height="256" width="256">
+        <circle r="128" cy="128" cx="128" fill="red" fill-rule="nonzero" stroke="black" stroke-width="16" />
+        <circle r="128" cy="128" cx="128" fill="blue" fill-rule="nonzero" stroke="black" stroke-width="32" />
+        <circle r="128" cy="128" cx="128" fill="green" fill-rule="nonzero" stroke="black" stroke-width="48" />
+    </svg>
+    `);
+
+    const largestStrokeWidth = renderer._findLargestStrokeWidth(svg);
+
+    t.equals(largestStrokeWidth, 48, 'stroke-width is set to largest value');
+    t.end();
+});
+
+
+test('stroke-width unset', t => {
+    const svg = parseSVGString(`
+    <svg xmlns="http://www.w3.org/2000/svg" version="1.1" viewBox="0 0 256 256" height="256" width="256">
+        <circle r="128" cy="128" cx="128" fill="red" fill-rule="nonzero" stroke="black" />
+    </svg>
+    `);
+
+    const largestStrokeWidth = renderer._findLargestStrokeWidth(svg);
+
+    t.equals(largestStrokeWidth, 1, 'stroke-width is 1 by default');
+    t.end();
+});
+
+test('stroke set to none', t => {
+    const svg = parseSVGString(`
+    <svg xmlns="http://www.w3.org/2000/svg" version="1.1" viewBox="0 0 256 256" height="256" width="256">
+        <circle r="128" cy="128" cx="128" fill="red" fill-rule="nonzero" stroke="none" stroke-width="32" />
+        <circle r="128" cy="128" cx="128" fill="red" fill-rule="nonzero" stroke="black" stroke-width="16" />
+    </svg>
+    `);
+
+    const largestStrokeWidth = renderer._findLargestStrokeWidth(svg);
+
+    t.equals(largestStrokeWidth, 16, 'stroke="none" doesn\'t affect width');
+    t.end();
+});
+
+test('stroke-width below 1', t => {
+    const svg = parseSVGString(`
+    <svg xmlns="http://www.w3.org/2000/svg" version="1.1" viewBox="0 0 256 256" height="256" width="256">
+        <circle r="128" cy="128" cx="128" fill="red" fill-rule="nonzero" stroke="black" stroke-width="0.5" />
+    </svg>
+    `);
+
+    const largestStrokeWidth = renderer._findLargestStrokeWidth(svg);
+
+    t.equals(largestStrokeWidth, 0.5, 'stroke-width is 0.5');
+    t.end();
+});
+
+test('stroke-width but no stroke', t => {
+    const svg = parseSVGString(`
+    <svg xmlns="http://www.w3.org/2000/svg" version="1.1" viewBox="0 0 256 256" height="256" width="256">
+        <circle r="128" cy="128" cx="128" fill="red" fill-rule="nonzero" stroke="black" stroke-width="16" />
+        <circle r="128" cy="128" cx="128" fill="white" fill-rule="nonzero" stroke-width="32" />
+    </svg>
+    `);
+
+    const largestStrokeWidth = renderer._findLargestStrokeWidth(svg);
+
+    t.equals(largestStrokeWidth, 16, 'stroke-width is ignored when element has no stroke attribute');
+    t.end();
+});
+
+test('stroke-width set to invalid value', t => {
+    const svg = parseSVGString(`
+    <svg xmlns="http://www.w3.org/2000/svg" version="1.1" viewBox="0 0 256 256" height="256" width="256">
+        <circle r="128" cy="128" cx="128" fill="red" fill-rule="nonzero" stroke="black" stroke-width="wrong" />
+    </svg>
+    `);
+
+    const largestStrokeWidth = renderer._findLargestStrokeWidth(svg);
+
+    t.equals(largestStrokeWidth, 1, 'invalid stroke-width defaults to 1');
+    t.end();
+});
+
+test('stroke-width on the wrong elements', t => {
+    const svg = parseSVGString(`
+    <svg xmlns="http://www.w3.org/2000/svg" version="1.1" viewBox="0 0 256 256" height="256" width="256">
+          <image href="" stroke="black" stroke-width="64" />
+          <circle r="128" cy="128" cx="128" fill="white" fill-rule="nonzero" stroke="black" stroke-width="16" />
+    </svg>
+    `);
+
+    const largestStrokeWidth = renderer._findLargestStrokeWidth(svg);
+
+    t.equals(largestStrokeWidth, 16, 'stroke-width is ignored when applied to elements that cannot have a stroke');
+    t.end();
+});