Improved font family parsing + support for `font` shorthand

[LaurenzV]

Had to update some SVG files because they contained the font declaration font-family="Mplus 1p", which is not correct because 1p is not a valid ident, so it must be quoted.

Also while rewriting fixed a bug (or was this intended?) where marker-start etc. would not be parsed in the style attribute but only if declared in a global rule.

[LaurenzV]

By the way, it was a bit annoying having to change the svgtypes dependency for each create manually. Would you accept a PR that changes resvg to use workspace dependencies (basically you define the versions for the crates once globally for the whole workspace, and then for each crate you can link to the workspace version and "refine" the features, etc. if you want)?

[RazrFalcon]

Had to update some SVG files because they contained the font declaration font-family="Mplus 1p", which is not correct because 1p is not a valid ident, so it must be quoted.

What about Noto Sans? It should be quote as well? I just checked and Safari resolves font-family="Noto Sans" to Times, but font-family="Apple Chancery" is fine. When we have a single family it shouldn't be quoted then? Chrome and Firefox allow spaces in font-family without quoting. It seems like spaces are allowed, despite that being against the spec?

[RazrFalcon]

Also while rewriting fixed a bug (or was this intended?) where marker-start etc. would not be parsed in the style attribute but only if declared in a global rule.

I'm not sure marker-start is allowed in styles. Styles are CSS and CSS has only marker. We should test this.

[RazrFalcon]

By the way, it was a bit annoying having to change the svgtypes dependency for each create manually

I plan to merge all usvg-* crates into one very soon 👀

[RazrFalcon]

PS: this is a good place to ask, but I plan to make usvg::Tree read-only. Aka it cannot be modified after creation, but can be post-processed internally. This would simplify and optimize a ton of things. But would make some people unhappy. Will it affect your use case?

[LaurenzV]

What about Noto Sans? It should be quote as well? I just checked and Safari resolves font-family="Noto Sans" to Times

That's weird, I can reproduce this when opening the SVG manually in Safari, but if I open it with vdiff Safari also renders it with Noto Sans.

But anway, yes it's not mandatory to quote here (and it works fine with the new parser as well). The spec mentions that it's allowed, although not recommended. My understanding is that it's fine as long as the individual words of the font name are valid idents, otherwise you need to quote it.

I'm not sure marker-start is allowed in styles. Styles are CSS and CSS has only marker. We should test this.

Okay, I'll add tests!

I plan to merge all usvg-* crates into one very soon 👀

Ahh okay, in this case I'll leave it as is!

PS: this is a good place to ask, but I plan to make usvg::Tree read-only. Aka it cannot be modified after creation, but can be post-processed internally. This would simplify and optimize a ton of things. But would make some people unhappy. Will it affect your use case?

I think it might affect us, but I'm not sure. I have to check. I'll try to look through the code of svg2pdf to see whether we update the tree somewhere.

Might also be worth to check whether resvg-js is affected by this.

On the other hand, I also like the idea of postprocessing steps and it would allow for some nice optimizations.

[RazrFalcon]

That's weird, I can reproduce this when opening the SVG manually in Safari, but if I open it with vdiff Safari also renders it with Noto Sans.

Preview/qlmanage and Safari are not the same. And I don't know why. Very confusing.

If I read the spec correctly, Noto Sans if perfectly fine, but Noto 1Sans is wrong, because ident cannot start with a digit? That's a bizarre edge case. Never knew about it.

On the other hand, I also like the idea of postprocessing steps and it would allow for some nice optimizations.

The idea is that usvg itself could post-process the tree, but the caller couldn't. The tree is already de-factor read-only. I just want to make it explicit. Right now, if you're modifying the tree after parsing - you're breaking something 99% of the time. A potentially painful change to some, but inevitable.

[RazrFalcon]

Published new svgtypes.

[LaurenzV]

I'm not sure marker-start is allowed in styles. Styles are CSS and CSS has only marker. We should test this.

Nono, so what I meant is this (this is the original code):

