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.
(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.
(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
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.
> 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?
Created attachment 192236 [details] WIP I have a preliminary patch for this.
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?
Created attachment 196627 [details] Patch
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?
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.
*** Bug 115400 has been marked as a duplicate of this bug. ***
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.
Comment on attachment 196627 [details] Patch Clearing flags on attachment: 196627 Committed r149360: <http://trac.webkit.org/changeset/149360>
All reviewed patches have been landed. Closing bug.