[Bug]: Cannot read properties of undefined (reading 'trim') in loadSVGFromString with @media in CSS

[silverwind]

CheckList

• I agree to follow this project's Code of Conduct
• I have read and followed the Contributing Guide
• I have read and followed the Issue Tracker Guide
• I have searched and referenced existing issues and discussions
• I am filing a BUG report.
• I have managed to reproduce the bug after upgrading to the latest version
• I have created an accurate and minimal reproduction

Version

6.0.0-beta19

In What environments are you experiencing the problem?

Node.js

Node Version (if applicable)

21.6.1

Link To Reproduction

See simple node script below

Steps To Reproduce

import {loadSVGFromString} from 'fabric/node';

await loadSVGFromString(`
  <svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 20 20">
    <style>
      circle { fill: black; }
      @media (prefers-color-scheme: dark) { circle { fill: white; } }
    </style>
    <circle r="10" cx="10" cy="10"/>
  </svg>
`);

Expected Behavior

No error.

Actual Behavior

Script errors. Observations:

• Removing the @media line makes it work.
• Fails in both 6.0.0-beta19 and 5.3.0.

Error Message & Stack Trace

TypeError: Cannot read properties of undefined (reading 'trim')
    at node_modules/fabric/dist/index.node.mjs:24637:27
    at Array.forEach (<anonymous>)
    at getCSSRules (node_modules/fabric/dist/index.node.mjs:24627:6)
    at new ElementsParser (node_modules/fabric/dist/index.node.mjs:24663:21)
    at parseSVGDocument (node_modules/fabric/dist/index.node.mjs:24822:25)
    at loadSVGFromString (node_modules/fabric/dist/index.node.mjs:24850:10)
    at fab.js:3:7
    at ModuleJob.run (node:internal/modules/esm/module_job:218:25)
    at async ModuleLoader.import (node:internal/modules/esm/loader:323:24)
    at async loadESM (node:internal/process/esm_loader:28:7)

[ShaMan123]

I can confirm and reproduce
For now I suggest you remove that directive yourself

[silverwind]

Yeah, removal should be possible in this case. We convert SVG to PNG via fabric and a prefers-color-mode media query does not make sense to during this convertion because it can not be preserved into the output raster image. Other media queries might, thought.

[ShaMan123]

agreed

[asturur]

Do SVGs as graphic format support media queries? Or is a browser specific support?

[asturur]

we recently added a fix for fontface declaration that has a very similar sintax, maybe you can leverage that

[asturur]

759d6b7

[silverwind]

Do SVGs as graphic format support media queries? Or is a browser specific support?

Good question. In this i read:

Elements in an SVG document can be styled using CSS. Most visual characteristics and some aspects of element geometry are controlled using CSS properties

The style element explicitely lists the media attribute, so I think it's safe to say it's supported in the style element as @media.

[asturur]

I mean but if you open it in inkscape, what are you going to see? the dark or light version?

[silverwind]

In Inscape 1.2.3, I see that black dot, e.g. the light mode rendering, even thought my OS is dark mode. I guess that only proves that Inkscape does not support such media queries.

[asturur]

yes inkscape has a pure svg parsers. I m sure for now we can't really support mediaqueries and if we do we won't be able anyway to do into node. So for now we can just fix to avoid crashing.

[silverwind]

On the web, this technique is somewhat common to say the least, as popularized by blog posts such as this one.

I guess fabric could expose a API to set dark/light mode. But as for this issue, I'd be happy if it wouldn't crash and produce the light mode output, e.g. ignoring the media query.

[asturur]

that pr/commit that i linked, is that clear to you? you can try to PR based on that code, knowing the are you have to look around is in that few lines.
I suspect since we are splitting by '{' and saying that the left part is the identifier, those nested curly braces are a pain in the teeth.
Maybe we can come up with a regex that identifies groups of @media {} and clean them out of a style text?

[silverwind]

Yes, I can try it. Sounds like you are not using a AST parser? I think you should, like postcss for example.

[asturur]

we don't have dependencies by choice for now
The whole svg parsing needs to be redone and will be different in node and browser.
For example a Browser can just use something like getComputedStyle and move on, while node not.
So far we have a simple naive js implementation of parser that i kept running when i joined the project.
I m not sure what is the best future for it, if it will be an external package, a different import for node and browser or the same code but improved.

[asturur]

is also unrealistic to hope to implement all the SVG features, there are so many and some are so complicated.
Filter and tspans and masks are 2 major missing, but then svg 2.0 brought in so many things i m not sure if they can be handled at all. In some cases is just better to change the SVG dom and paint it as an image in fabricJS

[silverwind]

AST parsers are reasonably lightweight and they can run in bother node and browser. I think you could definitely share the parser between all environments. postcss' primary use case is as a CSS transformation tool, but I don't see why it couldn't be used as a parser only. There is also css-tree, but it's not as well maintained as postcss.

https://bundlephobia.com/package/postcss@8.4.35
https://bundlephobia.com/package/css-tree@2.3.1

[silverwind]

#9707

[asturur]

i understand 49Kb is not much, but whole fabric is 300k. is a sixth of fabric i would add to save on 3 dumb loops i do.
The moment in which the svg parser becomes its own import with its own bundle i can understand that, today that is a whole single blob no.
if there is a website called 'bundlephobia' a reason exists. :)

[silverwind]

I guess it's okay for fabric's use case to not support the latest and greatest CSS features. Usage of CSS in SVG is rare, but this media query example is something that will be encountered sometimes as it's a nice way to create a portable dark-mode compatible SVG.