Bug 107231 – CSSParser::parseFontFamily should allow the keyword "default" as part of a font name

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Dominic Mazzoni]

If CSSParser::parseFontFamily encounters the tokens "default", "initial", or "inherit" as part of a CSS font-family declaration, it skips parsing the whole string.

The following html snippet demonstrates the problem:

<span style="font-family:arial,default serif,sans-serif">This line should use arial or sans-serif, but it does not as it contains "default" within the font-family</span>

If you change the word "default" to anything else, it parses fine and you get arial. If you leave the word default there, it skips the entire thing.

This bug probably existed before (with the keywords "initial" and "inherit", but the problem with the word "default" started at r125118 (https://bugs.webkit.org/show_bug.cgi?id=93491), so I'm hoping either Thiago or Kenneth will be able to help with a proper fix.

[Thiago Marcos P. Santos]

(In reply to comment #0)
> If CSSParser::parseFontFamily encounters the tokens "default", "initial", or "inherit" as part of a CSS font-family declaration, it skips parsing the whole string.
> 
> The following html snippet demonstrates the problem:
> 
> <span style="font-family:arial,default serif,sans-serif">This line should use arial or sans-serif, but it does not as it contains "default" within the font-family</span>
> 
> If you change the word "default" to anything else, it parses fine and you get arial. If you leave the word default there, it skips the entire thing.
> 
> This bug probably existed before (with the keywords "initial" and "inherit", but the problem with the word "default" started at r125118 (https://bugs.webkit.org/show_bug.cgi?id=93491), so I'm hoping either Thiago or Kenneth will be able to help with a proper fix.

I will have a look, thanks for reporting.

[Thiago Marcos P. Santos]

(In reply to comment #0)
> If CSSParser::parseFontFamily encounters the tokens "default", "initial", or "inherit" as part of a CSS font-family declaration, it skips parsing the whole string.
> 
> The following html snippet demonstrates the problem:
> 
> <span style="font-family:arial,default serif,sans-serif">This line should use arial or sans-serif, but it does not as it contains "default" within the font-family</span>
> 
> If you change the word "default" to anything else, it parses fine and you get arial. If you leave the word default there, it skips the entire thing.
> 
> This bug probably existed before (with the keywords "initial" and "inherit", but the problem with the word "default" started at r125118 (https://bugs.webkit.org/show_bug.cgi?id=93491), so I'm hoping either Thiago or Kenneth will be able to help with a proper fix.

Aren't you suppose to quote the default keyword?

Example: <span style="font-family:arial,'default' serif,sans-serif">

The spec says: Font family names that happen to be the same as a keyword value ('inherit', 'serif', 'sans-serif', 'monospace', 'fantasy', and 'cursive') must be quoted to prevent confusion with the keywords with the same names. The keywords 'initial' and 'default' are reserved for future use and must also be quoted when used as font names. UAs must not consider these keywords as matching the '<family-name>' type.

http://www.w3.org/TR/CSS21/fonts.html#font-family-prop

[Dominic Mazzoni]

I'm not sure I agree with that reading of the spec. To me, the spec says that the exact names "inherit", "default", etc. should never match font families that happen to have that name exactly, but it says nothing about a font family that contains the word "inherit" or "default" in the middle of their name.

Also, FWIW other browsers don't seem to require quotes in this case. They will match a font named "default serif" without needing quotes around anything.

[Vinod Seraphin]

> Aren't you suppose to quote the default keyword?
> 
> Example: <span style="font-family:arial,'default' serif,sans-serif">
> 
> The spec says: Font family names that happen to be the same as a keyword value ('inherit', 'serif', 'sans-serif', 'monospace', 'fantasy', and 'cursive') must be quoted to prevent confusion with the keywords with the same names. The keywords 'initial' and 'default' are reserved for future use and must also be quoted when used as font names. UAs must not consider these keywords as matching the '<family-name>' type.
> 
> http://www.w3.org/TR/CSS21/fonts.html#font-family-prop

No, I do not believe that is the proper way to interpret that section.  It is saying that if you want to reference the generic font family name you must use it without quotes, and if you want to reference an actual font named "serif" for instance, it must be quoted.
In other words
font-family:serif;
is very different from
font-family:'serif';

However, there is no ambiguity between
font-family:serif;
and
font-family:default serif;

Also earlier in the above spec section it does indeed says
Generic font family names are keywords and must NOT be quoted.

Quoting in the middle of a font name is very strange (and I believe invalid)...never seen that.  One workaround I've verified to this Chrome bug is to use 'default serif' but that should not be necessary.  It is not needed for any of the other web engines.

In the cited spec, it also states:
  For example, the following declarations are invalid:

  font-family: "Lucida" Grande, sans-serif;


When will this issue be fixed in webkit?

[Thiago Marcos P. Santos]

Created attachment 192236 [details]
WIP

I have a preliminary patch for this.

[Vinod Seraphin]

Looking at patch...
Checking for a nextValue should resolve the issue.  However, wondering if value->id is still going to be set to one of these constants (CSSValueInitial, CSSValueInherit or CSSValueDefault) when a word within a multi word font name has one of these values.  Isn't some other change required to only set that value when the entire font name value has this special value?

[Thiago Marcos P. Santos]

Created attachment 196627 [details]
Patch

[Darin Adler]

Comment on attachment 196627 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=196627&action=review

> Source/WebCore/css/CSSParser.cpp:5425
> +            if (nextValBreaksFont)
> +                value = m_valueList->next();
> +            else if (nextValIsFontName)
> +                value = nextValue;

I don’t understand this code. Since nextValue is the same as m_valueList->next(), why does one branch use one and one branch use the other?

[Thiago Marcos P. Santos]

Comment on attachment 196627 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=196627&action=review

>> Source/WebCore/css/CSSParser.cpp:5425
>> +                value = nextValue;
> 
> I don’t understand this code. Since nextValue is the same as m_valueList->next(), why does one branch use one and one branch use the other?

It is not the same thing. m_valueList->next() gets you a different value every time you call it because it increments the pointer to the current item.

CSSParserValue* next() { ++m_current; return current(); }

It was also tricky to me when I saw it for the first time in other parts of the code.

[Thiago Marcos P. Santos]

*** Bug 115400 has been marked as a duplicate of this bug. ***

[Ryosuke Niwa]

Comment on attachment 196627 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=196627&action=review

> Source/WebCore/ChangeLog:11
> +        This matches the behavior of Firefox and IE.

Does this also match the specification? If so, please add an URL to the specific version of the spec. you referenced here.

>>> Source/WebCore/css/CSSParser.cpp:5425
>>> +                value = nextValue;
>> 
>> I don’t understand this code. Since nextValue is the same as m_valueList->next(), why does one branch use one and one branch use the other?
> 
> It is not the same thing. m_valueList->next() gets you a different value every time you call it because it increments the pointer to the current item.
> 
> CSSParserValue* next() { ++m_current; return current(); }
> 
> It was also tricky to me when I saw it for the first time in other parts of the code.

We really need to rewrite this function. The code is incomphrensible as is.

[WebKit Commit Bot]

Comment on attachment 196627 [details]
Patch

Clearing flags on attachment: 196627

Committed r149360: <http://trac.webkit.org/changeset/149360>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.