Skip to content

fix(parser): ignore empty declarations in SVG style attributes instead of failing the import - #11185

Merged
Smrtnyk merged 3 commits into
fabricjs:masterfrom
Enesuygurs:fix/parser-blank-style-declarations
Oct 30, 2026
Merged

Smrtnyk merged 3 commits into
fabricjs:masterfrom
Enesuygurs:fix/parser-blank-style-declarations

Conversation

@Enesuygurs

Copy link
Copy Markdown
Contributor

Description

Closes #11184

parseStyleString only skipped chunks that were completely empty. A blank declaration, like the one in style="fill:red; ;", or a whitespace-only style=" ", gets past that check. Then chunk.split(':') has no value, and value.trim() throws. Because this happens during parsing, the whole loadSVGFromString call rejects, not just the element with the blank declaration:

await loadSVGFromString(
  '<svg xmlns="http://www.w3.org/2000/svg"><rect width="10" height="10" style="fill:red; ;"/></svg>',
);
// TypeError: Cannot read properties of undefined (reading 'trim')

Empty declarations are valid CSS and browsers ignore them. The parser used to guard against this (see #2241), but the check was lost.

The fix skips any chunk without a name:value pair. That replaces the old !chunk check, since an empty chunk has no value either.

Tests:

  • Unit tests in parser.spec.ts for a style with empty declarations between valid ones, and for a blank style attribute.
  • As suggested in the issue, a visual case in the SVG import tests (svg_style_empty_declarations). It uses four shapes whose style attributes each contain blank declarations, including one where a declaration after a blank one (the stroke) still has to apply. Without the fix it fails with the TypeError above; with it, it renders as expected.

Note: I generated the golden with Playwright's chrome channel (Chrome 156) on Windows. The CI runner is the Linux headless shell. The case only has solid fills and one stroke, so it should be well within the tolerance, but if CI reports a small diff the golden may need to be regenerated there.

In Action

Golden for the new case: red and blue squares and a green circle, all filled through styles with blank declarations, plus an orange circle whose black stroke comes after a blank declaration.

@codesandbox

codesandbox Bot commented Oct 23, 2026

Copy link
Copy Markdown

Review or Edit in CodeSandbox

Open the branch in Web Editor • VS Code • Insiders

Open Preview

@github-actions

github-actions Bot commented Oct 23, 2026 •

Copy link
Copy Markdown
Contributor

Build Stats

entrypoint / KiB (diff) generated minified
fabric (browser UMD) 805.942 (+0.010) 294.733 (+0.001)
fabric/node (CJS) 777.345 (+0.010) 0.000 (0)
@fabricjs/core 781.421 (+0.010) 0.000 (0)
@fabricjs/browser 0.538 (0) 0.000 (0)
@fabricjs/node 2.243 (0) 0.000 (0)
@fabricjs/gradient-controls 5.026 (0) 0.000 (0)
@fabricjs/cropping-controls 20.773 (0) 0.000 (0)
@fabricjs/aligning-guidelines 15.521 (0) 0.000 (0)
@fabricjs/data-updaters 2.915 (0) 0.000 (0)
@fabricjs/westures-integration 2.153 (0) 0.000 (0)
fabric/extensions (UMD) — 21.250 (0)

@asturur

asturur commented Oct 30, 2026

Copy link
Copy Markdown
Member

@Smrtnyk can you merge this when you can?

@Smrtnyk
Smrtnyk merged commit eac083c into fabricjs:master Oct 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: an empty declaration in an SVG style attribute makes loadSVGFromString reject

3 participants