Repository: fabricjs/fabric.js
Pull request #10050 (2024-08-10 17:41:01)

fix(): svg parsing logic with use and style
## Description

style tags between a path and use tag don't seem to work properly compared to Chrome. In Chrome, when both have a style, the path has priority but merges with the use tag where they do not share styles. Previously, fabric would take the use tag's style wholesale. This PR merges the two when both exist and tests were added to verify the both case and cases where only one has a style.

Closes https://github.com/fabricjs/fabric.js/issues/9840


codesandbox[bot]: #### Review or Edit in CodeSandbox

Open the branch in <a href="https://codesandbox.io/p/github/gloriousjob/fabric.js/ISSUE-9840-svg-parsing-error-with-use-and-style?mode=review&utm_source=gh_app">Web Editor</a> &bull; <a href="https://codesandbox.io/p/vscode?owner=gloriousjob&repo=fabric.js&branch=ISSUE-9840-svg-parsing-error-with-use-and-style&utm_source=gh_app">VS Code</a> &bull; <a href="https://codesandbox.io/p/vscode?owner=gloriousjob&repo=fabric.js&branch=ISSUE-9840-svg-parsing-error-with-use-and-style&insiders=true&utm_source=gh_app">Insiders</a><br>
Open <a href="https://codesandbox.io/p/devtool/preview/gloriousjob/fabric.js/ISSUE-9840-svg-parsing-error-with-use-and-style?task=preview&port=51423&redirect=true&utm_source=gh_app">Preview</a>

<!-- open-in-codesandbox:complete -->


Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 3c3d09c9f..7c616467c 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -2,6 +2,7 @@
 
 ## [next]
 
