Bug 64783 – @font-face: unquoted local font names containing spaces don't work

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

[Kenichi Ishibashi]

See the chromium bug http://crbug.com/89609 for details.

The specification (http://www.w3.org/TR/css3-fonts/#src-desc) mentions--referring to local():

> The name can optionally be enclosed in quotes.

And later gives this example:

/* bold face of Gentium */
@font-face {
  font-family: MyGentium;
  src: local(Gentium Bold),   /* full font name */
       local(Gentium-Bold),   /* Postscript name */
       url(GentiumBold.ttf);  /* otherwise, download it */
  font-weight: bold;
}

[Kenichi Ishibashi]

Created attachment 101271 [details]
Patch V0

[Kenichi Ishibashi]

Created attachment 116155 [details]
Patch

[Kenichi Ishibashi]

(In reply to comment #2)
> Created an attachment (id=116155) [details]
> Patch

Just rebased to ToT.

[Ryosuke Niwa]

Comment on attachment 116155 [details]
Patch

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

> Source/WebCore/ChangeLog:6
> +        Allows unquoted local font names contains spaces.

"local font names with spaces" or "local font names containing spaces".

> Source/WebCore/css/CSSParser.cpp:4366
> +                if (args->current()->unit == CSSPrimitiveValue::CSS_STRING) {
> +                    parsedValue = CSSFontFaceSrcValue::createLocal(args->current()->string);
> +                } else if (args->current()->unit == CSSPrimitiveValue::CSS_IDENT) {

No { } around a single statement.

> Source/WebCore/css/CSSParser.cpp:4370
> +                            failed = true;

Are you sure this is correct? What if we had a valid token after local() e.g. local(~~ non-ident) src(valid value)

[Kenichi Ishibashi]

Created attachment 116865 [details]
Patch

[Kenichi Ishibashi]

Comment on attachment 116155 [details]
Patch

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

Thanks for review! Revised the patch.

>> Source/WebCore/ChangeLog:6
>> +        Allows unquoted local font names contains spaces.
> 
> "local font names with spaces" or "local font names containing spaces".

Done.

>> Source/WebCore/css/CSSParser.cpp:4366
>> +                } else if (args->current()->unit == CSSPrimitiveValue::CSS_IDENT) {
> 
> No { } around a single statement.

Done.

>> Source/WebCore/css/CSSParser.cpp:4370
>> +                            failed = true;
> 
> Are you sure this is correct? What if we had a valid token after local() e.g. local(~~ non-ident) src(valid value)

I couldn't find how should we treat that case in the spec draft, but Firefox rejects it. I followed Firefox's manner.

[Ryosuke Niwa]

(In reply to comment #6)
> >> Source/WebCore/css/CSSParser.cpp:4370
> >> +                            failed = true;
> > 
> > Are you sure this is correct? What if we had a valid token after local() e.g. local(~~ non-ident) src(valid value)
> 
> I couldn't find how should we treat that case in the spec draft, but Firefox rejects it. I followed Firefox's manner.

Please add a test case for this, and also mention it in the change log.

[Ryosuke Niwa]

Comment on attachment 116865 [details]
Patch

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

> Source/WebCore/css/CSSParser.cpp:4380
>          } else if (val->unit == CSSParserValue::Function) {

You should put this entire block in a helper function.

> Source/WebCore/css/CSSParser.cpp:4386
> +            // There are two allowed functions: local() and format().

A better way to document this is by asserting that.

> Source/WebCore/css/CSSParser.cpp:4404
> +                    parsedValue = CSSFontFaceSrcValue::createLocal(builder.toString());

You should check that failed is false here. Otherwise, we don't bail out early on line 4426.

[Kenichi Ishibashi]

Comment on attachment 116865 [details]
Patch

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

Thank you for the review! I have one question (please see below).

>> Source/WebCore/css/CSSParser.cpp:4380
>>          } else if (val->unit == CSSParserValue::Function) {
> 
> You should put this entire block in a helper function.

Umm, the block contains many references to local variables that are used entire the function so I think just putting this block in a helper function doesn't make things clear. It might be better to refactor the whole function, but I'm not sure it's worth to do it just for fixing this problem. Should I do this? If so, I'll try it.

>> Source/WebCore/css/CSSParser.cpp:4386
>> +            // There are two allowed functions: local() and format().
> 
> A better way to document this is by asserting that.

Asserting isn't good here because the input comes from stylesheets and could contain arbitrary values.

>> Source/WebCore/css/CSSParser.cpp:4404
>> +                    parsedValue = CSSFontFaceSrcValue::createLocal(builder.toString());
> 
> You should check that failed is false here. Otherwise, we don't bail out early on line 4426.

Will do.

[Ryosuke Niwa]

Comment on attachment 116865 [details]
Patch

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

>>> Source/WebCore/css/CSSParser.cpp:4380
>>>          } else if (val->unit == CSSParserValue::Function) {
>> 
>> You should put this entire block in a helper function.
> 
> Umm, the block contains many references to local variables that are used entire the function so I think just putting this block in a helper function doesn't make things clear. It might be better to refactor the whole function, but I'm not sure it's worth to do it just for fixing this problem. Should I do this? If so, I'll try it.

Could be a follow up patch.

>>> Source/WebCore/css/CSSParser.cpp:4386
>>> +            // There are two allowed functions: local() and format().
>> 
>> A better way to document this is by asserting that.
> 
> Asserting isn't good here because the input comes from stylesheets and could contain arbitrary values.

That means this comment is incorrect, right? If you meant to say that we only accept local and format functions here, then that's just redundant; the code already tells us that much.

[Kenichi Ishibashi]

Created attachment 117115 [details]
Patch

[Kenichi Ishibashi]

Created attachment 117117 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 117117 [details]
Patch for landing

Clearing flags on attachment: 117117

Committed r101504: <http://trac.webkit.org/changeset/101504>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.