fix(): Parse use directive attribute issues (#10053)

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