GNOME Bugzilla – Bug 607931
[wikimedia] Support CSS2 shortand 'font' property
Last modified: 2017-12-13 17:37:19 UTC
Currently librsvg does not support the CSS2 shorthand font property as described in http://www.w3.org/TR/CSS2/fonts.html#font-shorthand It would be nice if librsvg could support this, to enhance it's compatibility. E.g. Protovis (http://protovis.org) generated SVGs are not rendered correctly.
Created attachment 303828 [details] [review] This patch implements a missing SVG feature as described in Gnome Bugzilla bug #607931 where support for shorthand font properties is demanded. This patch implements a missing SVG feature as described in Gnome Bugzilla bug #607931 where support for shorthand font properties is demanded. Structure of shorthand font properties is described in CSS2 specification. See http://www.w3.org/TR/CSS2/fonts.html#font-shorthand. This patch adds a parser for shorthand font properties but has limitations with quoted font names. As librsvg in general has such problems as described in Gnome Bugzilla bug #739329 it's considered acceptable here too.
Review of attachment 303828 [details] [review]: The code looks mostly fine for me, but here's some (non-maintainer) feedback, based mostly on preferences. I'll leave the final word of agreeing/disagreeing with it to the maintainer. ::: rsvg-styles.c @@ -461,0 +461,6 @@ +/* + A function for parsing a string declaring shorthand font properties. It has + limitations with qouted font-family names and it expects all quotations ... 3 more ... I guess you only need one before here, not before before. @@ -461,0 +461,22 @@ +/* + A function for parsing a string declaring shorthand font properties. It has + limitations with qouted font-family names and it expects all quotations ... 19 more ... I see no point of keeping the g_free(remainder) in both the if and the else branch, I guess it could simply be moved before the if. @@ -461,0 +461,42 @@ +/* + A function for parsing a string declaring shorthand font properties. It has + limitations with qouted font-family names and it expects all quotations ... 39 more ... This could be combined with the previous else...if branch with ||, as both use the same body, with rsvg_css_parse_font_style. @@ -461,0 +461,62 @@ +/* + A function for parsing a string declaring shorthand font properties. It has + limitations with qouted font-family names and it expects all quotations ... 59 more ... The previous four if...else branches also have the same body, you could prepend those to this else if branch condition with ||.
Created attachment 305191 [details] [review] This patch implements a missing SVG feature as described in Gnome Bugzilla bug #607931. This patch implements a missing SVG feature as described in Gnome Bugzilla bug #607931 where support for shorthand font properties is demanded. Structure of shorthand font properties is described in CSS2 specification. See http://www.w3.org/TR/CSS2/fonts.html#font-shorthand. This patch adds a parser for shorthand font properties but has limitations with quoted font names. As librsvg in general has such problems as described in Gnome Bugzilla bug #739329 it's considered acceptable here too.
Hello Robert! Thanks for your review. Three of your reviews are totally right and I have changed them in my last patch. I omitted your second review about moving g_free() outside if-else branch because this is a function working with memory allocation and pointers. Every direct work on memory is dangerous so I kept the free and replace operation together.
Created attachment 306795 [details] Testing various combination of shorthand font properties @Federico: I've seen you added tests on my other patches when you commit.
I don't think that shorthand properties can be used as presentation attributes. In the svg1.1 testsuite there is a test for the 'marker' short-hand property used as attribute (painting-marker-04-f.svg) and like the 'marker' the 'font' shorthand is not listed here: https://www.w3.org/TR/SVG/attindex.html#PresentationAttributes In other words I'd drop this hunk: https://bugzilla.gnome.org/attachment.cgi?id=305191&action=diff#a/rsvg-styles.c_sec3 I did not test this patch though.
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/librsvg/issues/34.