Repository: fabricjs/fabric.js
Pull request #10079 (2024-08-19 11:46:05)

fix(parser): Skip `id` attribute from normalization
<!--
        Hi there!
        Thanks for taking the time and putting the effort into making fabric better! 💖
        Take a look at /CONTRIBUTING.md for crucial instructions regarding local setup, testing etc.
        https://github.com/fabricjs/fabric.js/blob/master/CONTRIBUTING.md

        Adding tests that verify your fix and safeguard it from unwanted loss and changes is a MUST.

        Pull Requests are not always simple. Don't hesitate to ask for help (beware of [gotchas](http://fabricjs.com/fabric-gotchas) 😓).
        We appreciate your effort and would like the process to be productive and enjoyable.
        A strong community means a strong and better product for everyone.

        Each PR can introduce a single feature or change or fix a single bug
        Large refactors PR have been discussed before and probably splitted in steps
-->

## Description

<!-- Summarize the reasons of your changes and the meaning of your code. Why the code you are changing is wrong? why your is better? -->
<!-- If you are fixing a regression, please link the commit that introduced the bug -->
<!-- If you are proposing a feature, please link the discussion/issue where that was presented and discussed -->
<!-- Is this pullrequest a follow up from an open issue? if so please link the issue like the example below in addition to the summary -->
<!-- Related to #1234 -->

If we load SVG that has some object with `id` attribute starting with a `number`, we get a wrong id parsed.



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

Open the branch in <a href="https://codesandbox.io/p/github/madcoons/fabric.js/master?mode=review&utm_source=gh_app">Web Editor</a> &bull; <a href="https://codesandbox.io/p/vscode?owner=madcoons&repo=fabric.js&branch=master&utm_source=gh_app">VS Code</a> &bull; <a href="https://codesandbox.io/p/vscode?owner=madcoons&repo=fabric.js&branch=master&insiders=true&utm_source=gh_app">Insiders</a><br>
Open <a href="https://codesandbox.io/p/devtool/preview/madcoons/fabric.js/master?task=preview&port=51423&redirect=true&utm_source=gh_app">Preview</a>

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

zhe-he: This kind of attribute has issues as well, right?
`<svg data-custom-attribute="123abc"></svg>`


asturur: @miroljub1995 add this line to the changelog:
fix(SVGImport): Allow parsing of 'id' attribute that starts with a number [#10079](https://github.com/fabricjs/fabric.js/pull/10079)

and we can merge.

Please check that it gets formatted as the other lines or we get a lint error.
I can't edit myself as i don't have write access to your branch
miroljub1995: @asturur 
- I added line to CHANGELOG
- Regarding to lint, I checked with `npm run prettier:check && npm run lint`, but there are already some errors so I just checked if no new errors are on files I changed.
asturur: There are maybe warnings, the checkmark is green.
Or maybe somehow you are picking up a different configuration

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index dd7a75b6d..c9f0c4de8 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -2,6 +2,7 @@
 
 ## [next]
 
+- fix(SVGImport): Allow parsing of 'id' attribute that starts with a number [#10079](https://github.com/fabricjs/fabric.js/pull/10079)
 - fix(filter): pixelate filter has non square pixels in webgl (#10081)
 - feat(Canvas): Avoid styling the lower canvas with absolute positioning [#10077](https://github.com/fabricjs/fabric.js/pull/10077)
 - chore(TS): Add missing export type for Text events [#10076](https://github.com/fabricjs/fabric.js/pull/10076)
diff --git a/src/parser/normalizeValue.ts b/src/parser/normalizeValue.ts
index 7b68160a3..dcea03640 100644
--- a/src/parser/normalizeValue.ts
+++ b/src/parser/normalizeValue.ts
@@ -56,7 +56,12 @@ export function normalizeValue(
     } else if (fillIndex === -1 && strokeIndex > -1) {
       ouputValue = STROKE;
     }
-  } else if (attr === 'href' || attr === 'xlink:href' || attr === 'font') {
+  } else if (
+    attr === 'href' ||
+    attr === 'xlink:href' ||
+    attr === 'font' ||
+    attr === 'id'
+  ) {
     return value;
   } else if (attr === 'imageSmoothing') {
     return value === 'optimizeQuality';


Test patch:
diff --git a/src/parser/loadSVGFromString.test.ts b/src/parser/loadSVGFromString.test.ts
index b5a98a36e..de1fed8b2 100644
--- a/src/parser/loadSVGFromString.test.ts
+++ b/src/parser/loadSVGFromString.test.ts
@@ -1,4 +1,5 @@
 import { Path } from '../shapes/Path';
+import { Rect } from '../shapes/Rect';
 import { loadSVGFromString } from './loadSVGFromString';
 
 describe('loadSVGFromString', () => {
@@ -30,4 +31,17 @@ describe('loadSVGFromString', () => {
       expect(parsedSvg.objects[0] instanceof Path).toBe(true);
     }
   });
+
+  it('returns successful parse of svg with id starting with number', async () => {
+    const str = `<svg viewBox="0 0 128 128" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">
+      <rect id="123xyz" width="10" height="10" />
+      </svg>`;
+
+    const parsedSvg = await loadSVGFromString(str);
+
+    expect(
+      parsedSvg.objects[0] instanceof Rect &&
+        (parsedSvg.objects[0] as Rect & { id: string }).id,
+    ).toBe('123xyz');
+  });
 });