Proper quoting of CSS 'font-family' fallback lists.

(bzr r13341.1.56)

5 files changed, 102 insertions, 86 deletions

diff --git a/src/libnrtype/font-lister.cpp b/src/libnrtype/font-lister.cpp
index 9ff4fad05..43c3045b1 100644
--- a/src/libnrtype/font-lister.cpp
+++ b/src/libnrtype/font-lister.cpp
@@ -601,8 +601,15 @@ std::pair<Glib::ustring, Glib::ustring> FontLister::new_font_family (Glib::ustri
 
       Glib::ustring family = ui.first;
 
-      sp_repr_css_set_property (css, "-inkscape-font-specification", fontspec.c_str() );
-      sp_repr_css_set_property (css, "font-family", family.c_str() ); //Canonized w/ spaces
+
+      // Font spec is single quoted... for the moment
+      Glib::ustring fontspec_quoted( fontspec );
+      css_quote( fontspec_quoted );
+      sp_repr_css_set_property (css, "-inkscape-font-specification", fontspec_quoted.c_str() );
+
+      // Font families needs to be properly quoted in CSS (used unquoted in font-lister)
+      css_font_family_quote( family );
+      sp_repr_css_set_property (css, "font-family", family.c_str() );
 
       PangoFontDescription *desc = pango_font_description_from_string( fontspec.c_str() );
       PangoWeight weight = pango_font_description_get_weight( desc );
diff --git a/src/style-internal.cpp b/src/style-internal.cpp
index 8b4f3c1cd..c686a1807 100644
--- a/src/style-internal.cpp
+++ b/src/style-internal.cpp
@@ -656,20 +656,21 @@ SPIString::read( gchar const *str ) {
         set = true;
         inherit = false;
 
-        // libcroco puts quotes around some strings... remove
-        Glib::ustring str_unquoted(str);
-        css_unquote( str_unquoted );
-
-        // Unquote individual family names, Pango always uses unquoted names.
+        Glib::ustring str_temp(str);
         if( name.compare( "font-family" ) == 0 ) {
-            css_font_family_unquote( str_unquoted );
+            // Family names may be quoted in CSS, internally we use unquoted names.
+            css_font_family_unquote( str_temp );
+        } else if( name.compare( "-inkscape-font-specification" ) == 0 ) {
+            css_unquote( str_temp );
         }
 
-        value = g_strdup(str_unquoted.c_str());
+        value = g_strdup(str_temp.c_str());
     }
 }
 
 
