Repository: fabricjs/fabric.js
Pull request #9427 (2023-10-21 17:59:58)

Fix(SVGParser) ignore missing xlink target issue on svg parsing
## Motivation

@closes  https://github.com/fabricjs/fabric.js/issues/9109

## Description

Sometimes an svg has use tags with an xlink with no target, which most parsers ignore. We should ignore these use tags as well.

## Changes

Adds a null check and returns early, preventing an NPE.
Also removed elementById since it was needed for IE8 support, which we no longer support (see support for everything else here: https://developer.mozilla.org/en-US/docs/Web/API/Document/getElementById).

## In Action



gloriousjob: I didn't realize that was being used by the use tag (elements_parser apparently handles this). But I found an example and tested it. It worked normally and when I changed the id to a bad one, it ignored the clipping so looks like the code is already healthy for that.
 Only question I have now is how to get the 2 failures to pass. I'm not sure if this is because I'm using my own repo since I don't have write access on the official repo.
ShaMan123: > I didn't realize that was being used by the use tag (elements_parser apparently handles this). But I found an example and tested it. It worked normally and when I changed the id to a bad one, it ignored the clipping so looks like the code is already healthy for that.

Could you add a test?


>  Only question I have now is how to get the 2 check failures to pass. I'm not sure if this is because I'm using my own repo since I don't have write access on the official repo.

You are right, it is because of write access.

gloriousjob: > Could you add a test?

Tacked on additional test.
asturur: Do we have an svg with missing xlink and can we see how the browser renders it?
gloriousjob: > Do we have an svg with missing xlink and can we see how the browser renders it?

The svg from the unit test looks like:
![1](https://github.com/fabricjs/fabric.js/assets/9204009/289d5e02-6fe5-4e36-b739-3ae71f2d1f27)

Pretty simple but without the fix, nothing renders in fabric whereas it renders like this with the fix.
asturur: i moved the read shape into the defs element so we can test that the use element with the wrong clip-path still work without the clippath.
If the path is outside defs, you never know if the parser ignored the wrong clip-path attribute or ignored the whole `<use>` node
asturur: @ShaMan123 i m merging this since is a good fix and we can push it up with the beta right away, ok?
ShaMan123: sure

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index c4d3debed..46a57fe78 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -2,6 +2,7 @@
 
 ## [next]
 
+- fix(parser): fix svg's with use tags containing bad href's not loading [#9109](https://github.com/fabricjs/fabric.js/issues/9109)
 - fix(#9172): dep export `Object`, `Text`, `Image` [#9433](https://github.com/fabricjs/fabric.js/pull/9433)
 
 ## [6.0.0-beta13]
diff --git a/src/parser/elementById.ts b/src/parser/elementById.ts
deleted file mode 100644
index cf2dd1d9f..000000000
--- a/src/parser/elementById.ts
+++ /dev/null
@@ -1,18 +0,0 @@
-/**
- * @private
- * TODO: verify if this is still needed
- * to support IE8 missing getElementById on SVGdocument and on node xmlDOM
- */
-export function elementById(doc: Document, id: string): Element | null {
-  if (doc.getElementById) {
-    return doc.getElementById(id);
-  }
-  const nodelist = doc.getElementsByTagName('*');
-  for (let i = 0, len = nodelist.length; i < len; i++) {
-    const node = nodelist[i];
-    if (id === node.getAttribute('id')) {
-      return node;
-    }
-  }
-  return null;
-}
diff --git a/src/parser/parseUseDirectives.ts b/src/parser/parseUseDirectives.ts
index b7d69de9b..89435b5dd 100644
--- a/src/parser/parseUseDirectives.ts
+++ b/src/parser/parseUseDirectives.ts
@@ -1,5 +1,4 @@
 import { svgNS } from './constants';
-import { elementById } from './elementById';
 import { getMultipleNodes } from './getMultipleNodes';
 import { applyViewboxTransform } from './applyViewboxTransform';
 
@@ -17,7 +16,12 @@ export function parseUseDirectives(doc: Document) {
     const xlink = xlinkAttribute.slice(1);
     const x = el.getAttribute('x') || 0;
     const y = el.getAttribute('y') || 0;
-    let el2 = elementById(doc, xlink)!.cloneNode(true) as Element;
+    const el2Orig = doc.getElementById(xlink);
+    if (el2Orig === 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(' +
diff --git a/src/parser/recursivelyParseGradientsXlink.ts b/src/parser/recursivelyParseGradientsXlink.ts
index dde89cd1b..67174155c 100644
--- a/src/parser/recursivelyParseGradientsXlink.ts
+++ b/src/parser/recursivelyParseGradientsXlink.ts
@@ -1,5 +1,3 @@
-import { elementById } from './elementById';
-
 const gradientsAttrs = [
   'gradientTransform',
   'x1',
@@ -20,7 +18,7 @@ export function recursivelyParseGradientsXlink(
   gradient: Element
 ) {
   const xLink = gradient.getAttribute(xlinkAttr)?.slice(1) || '',
-    referencedGradient = elementById(doc, xLink);
+    referencedGradient = doc.getElementById(xLink);
   if (referencedGradient && referencedGradient.getAttribute(xlinkAttr)) {
     recursivelyParseGradientsXlink(doc, referencedGradient as Element);
   }


Test patch:
diff --git a/src/parser/loadSVGFromString.test.ts b/src/parser/loadSVGFromString.test.ts
new file mode 100644
index 000000000..06a838117
--- /dev/null
+++ b/src/parser/loadSVGFromString.test.ts
@@ -0,0 +1,36 @@
+import { Path } from '../shapes/Path';
+import { loadSVGFromString } from './loadSVGFromString';
+
+describe('loadSVGFromString', () => {
+  it('returns successful parse of svg with use tag containing bad reference', async () => {
+    // in this case, ignore bad use but still load rest of svg
+    const str = `<svg viewBox="0 0 128 128" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">
+      <rect width="10" height="10" />
+      <use href="#missing" x="50" y="50" ></use>
+      </svg>`;
+
+    const parsedSvg = await loadSVGFromString(str);
+    expect(parsedSvg.objects[0]).not.toBeNull();
+    if (parsedSvg.objects[0] !== null) {
+      expect(parsedSvg.objects[0].isType('Rect')).toBe(true);
+    }
+  });
+
+  it('returns successful parse of svg with use tag containing bad clip-path', async () => {
+    // in this case, load svg but ignore clip-path attribute in <use>
+    const str = `<svg viewBox="0 0 128 128" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">
+      <defs>
+      <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" />
+      </defs>
+      <use clip-path="url(#myClip)" href="#heart" fill="red" />
+      </svg>`;
+    // need to load Path here for it to populate in class registry; loadSvgFromString does not
+    // import Path so we'd fail the test without this.
+    const unused = Path.name;
+
+    const parsedSvg = await loadSVGFromString(str);
+    if (parsedSvg.objects[0] !== null) {
+      expect(parsedSvg.objects[0].isType('Path')).toBe(true);
+    }
+  });
+});