+- 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)
 - feat(): Add has method to classRegistry to allow to check if a class exists. (fixes #10001)
diff --git a/lib/aligning_guidelines.js b/lib/aligning_guidelines.js
index 495f1a4fa..25cbcc875 100644
--- a/lib/aligning_guidelines.js
+++ b/lib/aligning_guidelines.js
@@ -89,6 +89,79 @@ function initAligningGuidelines(canvas) {
           objectHeight = objectBoundingRect.height / viewportTransform[3],
           objectWidth = objectBoundingRect.width / viewportTransform[0];
 
+
+      // snaps if the right side of the active object touches the left side of the object
+      if (isInRange(activeObjectLeft + activeObjectWidth / 2, objectLeft - objectWidth / 2)) {
+
+        verticalInTheRange = true;
+        verticalLines.push({
+          x: objectLeft - objectWidth / 2,
+          y1: (objectTop < activeObjectTop)
+            ? (objectTop - objectHeight / 2 - aligningLineOffset)
+            : (objectTop + objectHeight / 2 + aligningLineOffset),
+          y2: (activeObjectTop > objectTop)
+            ? (activeObjectTop + activeObjectHeight / 2 + aligningLineOffset)
+            : (activeObjectTop - activeObjectHeight / 2 - aligningLineOffset)
+        });
+
+        activeObject.setPositionByOrigin(new fabric.Point(objectLeft - objectWidth / 2 - activeObjectWidth / 2, activeObjectTop), 'center', 'center');
+      }
+
+
+
+      // snaps if the left side of the active object touches the right side of the object
+      if (isInRange(activeObjectLeft - activeObjectWidth / 2, objectLeft + objectWidth / 2)) {
+
+        verticalInTheRange = true;
+        verticalLines.push({
+            x: objectLeft + objectWidth / 2,
+            y1: (objectTop < activeObjectTop)
+                ? (objectTop - objectHeight / 2 - aligningLineOffset)
+                : (objectTop + objectHeight / 2 + aligningLineOffset),
+            y2: (activeObjectTop > objectTop)
+                ? (activeObjectTop + activeObjectHeight / 2 + aligningLineOffset)
+                : (activeObjectTop - activeObjectHeight / 2 - aligningLineOffset)
+        });
+
+        activeObject.setPositionByOrigin(new fabric.Point(objectLeft + objectWidth / 2 + activeObjectWidth / 2, activeObjectTop), 'center', 'center');
+      }
+
+
+      // snaps if the bottom of the object touches the top of the active object
+      if (isInRange(objectTop + objectHeight / 2, activeObjectTop - activeObjectHeight / 2)) {
+
+        horizontalInTheRange = true;
+        horizontalLines.push({
+            y: objectTop + objectHeight / 2,
+            x1: (objectLeft < activeObjectLeft)
+            ? (objectLeft - objectWidth / 2 - aligningLineOffset)
+            : (objectLeft + objectWidth / 2 + aligningLineOffset),
+            x2: (activeObjectLeft > objectLeft)
+            ? (activeObjectLeft + activeObjectWidth / 2 + aligningLineOffset)
+            : (activeObjectLeft - activeObjectWidth / 2 - aligningLineOffset)
+        });
+
+        activeObject.setPositionByOrigin(new fabric.Point(activeObjectLeft, objectTop + objectHeight / 2 + activeObjectHeight / 2), 'center', 'center');
+      }
+
+
+      // snaps if the top of the object touches the bottom of the active object
+      if (isInRange(objectTop - objectHeight / 2, activeObjectTop + activeObjectHeight / 2)) {
+
+        horizontalInTheRange = true;
+        horizontalLines.push({
+          y: objectTop - objectHeight / 2,
+          x1: (objectLeft < activeObjectLeft)
+            ? (objectLeft - objectWidth / 2 - aligningLineOffset)
+            : (objectLeft + objectWidth / 2 + aligningLineOffset),
+          x2: (activeObjectLeft > objectLeft)
+            ? (activeObjectLeft + activeObjectWidth / 2 + aligningLineOffset)
+            : (activeObjectLeft - activeObjectWidth / 2 - aligningLineOffset)
+        });
+
+        activeObject.setPositionByOrigin(new fabric.Point(activeObjectLeft, objectTop - objectHeight / 2 - activeObjectHeight / 2), 'center', 'center');
+      }
+
       // snap by the horizontal center line
       if (isInRange(objectLeft, activeObjectLeft)) {
         verticalInTheRange = true;
@@ -190,7 +263,9 @@ function initAligningGuidelines(canvas) {
   });
 
   canvas.on('before:render', function() {
-    canvas.clearContext(canvas.contextTop);
+    if (canvas.contextTop) {
+      canvas.clearContext(canvas.contextTop);
+    }
   });
 
   canvas.on('after:render', function() {
diff --git a/lib/centering_guidelines.js b/lib/centering_guidelines.js
index 5c6f6c8ed..761e781a4 100644
--- a/lib/centering_guidelines.js
+++ b/lib/centering_guidelines.js
@@ -68,7 +68,9 @@ function initCenteringGuidelines(canvas) {
   });
 
   canvas.on('before:render', function() {
-    canvas.clearContext(canvas.contextTop);
+    if (canvas.contextTop) {
+      canvas.clearContext(canvas.contextTop);
+    }
   });
 
   canvas.on('after:render', function() {
diff --git a/src/parser/parseUseDirectives.ts b/src/parser/parseUseDirectives.ts
index 89435b5dd..520ca1e7b 100644
--- a/src/parser/parseUseDirectives.ts
+++ b/src/parser/parseUseDirectives.ts
@@ -1,6 +1,7 @@
 import { svgNS } from './constants';
 import { getMultipleNodes } from './getMultipleNodes';
 import { applyViewboxTransform } from './applyViewboxTransform';
+import { parseStyleString } from './parseStyleString';
 
 export function parseUseDirectives(doc: Document) {
   const nodelist = getMultipleNodes(doc, ['use', 'svg:use']);
@@ -67,6 +68,15 @@ export function parseUseDirectives(doc: Document) {
 
       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)
+        const styleRecord: Record<string, any> = {};
+        parseStyleString(nodeValue!, styleRecord);
+        parseStyleString(el2.getAttribute('style')!, styleRecord);
+        const mergedStyles = Object.entries(styleRecord)
+          .map((entry) => entry.join(':'))
+          .join(';');
+        el2.setAttribute(nodeName, mergedStyles);
       } else {
         el2.setAttribute(nodeName, nodeValue!);
       }


Test patch:
diff --git a/src/parser/parseUseDirectives.test.ts b/src/parser/parseUseDirectives.test.ts
new file mode 100644
index 000000000..495baf53d
--- /dev/null
+++ b/src/parser/parseUseDirectives.test.ts
@@ -0,0 +1,77 @@
+import { getFabricWindow } from '../env';
+import { parseUseDirectives } from './parseUseDirectives';
+
+describe('parseUseDirectives', () => {
+  it('returns successful parse where use tag uses fill style prioritizing path tag when both tags have a style', 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 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"
+        style="stroke:#000000;fill:#ff0000" />
+      <use x="100" y="0" xlink:href="#heart" style="stroke-width:5.0;fill:#0000ff" />
+      </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();
+    if (elements[0] !== null) {
+      const style0 = elements[0].getAttribute('style');
+      expect(style0).toContain('fill:#ff0000');
+    }
+    expect(elements[1]).not.toBeNull();
+    if (elements[1] !== null) {
+      const style1 = elements[1].getAttribute('style');
+      expect(style1).toContain('fill:#ff0000');
+      // also contains extra style that path tag does not have
+      expect(style1).toContain('stroke-width:5.0');
+    }
+  });
+  it('returns successful parse where use tag uses fill style from itself when path tag empty', 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 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-width:5.0;fill:#0000ff" />
+      </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();
+    if (elements[0] !== null) {
+      const style0 = elements[0].getAttribute('style');
+      expect(style0).toBeNull();
+    }
+    expect(elements[1]).not.toBeNull();
+    if (elements[1] !== null) {
+      const style1 = elements[1].getAttribute('style');
+      expect(style1).toContain('fill:#0000ff');
+      // also contains extra style that path tag does not have
+      expect(style1).toContain('stroke-width:5.0');
+    }
+  });
+  it('returns successful parse where use tag uses fill style from path when its style tag is empty', 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 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" 
+        style="stroke:#000000;fill:#ff0000" />
+      <use x="100" y="0" xlink:href="#heart" />
+      </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();
+    if (elements[0] !== null) {
+      const style0 = elements[0].getAttribute('style');
+      expect(style0).toContain('fill:#ff0000');
+    }
+    expect(elements[1]).not.toBeNull();
+    if (elements[1] !== null) {
+      const style1 = elements[1].getAttribute('style');
+      expect(style1).toContain('fill:#ff0000');
+    }
+  });
+});
diff --git a/test/visual/assets/use-and-style.svg b/test/visual/assets/use-and-style.svg
new file mode 100644
index 000000000..461aa9aaf
--- /dev/null
+++ b/test/visual/assets/use-and-style.svg
@@ -0,0 +1,24 @@
+<svg id="svg" viewBox="0 0 200 360" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">
+    <path 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"
+      style="stroke:#00FF00;fill:#ff0000" />
+    <use x="100" y="0" xlink:href="#heart"
+      style="stroke-width:8.0;fill:#0000ff" />
+    <g transform="translate(0, 100)">
+      <path id="heart2" 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="#heart2"
+        style="stroke-width:5.0;fill:#0000ff" />
+    </g>
+    <g transform="translate(0, 180)">
+      <path id="heart3" 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" 
+        style="stroke:#000000;fill:#ff0000" />
+      <use x="100" y="0" xlink:href="#heart3" />
+    </g>
+    <g transform="translate(0, 260)">
+      <path id="heart4" 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" 
+         fill="green" />
+      <use x="100" y="0" xlink:href="#heart4" fill="purple" stroke-width="8" stroke="yellow" />
+    </g>
+    <circle id="myCircle" cx="5" cy="5" r="4" stroke="blue" />
+    <use href="#myCircle" x="10" fill="blue" />
+    <use href="#myCircle" x="20" fill="white" stroke="red" />
+</svg>