Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(SVGParser): Don't crash on nested CSS at-rules #9707

Merged
merged 6 commits into from
Mar 4, 2024

Conversation

silverwind
Copy link
Contributor

@silverwind silverwind commented Mar 3, 2024

Description

This will ignore all CSS rules that contain unbalanced brackets after splitting on }.

  • circle { fill: black; } splits to circle { fill: black; which has one {, so it's okay.
  • @media (prefers-color-scheme: dark) { circle { fill: white; } } splits to @media (prefers-color-scheme: dark) { circle { fill: white; which has two { and starts with @ so is ignored.

As mentioned in #9679 I think using a AST parser would be a much better option than this current crude parsing, for the future.

Ref: https://developer.mozilla.org/en-US/docs/Web/CSS/At-rule
Fixes: #9679

Copy link

codesandbox bot commented Mar 3, 2024

Review or Edit in CodeSandbox

Open the branch in Web EditorVS CodeInsiders

Open Preview

@silverwind
Copy link
Contributor Author

I also verified this fix in the actual use case, and it did generate the black circle as expected.

@silverwind
Copy link
Contributor Author

silverwind commented Mar 3, 2024

I added a condition so it looks for the @ to only ignore at-rules. There is also nesting syntax in CSS which would still break the parser. If you prefer to also ignore this nesting syntax, I can remove the @ condition again.

src/parser/getCSSRules.ts Outdated Show resolved Hide resolved
asturur
asturur previously approved these changes Mar 3, 2024
@asturur
Copy link
Member

asturur commented Mar 3, 2024

i need to fix prettier or you can do it.
npm run prettier:write

@silverwind
Copy link
Contributor Author

Ran it.

@silverwind
Copy link
Contributor Author

I added a condition so it looks for the @ to only ignore at-rules. There is also nesting syntax in CSS which would still break the parser. If you prefer to also ignore this nesting syntax, I can remove the @ condition again.

What do you recommend about this one? Should we ignore non-at-rule nesting as well? I don't really understand the purpose of the parser and whether it's better to crash or ignore.

@asturur
Copy link
Member

asturur commented Mar 4, 2024

The parser has been written in 2010-2012 and supported ie7+ initially through a canvas polyfill.
It has been kept alive since then.
The scope of SVG parser in fabricJS is to being able to import SVGs as shapes rather than images. That is its scope.
We declare we support svg specs up version 1.2, so if something is above that version we can support it optionally when is easy.
I do not know which is the meaning of SVG versions, so if let's say SVG 1.2 supports style tag in 2012 then it means that it will support every new css syntax in 2024 but still being called SVG 1.2

In general all that can be fixed will be fixed also depending on the popularity of the feature.
How many softwares output svgs with nested css syntax?
Most of them just spit out a mess of attributes or at best single unique named classes.

@asturur asturur merged commit ab20586 into fabricjs:master Mar 4, 2024
20 of 22 checks passed
@silverwind silverwind deleted the atrules branch March 4, 2024 10:57
lunny pushed a commit to go-gitea/gitea that referenced this pull request Mar 27, 2024
DennisRasey pushed a commit to DennisRasey/forgejo that referenced this pull request Mar 30, 2024
Fixes go-gitea/gitea#29326 because it includes
fabricjs/fabric.js#9707.

(cherry picked from commit a9e5706696f7d593e281d33783877b7772e48e19)
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]: Cannot read properties of undefined (reading 'trim') in loadSVGFromString with @media in CSS
2 participants