Text positioning regression in 0.36 compared to 0.27

[Weasy666]

I was running version 0.27 of resvg-explorer-extension.exe for a long time and came around to update recently and noticed a regression between 0.27 and 0.36 in regard to the text handling/positioning.

As i am also using usvg in a project of mine to parse SVG files and then render them, i went ahead and also updated my project from 0.27 to 0.36 and noticed the same regression there.

Here you can find an example:

usvg 0.27 (correct text placement)

Rendered in my project:

usvg 0.36 (wrong text placement)

Rendered in my project:

Thumbnail rendered in explorer:

If you look closely where the arrow points to, you can see, some cut of text pieces there.

I'll also attach the file in question for debugging reasons:
twinkle.zip

[RazrFalcon]

resvg renders this file just fine. Maybe you're handling transforms incorrectly on your side?

As for thumbnailer, it looks like fonts were not loaded to begin with. Will try to figure it out.
This SVG file doesn't define any fonts to begin with. Times is not a valid family name in most cases. It should fallback to Times New Roman, but it didn't I guess.

[Weasy666]

If you look closely to where the red arrow is pointing, then you can see the comma an lower part of the f can be seen on the thumbnail. So it loads some font and displays it, but at the same incorrect position as it does in my project.

I am using the NodeExt abs_transform to get the position. Which worked fine in version 0.27, but not any more in 0.36, despite using TreeTextToPath.

[RazrFalcon]

Yes, as I've said, there is definitely a bug in the thumbnailer, but not in resvg.

Will see what's wrong with NodeExt::abs_transform. There were a lot of breaking changes since 0.27. Not surprising. And only resvg rendering is tested.

[Weasy666]

No worries. Thank you very much for all of your work and these crates 🙂

[StrikeForceZero]

it might be related to svgs having groups inside their tspan / text nodes?

[RazrFalcon]

Interesting. Yes, the g element isn't allowed inside text. I think we're simply skipping it.

[StrikeForceZero]

Interesting. Yes, the g element isn't allowed inside text. I think we're simply skipping it.

I haven't had time to confirm this in usvg code but it's appearing as though the text nodes are processed but they fail to get their positions populated even when the empty illegal group node is just a sibling to a text span but inside the text node itself. Deleting the empty group node or moving it to be a sibling outside of the text node restores the positioning being populated and thus the proper rendering of that SVG.

Not handling the illegal group node is totally fair but if my assumption is correct, would the expected behavior be to continue to populate the positions or to be atomic and discard the text node entirely?

Edit: Svg markdown context

<text x="2363" y="1215">
  <tspan>
    <g /><!-- illegal group prevents positioning of text to be populated -->
    <tspan font-size="180px" font-style="normal">
      Star
    </tspan>
  </tspan>
</text>

[StrikeForceZero]

here's a regression test

#[test]
#[cfg(feature = "text")]
fn group_in_text() {
    fn check(svg: &str) {
        let mut fontdb = usvg::fontdb::Database::default();
        fontdb.load_system_fonts();
        let fontdb = Arc::new(fontdb);
        let options = usvg::Options {
            fontdb,
            ..Default::default()
        };
        let tree = usvg::Tree::from_str(&svg, &options).unwrap_or_else(|err| panic!("{err}"));
        let root = tree.root();
        assert!(tree.has_text_nodes());
        assert_eq!(root.children().len(), 1, "root empty");
        let Node::Group(group) = &root.children()[0] else { unreachable!() };
        assert_eq!(group.id(), "g1");
        assert_eq!(group.children().len(), 1, "group empty");
        let Node::Text(text) = &group.children()[0] else { unreachable!() };
        assert_eq!(text.id(), "text1");
        let text_paths = text.flattened().children();
        assert_eq!(text_paths.len(), 1, "flattened text has no paths");
        let Node::Path(path) = &text_paths[0] else { unreachable!() };
        assert_eq!(text.abs_transform(), usvg::Transform::identity());
        type PS = usvg::tiny_skia_path::PathSegment;
        type P = usvg::tiny_skia_path::Point;
        assert_eq!(path.data().segments().collect::<Vec<_>>(), vec![
            PS::MoveTo(P::from_xy(54.98, 54.8)),
            PS::LineTo(P::from_xy(54.98, 34.8)),
            PS::LineTo(P::from_xy(56.18, 34.8)),
            PS::LineTo(P::from_xy(56.18, 54.8)),
            PS::LineTo(P::from_xy(54.98, 54.8)),
            PS::Close,
        ]);
    }

    check(r#"<?xml version="1.0" encoding="UTF-8" standalone="no"?>
<svg viewBox="0 0 100 100" version="1.1" xmlns="http://www.w3.org/2000/svg" overflow="visible">
    <g id="g1">
        <text id="text1" x="50" y="50" font-size="0px" font-style="normal">
            <tspan>
                <tspan font-size="20px" font-style="normal">
                    |
                </tspan>
            </tspan>
        </text>
    </g>
</svg>
"#);

    // this will panic in 0.42 because the segments positions are as if they are starting at 0,0 instead of 50,50
    check(r#"<?xml version="1.0" encoding="UTF-8" standalone="no"?>
<svg viewBox="0 0 100 100" version="1.1" xmlns="http://www.w3.org/2000/svg" overflow="visible">
    <g id="g1">
        <text id="text1" x="50" y="50" font-size="0px" font-style="normal">
            <tspan>
                <g id="g2" />
                <tspan font-size="20px" font-style="normal">
                    |
                </tspan>
            </tspan>
        </text>
    </g>
</svg>
"#)
}

[StrikeForceZero]

actually, this is probably related to #739

since this has the same issue as the empty group

// this will panic in 0.42 because the segments positions are as if they are starting at 0,0 instead of 50,50
    check(r#"<?xml version="1.0" encoding="UTF-8" standalone="no"?>
<svg viewBox="0 0 100 100" version="1.1" xmlns="http://www.w3.org/2000/svg" overflow="visible">
    <g id="g1">
        <text id="text1" x="50" y="50" font-size="0px" font-style="normal">
            <tspan>
                <tspan></tspan>
                <tspan font-size="20px" font-style="normal">
                    |
                </tspan>
            </tspan>
        </text>
    </g>
</svg>
"#)

[RazrFalcon]

Thanks for looking into this. Yes, handling of invalid elements inside text is undefined in the spec. Afaik, we're still trying to count the number of characters in the invalid elements, which is probably not necessary.

resvg/crates/usvg/src/parser/text.rs

Lines 229 to 238 in 1a6922d

	// TODO: what to do when <= 0? UB?
	let font_size = super::units::resolve_font_size(parent, state);
	let font_size = match NonZeroPositiveF32::new(font_size) {
	Some(n) => n,
	None => {
	// Skip this span.
	iter_state.chars_count += child.text().chars().count();
	continue;
	}
	};