2 fixes for usvg Tree writer #672
Conversation
|
|
||
| if let Some(ref mask) = g.mask { | ||
| if has_xlink(&mask.root) { | ||
| return true; | ||
| } | ||
|
|
||
| if let Some(ref sub_mask) = mask.mask { | ||
| if has_xlink(&sub_mask.root) { | ||
| return true; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
I guess it would be nice to reuse the node.subroots method here, but it looks like this one just allows me to pass a function to make changes to the node itself instead of giving me a way of iterating over the nodes itself and return a boolean from that.
There was a problem hiding this comment.
Yeah... I haven't figured out a better API.
|
I realized another issue, the following SVG will also not be rendered properly: <svg id="svg1" viewBox="0 0 200 200" xmlns="http://www.w3.org/2000/svg"
font-family="Noto Sans" font-size="80">
<title>Linear gradient on text</title>
<linearGradient id="lg1">
<stop id="stop1" offset="0" stop-color="white"/>
<stop id="stop2" offset="1" stop-color="black"/>
</linearGradient>
<text id="text1" x="100" y="120" text-anchor="middle" fill="url(#lg1)">Text</text>
<!-- image frame -->
<rect id="frame" x="1" y="1" width="198" height="198" fill="none" stroke="black"/>
</svg>The reason being that since the paint is on a |
|
So, turns out that there was another bug, because the assumption that a group element in a clip path only contains one element seems to be wrong if the clip path contains more than 2 (converted) text elements. This is why I just went for a for loop now instead of checking for the first child only and then unwrapping. |
No, it is... Just to clarify,
How is this possible? A clip path with a group is always manually generated and therefore can only have 1 element. |
Well, it doesn't seem too bad from what I've seen. And I would need this for my use case, so if you're okay with accepting PRs I'd like to work on improving it!
I'll look into it. |
Sure. I just wanted to clarify that only |
So, if I understood it right, in the normal case a clip path should have a root element (which is a group) and then a number of children consisting of paths/texts (except for this one edge case mentioned in the comments). However, when converting the texts to paths, we create a new group node which contains all of the text nodes, and thus if the text elements were part of a clip path, they are not the direct children of the clip path root anymore. Instead, the clip path root will have the group as a child that contains all text nodes. What's the best course of action here? Should I leave the for loop or should we change how text nodes are appended to the parent? I guess the second one would be better so that we can keep the edge case of a clip path root having another group as a child really be an edge case? |
|
I changed the code with a possible fix. Let me know what you think. Regarding the gradient problem, I will probably leave that one for a different PR since it requires a bit more thinking from my side. |
|
Hm... I genuinely do not remember why we have a group in a clip path. For one, I think we should update |
|
Okay, I'll see what I can do. |
|
Updating |
So basically just using a for loop, like I did originally? |
Actually, isn't the problem here that text/path elements can have other clip paths as well? In this case, we need to wrap the text/path inside of another group, because only groups can have other clip paths as well. |
|
I reverted the change to |
Yep. |
|
Should be done, I also added a comment so that we don't forget about it. So from my side this is ready. 😄 |
|
Thanks! |
Firstly, I fixed an issue where a call to
.unwrapwould cause a panic with the following svg file (from the test suite):I guess it's not really relevant since it's invalid, but I would say it's still better to just ignore it than let it crash in that case.
Secondly, the writer failed to write the
xmlns:xlinkattribute for images inside of masks because they weren't checked properly.