Since QFont always falls back to something, QtWebKit currently only tries the first font family from a given CSS. Depending on the fonts installed, this is something that can be reproduced (for me at least) on websites such as: http://whatthecommit.com/ http://code.google.com/more/table/
Assigning to Pierre since he is already working on a fix.
Created attachment 83471 [details] proposed fix (depends on another patch yet to land in Qt master)
Comment on attachment 83471 [details] proposed fix (depends on another patch yet to land in Qt master) I don't understand what this is doing. How do we test this?
(In reply to comment #3) > (From update of attachment 83471 [details]) > I don't understand what this is doing. How do we test this? The issue in QtWebKit (and subsequently Rekonq) is that QFont always falls back to something, and eventually it might be the default font, and not what we want at all. In the examples I cited, take whatthecommit.com for instance: on my system, the request for "Lucida Console" defaults to "DejaVu Sans", so it never even tries "Courier New" and the rendering looks very different from what I get in Chromium or Firefox. As for the testing part, well I believe there are tests already, and I suspect a bunch of them might break because of this, but that would be because the "expected" results were not quite correct in the first place IMHO.
(In reply to comment #3) > (From update of attachment 83471 [details]) > I don't understand what this is doing. How do we test this? Oh, I forgot to point there: http://bugreports.qt.nokia.com/browse/QTBUG-15575 to find the patches required on top of Qt for this to work.
Adding as nice to have for QtWebKit 2.2 release. If the API is in Qt, it would be stupid not to fix the bug.
Unfortunately that's not something that can be fixed easily for 2.2 at least since 4.8 is feature frozen and I didn't push to add the required function because of the new raw font approach that should have made that useless.
Created attachment 101713 [details] patch v2 This might require synchronizing for the the bots since the #ifdef will break the build until Qt is bumped to have QFontDatabase::hasFamily(). And an obscene number of layout tests results are going to need to be updated as a result, not sure how to do that properly.
Comment on attachment 101713 [details] patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=101713&action=review > Source/WebCore/ChangeLog:16 > + No new tests. (OOPS!) Remove this line. Instead, you should write this is covered by existing tests.
Andreas, Robert, any comment?
Hi Pierre, This is the same as 36351, I think you should add the test from there or some other suitable test. Otherwise looks fine to me!
Comment on attachment 101713 [details] patch v2 View in context: https://bugs.webkit.org/attachment.cgi?id=101713&action=review > Source/WebCore/ChangeLog:9 > + the requestfor a given font family can be s/requestfor/request for
(In reply to comment #11) > Hi Pierre, > This is the same as 36351, I think you should add the test from there or some other suitable test. Otherwise looks fine to me! I'm afraid that test won't work with just the font fallback getting fixed, but is going to with another patch I have in store (essentially it just requires loading the data we get with QRawFont). Can I steal it for that other change then ?
(In reply to comment #13) > (In reply to comment #11) > > Hi Pierre, > > This is the same as 36351, I think you should add the test from there or some other suitable test. Otherwise looks fine to me! > > I'm afraid that test won't work with just the font fallback getting fixed, but is going to with another patch I have in store (essentially it just requires loading the data we get with QRawFont). Can I steal it for that other change then ? Oh OK. The test I'm referring to is fast/css/font-face-invalid-local-source.html in the patch, btw. I would have expected this change to make it pass, but it's been a while.
Created attachment 103235 [details] Patch Ok then, just changed the ChangeLog about tests.
Comment on attachment 103235 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=103235&action=review r- to get some more polish up in here. :) > Source/WebCore/ChangeLog:3 > + Fix the QFont fallback issue. That could mean a lot of things. The bug title would fit very nicely here instead. > Source/WebCore/ChangeLog:12 > + This is a showstopper to get webfonts working > + properly in QtWebKit. This is bug tracker talk, and can be excluded from the ChangeLog. > Source/WebCore/ChangeLog:18 > + This change should be covered by existing tests, > + but there's a good chance some of these tests > + will in turn need to be fixed. We should strive to be more certain in ChangeLog entries. Run the tests with your change and find out what breaks, then (if applicable) include the updated qt-4.8 baselines in the patch. > Source/WebCore/platform/graphics/qt/FontCacheQt.cpp:114 > +#if QT_VERSION >= QT_VERSION_CHECK(4, 8, 0) > + QFontDatabase db; > + if (!db.hasFamily(familyName)) > + return 0; > +#endif This looks good to me. Is the API available on the Qt-4.8 used by our bots though?
Created attachment 103862 [details] fix using QFont::exactMatch I think my bug 66186 is a duplicate of this, I really need a fix that works with Qt 4.7 for that. Attached patch uses QFont::exactMatch() on the FontPlatformData::font() to check if it is the font we asked for, and return 0 if not.
*** Bug 66186 has been marked as a duplicate of this bug. ***
Comment on attachment 103862 [details] fix using QFont::exactMatch Never mind, just read the discussion in bug 36351 on why exactMatch() won't work.
(In reply to comment #16) > > Source/WebCore/ChangeLog:18 > > + This change should be covered by existing tests, > > + but there's a good chance some of these tests > > + will in turn need to be fixed. > > We should strive to be more certain in ChangeLog entries. > Run the tests with your change and find out what breaks, then (if applicable) include the updated qt-4.8 baselines in the patch. Agreed, I just remembered it made the layout tests unhappy. Now I re-ran the tests and it breaks more than four thousand of them. Is there a nice way to update all of that ?
(In reply to comment #20) > (In reply to comment #16) > > > > Source/WebCore/ChangeLog:18 > > > + This change should be covered by existing tests, > > > + but there's a good chance some of these tests > > > + will in turn need to be fixed. > > > > We should strive to be more certain in ChangeLog entries. > > Run the tests with your change and find out what breaks, then (if applicable) include the updated qt-4.8 baselines in the patch. > > Agreed, I just remembered it made the layout tests unhappy. Now I re-ran the tests and it breaks more than four thousand of them. Is there a nice way to update all of that ? You can ask Ossy for help, he has a talent at keeping bots happy ;) But I think the comment here is more that the Changelog is not good enough. It is your job to make sure a patch is tested correctly. Then you detail it in the Changelog. Here the Changelog reads like you have no clue if the patch is tested properly.
4000 sounds like too many to be right - the tests must be rendering with a different (possibly more accurate) font now. The best approach may be to patch DumpRenderTree to ensure the same fonts as before are used (for now). Normally you would use the QBat-VM in a case like this: http://webkit.sed.hu/blog/20101028/qtwebkit-builder-and-tester-virtual-machine But it doesn't have a 4.8 environment. You should post a link to some of the failures maybe, in case there's something else wrong.
Created attachment 104041 [details] Patch From looking at the results, I think there are so many tests effected by a change in font matching and hence font metrics, 4000 might not be that shocking after all.
Comment on attachment 104041 [details] Patch The 4.8 version on the bot is too old for now, so I guess this will have to wait a little longer.
(In reply to comment #24) > (From update of attachment 104041 [details]) > The 4.8 version on the bot is too old for now, so I guess this will have to wait a little longer. You can land something now and update the results when the bots get updated.
Comment on attachment 104041 [details] Patch r=me Do exercise caution (with regard to the bots) when landing this.
Unfortunately it is more complex problem that I thought previously. :(( I sent a mail to webkit-qt mailing list about it: https://lists.webkit.org/pipermail/webkit-qt/2011-September/001864.html
Check also this upstream Qt bug: https://bugreports.qt.nokia.com/browse/QTBUG-21036
Patch landed in: http://trac.webkit.org/changeset/97990 ~6000 rebased expected files landed in: http://trac.webkit.org/changeset/97994 http://trac.webkit.org/changeset/97995 http://trac.webkit.org/changeset/98015 http://trac.webkit.org/changeset/98018 http://trac.webkit.org/changeset/98072 http://trac.webkit.org/changeset/98078