// Apply CSS.
    for rule in &style_sheet.rules {
        if rule.selector.matches(&XmlNode(xml_node)) {
            for declaration in &rule.declarations {
                // TODO: perform XML attribute normalization
                if let Some(aid) = AId::from_str(declaration.name) {
                    // Parse only the presentation attributes.
                    if aid.is_presentation() {
                        insert_attribute(aid, declaration.value);
                    }
                } else if declaration.name == "marker" {
                    insert_attribute(AId::MarkerStart, declaration.value);
                    insert_attribute(AId::MarkerMid, declaration.value);
                    insert_attribute(AId::MarkerEnd, declaration.value);
                }
            }
        }
    }

    // Split a `style` attribute.
    if let Some(value) = xml_node.attribute("style") {
        for declaration in simplecss::DeclarationTokenizer::from(value) {
            // TODO: preform XML attribute normalization
            if let Some(aid) = AId::from_str(declaration.name) {
                // Parse only the presentation attributes.
                if aid.is_presentation() {
                    insert_attribute(aid, declaration.value);
                }
            }
        }
    }

So the marker is applied if it appears in the style sheet but not if it appears in a style attribute. But they are both CSS, so either it should happen in none of them or in both of them (and I just tested and it should be in both of them).

[LaurenzV]

If I read the spec correctly, Noto Sans if perfectly fine, but Noto 1Sans is wrong, because ident cannot start with a digit? That's a bizarre edge case. Never knew about it.

Yes, that's my understanding as well.

[RazrFalcon]

As for marker-start, can you create a test file for this case? I don't remember if it was intentionally ignored from style or not.

[LaurenzV]

I'm confused, you mean something like this?

<svg id="svg1" viewBox="0 0 200 200" xmlns="http://www.w3.org/2000/svg">
    <title>The `marker` property in CSS</title>

    <marker id="marker1" refX="10" refY="10" markerWidth="20" markerHeight="20">
        <path id="path-marker" d="M 10 0 16 20 H 4 Z" fill="blue" opacity="0.75"/>
    </marker>
    <path id="path1" fill="green" style="marker-start:url(#marker1)" d="M 100 15 l 50 160 l -130 -100 l 160 0 l -130 100"/>

    <!-- image frame -->
    <rect id="frame" x="1" y="1" width="198" height="198" fill="none" stroke="black"/>
</svg>

But it's not marker-start that was ignored, but just marker.

[RazrFalcon]

Oh, got it. I just want to make sure what other apps do when there is marker in style.

[LaurenzV]

They all show it except for resvg (in the current master branch):

So should I add this as a test case?

[RazrFalcon]

Good. Then it was indeed my bug.

[RazrFalcon]

I saw you have quoted some "Noto Sans" fonts as well. We don't need this, right?

[LaurenzV]

Good. Then it was indeed my bug.

So should I add the test?

I saw you have quoted some "Noto Sans" fonts as well. We don't need this, right?

Yes it's not needed, the SVG spec just recomments to quote all font names with spaces.

[RazrFalcon]

So should I add the test?

No need to.

Yes it's not needed, the SVG spec just recomments to quote all font names with spaces.

I think we can ignore it. Otherwise we would have to change hundreds of tests.

[LaurenzV]

I saw you have quoted some "Noto Sans" fonts as well. We don't need this, right?

Oh, now I know what you mean. I didn't change it for the input SVGs, only for the output SVGs via usvg writer. But that's just because it's safer to just always quote the font family name instead of only quoting it if necessary. That's the code:

 let font_family_to_str = |font_family: &FontFamily| match font_family {
        FontFamily::Monospace => "monospace".to_string(),
        FontFamily::Serif => "serif".to_string(),
        FontFamily::SansSerif => "sans-serif".to_string(),
        FontFamily::Cursive => "cursive".to_string(),
        FontFamily::Fantasy => "fantasy".to_string(),
        FontFamily::Named(s) => {
            if ctx.opt.writer_opts.use_single_quote {
                format!("\"{}\"", s)
            } else {
                format!("'{}'", s)
            }
        }
    };

I think it would be a bit unnecessary to add extra logic just to check whether the font name could in theory be written without quotes, no? It's safer to just always do it for the usvg writer.

[RazrFalcon]

Oh, those are usvg writer tests, my bad. Hmm... I'm sure it will annoy a lot of people. I would prefer usability to correctness here. Can't we use svgtypes here to check if a family name needs quotes? If it was parsed correctly - then no. If error - add quotes.

[LaurenzV]

Done!

[RazrFalcon]

Thanks!