fix(): Parse use directive attribute issues

CHANGELOG.md

@@ -2,6 +2,7 @@
 
 ## [next]
 
+- fix(): Parse use directive attribute issues [#10053](https://github.com/fabricjs/fabric.js/pull/10053)
 - fix(parseUseDirectives): Fix style tag processing in use tag when reference also has a style [#10050](https://github.com/fabricjs/fabric.js/pull/10050)
 - fix(): Fix path Arc parsing regression issue [#10048](https://github.com/fabricjs/fabric.js/pull/10048)
 - chore(TS): Update TS to latest [#10044](https://github.com/fabricjs/fabric.js/pull/10044)

src/parser/parseStyleString.ts

@@ -12,6 +12,7 @@ export function parseStyleString(
     .replace(/;\s*$/, '')
     .split(';')
     .forEach((chunk) => {
+      if (!chunk) return;
       const [attr, value] = chunk.split(':');
       oStyle[attr.trim().toLowerCase()] = value.trim();
     });

src/parser/parseUseDirectives.test.ts

@@ -74,4 +74,40 @@ describe('parseUseDirectives', () => {
       expect(style1).toContain('fill:#ff0000');
     }
   });
+  it('correctly merge styles tags considering attributes', async () => {
+    const str = `<svg id="svg" viewBox="0 0 256 256" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">
+      <path fill="red" id="heart" d="M10,30 A20,20,0,0,1,50,30 A20,20,0,0,1,90,30 Q90,60,50,90 Q10,60,10,30 Z" />
+      <use x="100" y="0" xlink:href="#heart" style="stroke:#000000;fill:green" />
+      </svg>`;
+
+    const parser = new (getFabricWindow().DOMParser)();
+    const doc = parser.parseFromString(str.trim(), 'text/xml');
+    parseUseDirectives(doc);
+
+    const elements = Array.from(doc.documentElement.getElementsByTagName('*'));
+    expect(elements[0]).not.toBeNull();
+    expect(elements[1]).not.toBeNull();
+    if (elements[1] !== null) {
+      const style1 = elements[1].getAttribute('style');
+      expect(style1).toContain('fill:red');
+    }
+  });
+  it('Will not override existing attributes', async () => {
+    const str = `<svg id="svg" viewBox="0 0 256 256" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">
+      <path fill="yellow" id="heart" d="M10,30 A20,20,0,0,1,50,30 A20,20,0,0,1,90,30 Q90,60,50,90 Q10,60,10,30 Z" />
+      <use x="100" y="0" xlink:href="#heart" fill="blue" />
+      </svg>`;
+
+    const parser = new (getFabricWindow().DOMParser)();
+    const doc = parser.parseFromString(str.trim(), 'text/xml');
+    parseUseDirectives(doc);
+
+    const elements = Array.from(doc.documentElement.getElementsByTagName('*'));
+    expect(elements[0]).not.toBeNull();
+    expect(elements[1]).not.toBeNull();
+    if (elements[1] !== null) {
+      const style1 = elements[1].getAttribute('fill');
+      expect(style1).toBe('yellow');
+    }
+  });
 });

src/parser/parseUseDirectives.ts

@@ -5,91 +5,87 @@ import { parseStyleString } from './parseStyleString';
 
 export function parseUseDirectives(doc: Document) {
   const nodelist = getMultipleNodes(doc, ['use', 'svg:use']);
-  let i = 0;
-  while (nodelist.length && i < nodelist.length) {
-    const el = nodelist[i],
-      xlinkAttribute = el.getAttribute('xlink:href') || el.getAttribute('href');
+  const skipAttributes = ['x', 'y', 'xlink:href', 'href', 'transform'];
 
-    if (xlinkAttribute === null) {
-      return;
+  for (const useElement of nodelist) {
+    const useAttributes: NamedNodeMap = useElement.attributes;
+
+    const useAttrMap: Record<string, string> = {};
+    for (const attr of useAttributes) {
+      attr.value && (useAttrMap[attr.name] = attr.value);
     }
 
-    const xlink = xlinkAttribute.slice(1);
-    const x = el.getAttribute('x') || 0;
-    const y = el.getAttribute('y') || 0;
-    const el2Orig = doc.getElementById(xlink);
-    if (el2Orig === null) {
+    const xlink = (useAttrMap['xlink:href'] || useAttrMap.href || '').slice(1);
+
+    if (xlink === '') {
+      return;
+    }
+    const referencedElement = doc.getElementById(xlink);
+    if (referencedElement === null) {
       // if we can't find the target of the xlink, consider this use tag bad, similar to no xlink
       return;
     }
-    let el2 = el2Orig.cloneNode(true) as Element;
-    let currentTrans =
-      (el2.getAttribute('transform') || '') +
-      ' translate(' +
-      x +
-      ', ' +
-      y +
-      ')';
-    const oldLength = nodelist.length;
-    const namespace = svgNS;
+    let clonedOriginal = referencedElement.cloneNode(true) as Element;
 
-    applyViewboxTransform(el2);
-    if (/^svg$/i.test(el2.nodeName)) {
-      const el3 = el2.ownerDocument.createElementNS(namespace, 'g');
-      for (
-        let j = 0, attrs = el2.attributes, len = attrs.length;
-        j < len;
-        j++
-      ) {
-        const attr: Attr | null = attrs.item(j);
-        attr && el3.setAttributeNS(namespace, attr.nodeName, attr.nodeValue!);
-      }
-      // el2.firstChild != null
-      while (el2.firstChild) {
-        el3.appendChild(el2.firstChild);
-      }
-      el2 = el3;
+    const originalAttributes: NamedNodeMap = clonedOriginal.attributes;
+
+    const originalAttrMap: Record<string, string> = {};
+    for (const attr of originalAttributes) {
+      attr.value && (originalAttrMap[attr.name] = attr.value);
+    }
+
+    // Transform attribute needs to be merged in a particular way
+    const { x = 0, y = 0, transform = '' } = useAttrMap;
+    const currentTrans = `${transform} ${
+      originalAttrMap.transform || ''
+    } translate(${x}, ${y})`;
+
+    applyViewboxTransform(clonedOriginal);
+
+    if (/^svg$/i.test(clonedOriginal.nodeName)) {
+      // if is an SVG, create a group and apply all the attributes on top of it
+      const el3 = clonedOriginal.ownerDocument.createElementNS(svgNS, 'g');
+      Object.entries(originalAttrMap).forEach(([name, value]) =>
+        el3.setAttributeNS(svgNS, name, value)
+      );
+      el3.append(...clonedOriginal.childNodes);
+      clonedOriginal = el3;
     }
 
-    for (let j = 0, attrs = el.attributes, len = attrs.length; j < len; j++) {
-      const attr = attrs.item(j);
+    for (const attr of useAttributes) {
       if (!attr) {
         continue;
       }
-      const { nodeName, nodeValue } = attr;
-      if (
-        nodeName === 'x' ||
-        nodeName === 'y' ||
-        nodeName === 'xlink:href' ||
-        nodeName === 'href'
-      ) {
+      const { name, value } = attr;
+      if (skipAttributes.includes(name)) {
         continue;
       }
 
-      if (nodeName === 'transform') {
-        currentTrans = nodeValue + ' ' + currentTrans;
-      } else if (nodeName === 'style' && el2.getAttribute('style') !== null) {
-        // when both sides have styles, merge the two styles, with the ref being priority (not use)
+      if (name === 'style') {
+        // when use has a style, merge the two styles, with the ref being priority (not use)
+        // priority is by feature. an attribute for fill on the original element
+        // will overwrite the fill in style or attribute for tha use
         const styleRecord: Record<string, any> = {};
-        parseStyleString(nodeValue!, styleRecord);
-        parseStyleString(el2.getAttribute('style')!, styleRecord);
+        parseStyleString(value!, styleRecord);
+        // cleanup styleRecord from attributes of original
+        Object.entries(originalAttrMap).forEach(([name, value]) => {
+          styleRecord[name] = value;
+        });
+        // now we can put in the style of the original that will overwrite the original attributes
+        parseStyleString(originalAttrMap.style || '', styleRecord);
         const mergedStyles = Object.entries(styleRecord)
           .map((entry) => entry.join(':'))
           .join(';');
-        el2.setAttribute(nodeName, mergedStyles);
+        clonedOriginal.setAttribute(name, mergedStyles);
       } else {
-        el2.setAttribute(nodeName, nodeValue!);
+        // set the attribute from use element only if the original does not have it already
+        !originalAttrMap[name] && clonedOriginal.setAttribute(name, value!);
       }
     }
 
-    el2.setAttribute('transform', currentTrans);
-    el2.setAttribute('instantiated_by_use', '1');
-    el2.removeAttribute('id');
-    const parentNode = el.parentNode;
-    parentNode!.replaceChild(el2, el);
-    // some browsers do not shorten nodelist after replaceChild (IE8)
-    if (nodelist.length === oldLength) {
-      i++;
-    }
+    clonedOriginal.setAttribute('transform', currentTrans);
+    clonedOriginal.setAttribute('instantiated_by_use', '1');
+    clonedOriginal.removeAttribute('id');
+    useElement.parentNode!.replaceChild(clonedOriginal, useElement);
   }
 }

test/visual/svg_import.js

@@ -36,7 +36,7 @@
       test: 'Svg import test ' + svgName,
       code: test,
       golden: svgName + '.png',
-      percentage: 0.06,
+      percentage: 0.055,
     };
   }
 
@@ -101,6 +101,8 @@
     'accordion',
     'car',
     'seaClipPath',
+    'use-and-style',
+    'use-svg-style-2',
   ].map(createTestFromSVG);
 
   tests.forEach(visualTestLoop(QUnit));