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.
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.
Created attachment 51534 [details] Automatic layout test case The expected output of this test case is "PASS".
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).
Thanks for filing this bug and including a nice automated test case. I should have a patch up shortly.
Created attachment 68032 [details] Patch for this issue
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!)
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.
(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.
Created attachment 68290 [details] Updated patch with small fix for bold fonts
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 on attachment 68290 [details] Updated patch with small fix for bold fonts Very good!
Committed r68406: <http://trac.webkit.org/changeset/68406>
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
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?
(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
(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 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.
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.
(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.
I'll ask about relicensing. I don't think it'll be an issue, just wanna double-check.