Bug 36548 – [GTK] Wrong font instantiated from an unknown font family

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Olivier Tilloy]

Created attachment 51532 [details]
layout test case

According to the CSS fonts specification (http://www.w3.org/TR/css3-fonts/#font-family-the-font-family-property), "[the
font-family property] specifies a prioritized list of font family names or generic family names", and "authors are encouraged to append a generic font family as a last alternative for improved robustness".

If a font family in the list doesn't match any known font on the system, the next family in the list should be used. The current implementation in WebKitGTK seems to always find a match for the first family in the list (falling back to the generic font family if necessary), whether it matches an actual existing font or not. Consequently, other families in the list are never given a chance to render the text as intended.

Consider the attached layout test case: it can reasonably be assumed that "UnknownFontFamily" doesn't match any known font on the system, therefore the text should be rendered using the second family in the list, Courier, which is a monospace font. Instead it is rendered using the generic family, serif.

[Olivier Tilloy]

Note that the attached layout test case can easily be turned into an automatic test for the test suite by making the two <p> elements display inline and comparing their rendered width for equality using some javascript.

[Olivier Tilloy]

Created attachment 51534 [details]
Automatic layout test case

The expected output of this test case is "PASS".

[Olivier Tilloy]

It looks like the same problem also exists in the Qt port: the layout test fails in konqueror 4.2. However it passes in safari 4.0 on windows as well as in chrome on linux. All gecko-based browsers tested behave as expected (the test passes).

[Martin Robinson]

Thanks for filing this bug and including a nice automated test case. I should have a patch up shortly.

[Martin Robinson]

Created attachment 68032 [details]
Patch for this issue

[Martin Robinson]

This will likely need to be landed along with the patch in https://bugs.webkit.org/show_bug.cgi?id=46038 with new baselines, as both of these issues have conspired to create many incorrect DRT dumps. If they are landed separately, we will need to make new baselines twice.

Here's an example of what's happening for fast/borders/border-image-border-radius.html without fixing fonts.conf:

WebCore requests font family: Times
    family name after config: Times (no alias for Times)
    family name after matching: Nimbus Roman No9 L (fallback for Times -- rejected)

WebCore requests font family:Times New Roman (WebCore CSS fallback)
    family name after config:Times New Roman
    family name after matching:Ahem (fallback for Times New Roman -- rejected)

family name:serif (last fallback)
    family name after config:serif
    family name after matching:Ahem (accepted, because "serif" allows falling back)

The correct trace, with a fixed fonts.conf, would look like:
WebCore requests font family: Times
    family name after config: Liberation Serif (aliased to Times)
    family name after matching: Liberation Serif (matched! success!)

[Olivier Tilloy]

Nice Martin!
I haven’t tested the patch yet, but I went through it very quickly and there is a typo in LayoutTests/platform/gtk/fonts/font-family-fallback.html: s/dimenions/dimensions/. Other than that I’m definitely not competent to validate the correctness of the patch but I’ll test it functionally.

[Martin Robinson]

(In reply to comment #7)

> I haven’t tested the patch yet, but I went through it very quickly and there is a typo in LayoutTests/platform/gtk/fonts/font-family-fallback.html: s/dimenions/dimensions/. 

Thanks! I'll make sure to fix that.

[Martin Robinson]

Created attachment 68290 [details]
Updated patch with small fix for bold fonts

[Martin Robinson]

Comment on attachment 68290 [details]
Updated patch with small fix for bold fonts

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

> WebCore/platform/graphics/cairo/SimpleFontDataCairo.cpp:86
> +    // FIXME(mrobinson): I think we want to ask FontConfig for the right font again.

I will change this to just say FIXME.

[Gustavo Noronha (kov)]

Comment on attachment 68290 [details]
Updated patch with small fix for bold fonts

Very good!

[Martin Robinson]

Committed r68406: <http://trac.webkit.org/changeset/68406>

[WebKit Review Bot]

http://trac.webkit.org/changeset/68406 might have broken GTK Linux 64-bit Debug
The following changes are on the blame list:
http://trac.webkit.org/changeset/68403
http://trac.webkit.org/changeset/68404
http://trac.webkit.org/changeset/68405
http://trac.webkit.org/changeset/68406

[Holger Freyther]

Hmm the google code and yours look really similiar. Are you sure it is not copyrighted by Google? The problem is that AFAIK Apachae License is not compatible with LGPL, is it compatible with LGPL?

[Martin Robinson]

(In reply to comment #14)
> Hmm the google code and yours look really similiar. Are you sure it is not copyrighted by Google? The problem is that AFAIK Apachae License is not compatible with LGPL, is it compatible with LGPL?

Thank you for bringing this to my attention. I believe I have removed the big block comment and the similar code here: http://trac.webkit.org/changeset/69528

[Tony Chang]

(In reply to comment #14)
> Hmm the google code and yours look really similiar. Are you sure it is not copyrighted by Google? The problem is that AFAIK Apachae License is not compatible with LGPL, is it compatible with LGPL?

Out of curiosity, what code are you referring to that is under the Apache License?

[Martin Robinson]

The large block comment was originally from SkFontHost_fontconfig.cpp and Holger felt that some other bits of code were too similiar to the Skia implementation. In a recently-landed cleanup patch I removed the comment and rewrote the other code.

[Martin Robinson]

If there is still a problem with this section of code, I think it should be removed now and rewritten by someone who has not seen the original Skia code. Otherwise, if it was possible for someone at Google to relicense it under a compatible license, that would be very kind.

[Holger Freyther]

(In reply to comment #16)
> (In reply to comment #14)
> > Hmm the google code and yours look really similiar. Are you sure it is not copyrighted by Google? The problem is that AFAIK Apachae License is not compatible with LGPL, is it compatible with LGPL?
> 
> Out of curiosity, what code are you referring to that is under the Apache License?

The code had a pointer to the url below. I agree that the actual matching (Font Config calls) can not be really different, and I am not sure that one can copyright the comment and on top of that I am not sure if LGPL2.x is compatible with the Apache License, I know that GPL is not compatible though.

http://code.google.com/p/skia/source/browse/trunk/src/ports/SkFontHost_fontconfig.cpp?r=587

BTW: thanks for cleaning it up.

[Evan Martin]

I'll ask about relicensing.  I don't think it'll be an issue, just wanna double-check.