linebender / resvg

An SVG rendering library.
Apache License 2.0
2.84k stars 229 forks source link

Add support for stylesheet injection #785

Closed LaurenzV closed 2 months ago

LaurenzV commented 4 months ago

Not really in a mergeable state yet, but opening it for discussion. In particular:

sheet
                .rules
                .iter()
                .filter(|r| r.selector.matches(&XmlNode(xml_node)))
                .flat_map(|r| r.declarations.clone())
                .collect::<Vec<Declaration>>();

But if I put that into a function, I once again will have to add some more lifetime annotations which are currently omitted.

Fixes #277.

RazrFalcon commented 4 months ago

Shouldn't we simply call simplecss::StyleSheet::parse_more for the injected stylesheet in resolve_css before SVG styles? That's it. It should simplify code quite a lot.

The reason why I haven't added such "simple" feature yet is because I'm still confused about user styles order. Should we make this configureable? Should we hard code it? Had no time to think about it. In my opinion, style injection must override every style SVG contains. This should be the default behavior. And we can add priority/order support later.

PS: remember that we do not support !important yet. Not sure how it plays with style injection.

LaurenzV commented 4 months ago

Shouldn't we simply call simplecss::StyleSheet::parse_more for the injected stylesheet in resolve_css before SVG styles? That's it. It should simplify code quite a lot.

As I mentioned, the problem is that the injected stylesheet then can't overwrite values that are defined via an inline style property (because the stylesheet is applied before the inline styles attribute), which I think is desirable, at least for my use case...

The reason why I haven't added such "simple" feature yet is because I'm still confused about user styles order. Should we make this configureable? Should we hard code it? Had no time to think about it.

I don't know the details about those different style origins, but imo it makes sense to just let the user decide what they want, i.e. making it configurable.

PS: remember that we do not support !important yet. Not sure how it plays with style injection.

No idea either. Guess we'd have to figure it out once we add it.

LaurenzV commented 4 months ago

But maybe it does make sense to figure out the style origin thing in more detail and see if it makes sense to follow that, so I guess I'll mark this PR as draft for now.

RazrFalcon commented 4 months ago

Can you explain your use case? Do you want to be able to inject rect { fill:green; } and it will overwrite all rectangle fills. Or do you want to set existing styles, like .rect-fill-style { fill:green; }, which is not set by default? Because those are two very different operations.

Also, simply putting rect { fill:green !important; } at the top or the bottom of the CSS should do the trick, no? I do not remember how CSS works. But then we have to implement !important support first.

Basically, it all boils down to what behavior do you expect. And the original author of #277 didn't specified it either. Like something in SVG can be described as "self-explanatory"...

LaurenzV commented 4 months ago

Can you explain your use case? Do you want to be able to inject rect { fill:green; } and it will overwrite all rectangle fills. Or do you want to set existing styles, like .rect-fill-style { fill:green; }, which is not set by default? Because those are two very different operations.

For me it's the first one. If I define rect {fill: green}, I want all rects in the SVG to have that fill, regardless of whether their fill is set to a different one. If it makes it easier, having !important in it would also work for me. I guess I'll have to take a look at it.

RazrFalcon commented 4 months ago

Can you try it out using rsvg-convert from librsvg? It does support CSS injection. We can mimic their logic.

LaurenzV commented 4 months ago

Will try.

LaurenzV commented 2 months ago

Alright, I ran an experiment with rsvg-convert:

0:

<svg id="svg1" viewBox="0 0 200 200" xmlns="http://www.w3.org/2000/svg">
    <rect id="rect1" x="20" y="20" width="160" height="160"/>
</svg>

1:

<svg id="svg1" viewBox="0 0 200 200" xmlns="http://www.w3.org/2000/svg">
    <rect id="rect1" x="20" y="20" width="160" height="160" fill="green"/>
</svg>

2:

<svg id="svg1" viewBox="0 0 200 200" xmlns="http://www.w3.org/2000/svg">
    <rect id="rect1" x="20" y="20" width="160" style="fill: green" height="160"/>
</svg>

3:

<svg id="svg1" viewBox="0 0 200 200" xmlns="http://www.w3.org/2000/svg">
    <rect id="rect1" x="20" y="20" width="160" style="fill: green" height="160"/>
</svg>

Style sheet:

rect {
    fill: blue;
}

Results: 0:

0 1:

1 2:

2 3:

3

So indeed, looks like rsvg-convert does not override existing style sheets in the CSS. I still would like to have that ability at some point, but I think for now we can just go with the simple version, and extend it in the future, if necessary.

RazrFalcon commented 2 months ago

Looks good overall. Thanks again!