resvg icon indicating copy to clipboard operation
resvg copied to clipboard

Support `font` attribute shorthand

Open RazrFalcon opened this issue 2 years ago • 6 comments

https://developer.mozilla.org/en-US/docs/Web/CSS/font

RazrFalcon avatar Aug 21 '21 21:08 RazrFalcon

There are actually two variants of it: the XML one and the CSS one. No one except Batik supports the first one.

RazrFalcon avatar Mar 21 '23 10:03 RazrFalcon

Hello,

I spent some time thinking about how we could implement this. I believe the cleanest approach (which would also avoid us having to take the edge case of the font property into consideration when resolving font-weight, etc.) would be to resolve this when adding the attributes to a SvgNode, so here:

https://github.com/RazrFalcon/resvg/blob/90b0ea168c39d3ec1dcd07b06ded001fc45e90ea/crates/usvg-parser/src/svgtree/parse.rs#L289-L299

I would propose to roughly change this to something like this:

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() {
                    // Shorthands such as `font` cover multiple AId's, so we need to insert them separately
                    let aids = match aid {
                        AId::Font => // A function that resolves the value and returns something like
                        //[(AId::FontSize, "value"), (AId::FontStretch, "value"), etc. for all values that are covered by
                        // the shorthand, as well as the default values for the ones that were omitted]
                        _ => vec![(aid, declaration.value)]
                    };

                    for (aid, value) in aids {
                        insert_attribute(aid, value);
                    }
                }
            }
        }
    }

I think that's the safest approach because it basically just replaces the font attribute with the one's it covers (including the attributes that get reset to their default value), so whenever in the code we do something like find_attribute(AId::FontSize), it will get resolved correctly. So we could basically abstract away the complexity of shorthand resolving, which would mean no changes would be required in other parts of the codebase. The only tricky thing is to actually implement the segmenter that parses the font attribute.

Which brings me to my second point: I would've assumed that the best place to do this is within svgtypes (speaking of which, is there any chance that this crate could be moved to this GitHub repository and Cargo workspace? I think this would make sense since they are relatively intertwined, and this would save the hassle of having to submit PRs to two different repositories, but not sure if there is a good reason to keep them separate).

But it turns out that some properties such as FontWeight as well as their parsing logic are declared in usvg-tree/src/text.rs, which means I can't reuse them if I were to implement the parsing logic in svgtypes. Is there a reason why these aren't defined in svgtypes?

LaurenzV avatar Jan 15 '24 20:01 LaurenzV

Yes, overwriting of existing attributes was the idea. We do this already with the marker attribute. Handling the font attribute separately would be to much work. The hard part is the actual parsing.

I would've assumed that the best place to do this is within svgtypes

Yep.

speaking of which, is there any chance that this crate could be moved to this GitHub repository and Cargo workspace?

We could, but if you look at the commits history, svgtypes has like 10 commits per year. So having it in a separate repo makes sense to me. You just happen to interact with it a lot. From my perspective, 95% of work is done in the resvg repo. So it didn't bothered me at all. I will think about it.

But it turns out that some properties such as FontWeight as well as their parsing logic are declared in usvg-tree/src/text.rs

Yes, we could move them to svgtypes. Long time ago, text parsing and text-to-paths conversion was fully private, so it made sense, sort of. But not anymore. So it's just a legacy decision. Also, font-weight is a single-value property. Therefore there is not much "parsing" involved. It's just a single match.

And we should probably move font-family parsing to svgtypes as well.

RazrFalcon avatar Jan 15 '24 21:01 RazrFalcon

I see. I will experiment a bit!

LaurenzV avatar Jan 15 '24 21:01 LaurenzV

Hmm yeah, looks like it might be somewhat tricky to implement it...Because most of the current parsers seem to be based on the fact that there is only one type of argument expected which are also relatively simple in their nature (e.g. where it just consists in matching for keyword), but for the font shorthand we would basically need to combine multiple different parsers and also allow for different combinations of how to use them, which is kind of new.

The cleaner approach would probably be to use some parser-combinator library, but not sure if it's worth it to introduce a library just to make supporting this one shorthand easier (and I don't think there are many other that are missing)? So yeah, I guess just implementing it by hand somehow is the way to do it. :/

LaurenzV avatar Jan 19 '24 21:01 LaurenzV

Filter functions are somewhat complicated. You can look at those. I'm not sure what you mean by using multiple parsers. We could probably even ignore the "string parsing" part and return just strings. I.e. just:

struct Font {
    family: String, // or &str if possible
    size: String,
    stretch: String,
    style: String,
    variant: String,
    weight: String,
}

And the caller would handle the rest. Especially since we plan to implement font via attributes replace, so we do need strings anyway.

3rd-party libraries are out of scope.

RazrFalcon avatar Jan 20 '24 14:01 RazrFalcon