+// This routine is actually rarely used. Writing is done usually
+// in sp_repr_css_write_string...
 const Glib::ustring
 SPIString::write( guint const flags, SPIBase const *const base) const {
 
@@ -684,13 +685,13 @@ SPIString::write( guint const flags, SPIBase const *const base) const {
         } else {
             if( this->value ) {
                 if( name.compare( "font-family" ) == 0 ) {
-                    // This is for compatibilty with the C version of code.
-                    // This is incorrect as it puts single quotes around the
-                    // entire string rather around the individule font names.
-                    // This should be handled by the routines that extract
-                    // out the font family names and reassembles them into a
-                    // font fallback list. FIXME
-                    return (name + ":" + css2_escape_quote(this->value) + ";");
+                    Glib::ustring font_family( this->value );
+                    css_font_family_quote( font_family );
+                    return (name + ":" + font_family + ";");
+                } else if( name.compare( "-inkscape-font-specification" ) == 0 ) {
+                    Glib::ustring font_spec( this->value );
+                    css_quote( font_spec );
+                    return (name + ":" + font_spec + ";");
                 } else {
                     return (name + ":" + this->value + ";");
                 }
diff --git a/src/style.cpp b/src/style.cpp
index 8e4c89839..c6a98e7f4 100644
--- a/src/style.cpp
+++ b/src/style.cpp
@@ -1826,12 +1826,67 @@ sp_css_attr_scale(SPCSSAttr *css, double ex)
 }
 
 
+/**
+ * Quote and/or escape string for writing to CSS, changing strings in place.
+ * See: http://www.w3.org/TR/CSS21/syndata.html#value-def-identifier
+ */
+void
+css_quote(Glib::ustring &val)
+{
+    Glib::ustring out;
+    bool quote = false;
+
+    // Can't wait for C++11!
+    for( Glib::ustring::iterator it = val.begin(); it != val.end(); ++it) {
+        if(g_ascii_isalnum(*it) || *it=='-' || *it=='_' || *it > 0xA0) {
+            out += *it;
+        } else if (*it == '\'') {
+            // Single quotes require escaping and quotes.
+            out += '\\';
+            out += *it;
+            quote = true;
+        } else {
+            // Quote everything else including spaces.
+            // (CSS Fonts Level 3 recommends quoting with spaces.)
+            out += *it;
+            quote = true;
+        }
+        if( it == val.begin() && !g_ascii_isalpha(*it) ) {
+            // A non-ASCII/non-alpha initial value on any indentifier needs quotes.
+            // (Actually it's a bit more complicated but as it never hurts to quote...)
+            quote = true;
+        }
+    }
+    if( quote ) {
+        out.insert( out.begin(), '\'' );
+        out += '\'';
+    }
+    val = out;
+}
+
+
+/**
+ * Quote font names in font-family lists, changing string in place.
+ * We use unquoted names internally but some need to be quoted in CSS.
+ */
+void
+css_font_family_quote(Glib::ustring &val)
+{
+    std::vector<Glib::ustring> tokens = Glib::Regex::split_simple("\\s*,\\s*", val );
+
+    val.erase();
+    for( unsigned i=0; i < tokens.size(); ++i ) {
+        css_quote( tokens[i] );
+        val += tokens[i] + ", ";
+    }
+    if( val.size() > 1 )
+        val.erase( val.size() - 2 ); // Remove trailing ", "
+}
+
+
 // Called in style-internal.cpp, xml/repr-css.cpp
 /**
  * Remove paired single and double quotes from a string, changing string in place.
- * Note: in CSS (in style= and in stylesheets), unquoting and unescaping is done
- * by libcroco, our CSS parser, though it adds a new pair of "" quotes for the strings
- * it parsed for us.
  */
 void
 css_unquote(Glib::ustring &val)
@@ -1849,7 +1904,7 @@ css_unquote(Glib::ustring &val)
 /**
  * Remove paired single and double quotes from font names in font-family lists,
  * changing string in place.
- * Pango expects unquoted font family names. We use unquoted names in interface.
+ * We use unquoted family names internally but CSS sometimes uses quoted names.
  */
 void
 css_font_family_unquote(Glib::ustring &val)
@@ -1865,57 +1920,6 @@ css_font_family_unquote(Glib::ustring &val)
         val.erase( val.size() - 2 ); // Remove trailing ", "
 }
 
-// Called in style.cpp, xml/repr-css.cpp
-/**
- * Quote and/or escape string for writing to CSS (style=). Returned value must be g_free'd.
- */
-Glib::ustring css2_escape_quote(gchar const *val) {
-
-    Glib::ustring t;
-    bool quote = false;
-    bool last_was_space = false;
-
-    for (gchar const *i = val; *i; i++) {
-        bool is_space = ( *i == ' ' );
-        if (g_ascii_isalnum(*i) || *i=='-' || *i=='_') {
-            // ASCII alphanumeric, - and _ don't require quotes
-            t.push_back(*i);
-        } else if ( is_space && !last_was_space ) {
-            // non-consecutive spaces don't require quotes
-            t.push_back(*i);
-        } else if (*i=='\'') {
-            // single quotes require escaping and quotes
-            t.push_back('\\');
-            t.push_back(*i);
-            quote = true;
-        } else {
-            // everything else requires quotes
-            t.push_back(*i);
-            quote = true;
-        }
-        if (i == val && !g_ascii_isalpha(*i)) {
-            // a non-ASCII/non-alpha initial character requires quotes
-            quote = true;
-        }
-        last_was_space = is_space;
-    }
-
-    if (last_was_space) {
-        // a trailing space requires quotes
-        quote = true;
-    }
-
-    if (quote) {
-        // we use single quotes so the result can be stored in an XML
-        // attribute without incurring un-aesthetic additional quoting
-        // (our XML emitter always uses double quotes)
-        t.insert(t.begin(), '\'');
-        t.push_back('\'');
-    }
-
-    return t;
-}
-
 /*
   Local Variables:
   mode:c++
diff --git a/src/style.h b/src/style.h
index 506b90b44..931dcc90e 100644
--- a/src/style.h
+++ b/src/style.h
@@ -292,7 +292,9 @@ void sp_style_unset_property_attrs(SPObject *o);
 
 void sp_style_set_property_url (SPObject *item, gchar const *property, SPObject *linked, bool recursive);
 
+void css_quote( Glib::ustring &val );   // Add quotes around CSS values
 void css_unquote( Glib::ustring &val ); // Remove quotes from CSS values (style-internal.cpp, xml/repr-css.cpp)
+void css_font_family_quote( Glib::ustring &val ); // style-internal.cpp, text-toolbar.cpp
 void css_font_family_unquote( Glib::ustring &val ); // style-internal.cpp, text-toolbar.cpp
 
 Glib::ustring css2_escape_quote(gchar const *val);
diff --git a/src/xml/repr-css.cpp b/src/xml/repr-css.cpp
index e462f70da..c043904a7 100644
--- a/src/xml/repr-css.cpp
+++ b/src/xml/repr-css.cpp
@@ -283,14 +283,7 @@ void sp_repr_css_write_string(SPCSSAttr *css, Glib::ustring &str)
 
         str.append(g_quark_to_string(iter->key));
         str.push_back(':');
-        if (!strcmp(g_quark_to_string(iter->key), "font-family")
-                || !strcmp(g_quark_to_string(iter->key), "-inkscape-font-specification")) {
-            // we only quote font-family/font-specification, as SPStyle does
-            Glib::ustring val_quoted = css2_escape_quote (iter->value);
-            str.append(val_quoted);
-        } else {
-            str.append(iter->value); // unquoted
-        }
+        str.append(iter->value); // Any necessary quoting to be done by calling routine.
 
         if (rest(iter)) {
             str.push_back(';');
@@ -346,13 +339,23 @@ void sp_repr_css_merge(SPCSSAttr *dst, SPCSSAttr *src)
 
 /**
  * Merges style properties as parsed by libcroco into an existing SPCSSAttr.
+ * libcroco converts all single quotes to double quotes, which needs to be
+ * undone as we always use single quotes inside our 'style' strings since
+ * double quotes are used outside: e.g.:
+ *   style="font-family:'DejaVu Sans'"
  */
 static void sp_repr_css_merge_from_decl(SPCSSAttr *css, CRDeclaration const *const decl)
 {
     guchar *const str_value_unsigned = cr_term_to_string(decl->value);
 
-    Glib::ustring value_unquoted( reinterpret_cast<gchar *>(str_value_unsigned ) );
-    css_unquote( value_unquoted );  // libcroco returns strings quoted in "", remove
+    Glib::ustring value( reinterpret_cast<gchar *>(str_value_unsigned ) );
+    g_free(str_value_unsigned);
+
+    Glib::ustring::size_type pos = 0;
+    while( (pos=value.find("\"",pos)) != Glib::ustring::npos) {
+        value.replace(pos,1,"'");
+        ++pos;
+    }
 
     Glib::ustring units;
 
@@ -363,11 +366,11 @@ static void sp_repr_css_merge_from_decl(SPCSSAttr *css, CRDeclaration const *con
     *
     * HACK for now is to strip off em and ex units and add them back at the end
     */
-    int le = value_unquoted.length();
+    int le = value.length();
     if (le > 2) {
-        units = value_unquoted.substr(le-2, 2);
+        units = value.substr(le-2, 2);
         if ((units == "em") || (units == "ex")) {
-            value_unquoted = value_unquoted.substr(0, le-2);
+            value = value.substr(0, le-2);
         }
         else {
             units.clear();
@@ -378,7 +381,7 @@ static void sp_repr_css_merge_from_decl(SPCSSAttr *css, CRDeclaration const *con
     // CSSOStringStream is used here to write valid CSS (as in sp_style_write_string). This has
     // the additional benefit of respecting the numerical precission set in the SVG Output
     // preferences. We assume any numerical part comes first (if not, the whole string is copied).
-    std::stringstream ss( value_unquoted );
+    std::stringstream ss( value );
     double number = 0;
     std::string characters;
     std::string temp;
@@ -400,7 +403,6 @@ static void sp_repr_css_merge_from_decl(SPCSSAttr *css, CRDeclaration const *con
         //g_message("sp_repr_css_merge_from_decl looks like em or ex units %s --> %s", str_value, os.str().c_str());
     }
     ((Node *) css)->setAttribute(decl->property->stryng->str, os.str().c_str(), false);
-    g_free(str_value_unsigned);
 }
 
 /**