Bug 36548 - [GTK] Wrong font instantiated from an unknown font family
Summary: [GTK] Wrong font instantiated from an unknown font family
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on: 46038
Blocks: 33299 42052
  Show dependency treegraph
 
Reported: 2010-03-24 11:54 PDT by Olivier Tilloy
Modified: 2010-10-12 09:39 PDT (History)
11 users (show)

See Also:


Attachments
layout test case (241 bytes, text/html)
2010-03-24 11:54 PDT, Olivier Tilloy
no flags Details
Automatic layout test case (651 bytes, text/html)
2010-03-24 12:40 PDT, Olivier Tilloy
no flags Details
Patch for this issue (25.20 KB, patch)
2010-09-19 12:31 PDT, Martin Robinson
no flags Details | Formatted Diff | Diff
Updated patch with small fix for bold fonts (25.40 KB, patch)
2010-09-21 14:11 PDT, Martin Robinson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Olivier Tilloy 2010-03-24 11:54:38 PDT
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.
Comment 1 Olivier Tilloy 2010-03-24 11:58:59 PDT
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.
Comment 2 Olivier Tilloy 2010-03-24 12:40:34 PDT
Created attachment 51534 [details]
Automatic layout test case

The expected output of this test case is "PASS".
Comment 3 Olivier Tilloy 2010-03-24 14:10:23 PDT
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).
Comment 4 Martin Robinson 2010-09-19 12:18:59 PDT
Thanks for filing this bug and including a nice automated test case. I should have a patch up shortly.
Comment 5 Martin Robinson 2010-09-19 12:31:32 PDT
Created attachment 68032 [details]
Patch for this issue
Comment 6 Martin Robinson 2010-09-19 12:38:13 PDT
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!)
Comment 7 Olivier Tilloy 2010-09-19 23:44:38 PDT
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.
Comment 8 Martin Robinson 2010-09-20 08:02:42 PDT
(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.
Comment 9 Martin Robinson 2010-09-21 14:11:47 PDT
Created attachment 68290 [details]
Updated patch with small fix for bold fonts
Comment 10 Martin Robinson 2010-09-24 10:42:00 PDT
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.
Comment 11 Gustavo Noronha (kov) 2010-09-24 10:58:48 PDT
Comment on attachment 68290 [details]
Updated patch with small fix for bold fonts

Very good!
Comment 12 Martin Robinson 2010-09-27 11:01:37 PDT
Committed r68406: <http://trac.webkit.org/changeset/68406>
Comment 13 WebKit Review Bot 2010-09-27 12:04:24 PDT
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
Comment 14 Holger Freyther 2010-10-10 13:29:46 PDT
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?
Comment 15 Martin Robinson 2010-10-11 15:03:19 PDT
(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
Comment 16 Tony Chang 2010-10-11 15:47:48 PDT
(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?
Comment 17 Martin Robinson 2010-10-11 16:30:50 PDT
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.
Comment 18 Martin Robinson 2010-10-11 18:31:28 PDT
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.
Comment 19 Holger Freyther 2010-10-12 07:24:56 PDT
(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.
Comment 20 Evan Martin 2010-10-12 09:39:08 PDT
I'll ask about relicensing.  I don't think it'll be an issue, just wanna double-check.