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

Add support for stylesheet injection #785

Merged
merged 16 commits into from
Sep 21, 2024

Conversation

LaurenzV
Copy link
Contributor

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

  • Any suggestions on how to avoid adding a new parameter for the functions? I thought about merging it with the global stylesheet in the beginning, but the problem is that if we do that, the injected stylesheet cannot take precedence over the delcarations from the style attribute, which is pretty important (at least for my use case).
  • I tried avoiding collecting the declarations into vectors, but then I would probably have to add more lifetime annotations, at least from my initial experiments. Not sure what you prefer.
  • Not a fan of the duplicated
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.

pub has_priority: bool,
/// The stylesheet that should be injected.
pub style_sheet: &'a str,
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An owned string here is fine.

@RazrFalcon
Copy link
Collaborator

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
Copy link
Contributor Author

LaurenzV commented Jun 28, 2024

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
Copy link
Contributor Author

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.

@LaurenzV LaurenzV marked this pull request as draft June 28, 2024 09:10
@RazrFalcon
Copy link
Collaborator

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
Copy link
Contributor Author

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
Copy link
Collaborator

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

@LaurenzV
Copy link
Contributor Author

Will try.

@LaurenzV
Copy link
Contributor Author

LaurenzV commented Sep 20, 2024

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.

@LaurenzV LaurenzV marked this pull request as ready for review September 20, 2024 08:59
@RazrFalcon
Copy link
Collaborator

RazrFalcon commented Sep 20, 2024

Looks good overall. Thanks again!

@RazrFalcon RazrFalcon merged commit 5141a83 into linebender:master Sep 21, 2024
3 checks passed
@LaurenzV LaurenzV deleted the stylesheet-injection branch September 21, 2024 07:50
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.

Feature request: stylesheet injection
2 participants