This is a follow up of https://bugs.webkit.org/show_bug.cgi?id=140290#c17 Note that this was reported before also for the extinct QtWebkit https://bugs.webkit.org/show_bug.cgi?id=41474, so perhaps is a freetype issue.
Created attachment 245525 [details] Minimal example of bad font rendering
BTW, this looks fine for me in Firefox but it isn't using the woff fonts mentioned in @font-face. In my system Firefox falls back to using Nimbus font. So it seems that the issue is that we aren't using the web fonts...
They're being loaded though; if you open attachment #245525 [details] and check the web inspector, you'll see insecure content warnings (which show that the insecure access is permitted).
Thanks for the minimal example! Sergio, can you post a screenshot of the output? I also have Nimbus fonts installed and I cannot reproduce the issue. Another thing that would be really useful is the output of "fc-match Helvetica."
Created attachment 245556 [details] screenshot
(In reply to comment #2) > BTW, this looks fine for me in Firefox but it isn't using the woff fonts > mentioned in @font-face. In my system Firefox falls back to using Nimbus > font. It's because you are trying to use fonts loaded over http:// from https:// bugs.webkit.org (or from a local page), so Firefox blocks them as mixed content. We just warn.
(In reply to comment #6) > (In reply to comment #2) > > BTW, this looks fine for me in Firefox but it isn't using the woff fonts > > mentioned in @font-face. In my system Firefox falls back to using Nimbus > > font. > > It's because you are trying to use fonts loaded over http:// from https:// > bugs.webkit.org (or from a local page), so Firefox blocks them as mixed > content. We just warn. I don't think so... I just uploaded the test case here (plain http): http://people.igalia.com/clopez/wkbug/140994/attachment_245525.html And this is how it looks: http://people.igalia.com/clopez/wkbug/140994/fonts_webkitgtk_firefox_chromium.png The window on the left is the WebKitGTK+ Minibrowser (trunk), the one in the middle is firefox (iceweasel rebranded from Debian), and the one at the right chromium (from Debian also)
(In reply to comment #4) > Another thing that would be really useful is the output of "fc-match > Helvetica." $ fc-match Helvetica texgyreheros-regular.otf: "TeX Gyre Heros" "Regular" $ fc-match Helvetica. DejaVuSans.ttf: "DejaVu Sans" "Book"
(In reply to comment #7) > And this is how it looks: > http://people.igalia.com/clopez/wkbug/140994/ > fonts_webkitgtk_firefox_chromium.png Now Firefox is still blocking those fonts, this time due to its same origin policy. It gives you nice reasons if you open its web inspector. (WebKit is less strict and lets the fonts through.) Anyway, Martin discovered that the fonts aren't being hinted on my computer, and he has a patch that makes this example and CNN both look good for me. But we don't know why they look good without the patch for Martin.
(In reply to comment #9) > (In reply to comment #7) > > And this is how it looks: > > http://people.igalia.com/clopez/wkbug/140994/ > > fonts_webkitgtk_firefox_chromium.png > > Now Firefox is still blocking those fonts, this time due to its same origin > policy. It gives you nice reasons if you open its web inspector. (WebKit is > less strict and lets the fonts through.) Yeah well, but that does not apply to the original site. In any case you can download the fonts to a server and place the html there. The issue is still present.
Martin, were you planning to post your patch for this issue?
Created attachment 246847 [details] test patch So that we don't lose it, here is Martin's patch that works for me. We're still trying to figure out if this patch is the right thing to do (in particular, why the text looks good for Martin but not for me without the patch).
So, from [1]: * "By default, hinting is enabled and the font's native hinter (see FT_FACE_FLAG_HINTER) is preferred over the auto-hinter. You can disable hinting by setting FT_LOAD_NO_HINTING or change the precedence by setting FT_LOAD_FORCE_AUTOHINT. You can also set FT_LOAD_NO_AUTOHINT in case you don't want the auto-hinter to be used at all." So sounds like we should use the native hinter (i.e. don't pass FT_LOAD_FORCE_AUTOHINT, i.e. don't change anything) unless we have a good reason not to (e.g. everything looks terrible without it). * FT_FACE_FLAG_HINTER: "Set if the font driver has a hinting machine of its own. For example, with TrueType fonts, it makes sense to use data from the SFNT ‘gasp’ table only if the native TrueType hinting engine (with the bytecode interpreter) is available and active." [1] http://www.freetype.org/freetype2/docs/reference/ft2-base_interface.html
Created attachment 247101 [details] [GTK] Fonts loaded via @font-face look bad
Comment on attachment 246847 [details] test patch I think fontconfig was not being checked at all when loading web fonts. This patch seems to work. I tested cnn.com and rachelandrew.co.uk and they both look much better now.
Comment on attachment 247101 [details] [GTK] Fonts loaded via @font-face look bad View in context: https://bugs.webkit.org/attachment.cgi?id=247101&action=review It also works fine for me. Is this Martin's patch? Because it'd be weird to ask him to review it if that's the case :). > Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:122 > + // Strategy from Behdad Esfahbod in https://code.google.com/p/chromium/issues/detail?id=173207#c35 What about what he says in comment#36? "Though, of course this doesn't take into account the settings set by GNOME/KDE using XSETTINGS."
No, this one is my patch; Martin's was the one I labeled "test patch." I was about to say "we don't look at the GTK+ font settings at all" -- we certainly do not do so in the freetype platform backend -- but actually I'm simply wrong. If I turn off antialiasing in Tweak Tool then the pages we load look terrible. If I turn hinting down from Medium to Slight (this triggers use of the freetype autohinter instead of the font's native hinting) then the problem with these fonts goes away. I don't know where we are checking these settings though.
(In reply to comment #17) > No, this one is my patch; Martin's was the one I labeled "test patch." > > I was about to say "we don't look at the GTK+ font settings at all" -- we > certainly do not do so in the freetype platform backend -- but actually I'm > simply wrong. If I turn off antialiasing in Tweak Tool then the pages we > load look terrible. If I turn hinting down from Medium to Slight (this > triggers use of the freetype autohinter instead of the font's native > hinting) then the problem with these fonts goes away. I don't know where we > are checking these settings though. Hrm. This sounds like a bug. :/
Comment on attachment 247101 [details] [GTK] Fonts loaded via @font-face look bad View in context: https://bugs.webkit.org/attachment.cgi?id=247101&action=review Looks good, though I have some minor suggestions. > Source/WebCore/ChangeLog:9 > + Reviewed by NOBODY (OOPS!). > + > + No new tests because I haven't found a freely-distributable font that triggers the bug. > + This is a great change, but I think it deserves a full explanation in the changelog which describes the change and why it fixes the problem. > Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:129 > + RefPtr<FcPattern> pattern = adoptRef(FcPatternCreate()); > + FcConfigSubstitute(nullptr, pattern.get(), FcMatchPattern); > + FcDefaultSubstitute(pattern.get()); > + FcPatternRemove(pattern.get(), FC_FAMILY, 0); > + FcConfigSubstitute(nullptr, pattern.get(), FcMatchFont); > + > + return pattern; We can probably allocate this once and simply keep returning it from a static variable. > Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:186 > - : m_fallbacks(nullptr) > + : m_pattern(getDefaultFontconfigOptions()) > + , m_fallbacks(nullptr) Instead of making a new pattern per font, I think you should explicitly handle a null pattern in FontPlatformData::initializeWithFontFace. The way you have this now, it looks like it makes two separate instances with otherwise equal data, fail the equality test.
OK, a few further discoveries: * The GTK+ font settings (e.g. gtk-xft-hintstyle) are used to replace the default cairo font settings in getDefaultCairoFontOptions(), but most of them will be overridden immediately by fontconfig except for web fonts. With my patch the settings for web fonts will be overridden too, which will be more consistent. * Since GTK+ uses xsettings, your normal desktop font settings "pollute" the jhbuild environment, which is why Martin and I had different results. Martin has GTK+ set to slight hinting but mine is set to medium hinting. Slight hinting forces use of the autohinter, so it means the font's native hints aren't used at all. I think the native hints are broken in these fonts, because they look dramatically better when slight hinting is used. * The more I learn about our font configuration, the less I understand.... Now, I think we should actually go with a combination of Martin's patch and mine, and force autohinter use for all web fonts. For many fonts this will (subjectively) not look as good as medium or full hinting, and it will be inconsistent to ignore the system configuration for this one setting for web fonts, but I think most people won't notice and it has a huge advantage: we don't have to worry about problems with native hinting. My guess is those two fonts look terrible because their hinting is broken, and other browsers (except Firefox on Linux) don't use the native hinting, so webmasters don't notice. (This is just a guess.) But the autohinter should always produce a decent result, I think. And there is another advantage: by disabling native hints, FreeType will no longer be executing bytecode from potentially malicious fonts, which has caused security issues in the past. On the other hand, the Chromium folks are currently forcing the autohinter as well but want to switch to native hinting instead (that's https://code.google.com/p/chromium/issues/detail?id=173207).
Created attachment 247194 [details] Patch
Comment on attachment 247194 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=247194&action=review > Source/WebCore/ChangeLog:12 > + default settings from GTK+, which are normally overridden by fontconfig when loading system Nit, either use Fontconfig or fontconfig but not both. > Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:131 > + } I think that new code tends to use for this cases std::once_flag and std::call_once() but not sure that's a convention yet.
(In reply to comment #22) > Nit, either use Fontconfig or fontconfig but not both. Sure > I think that new code tends to use for this cases std::once_flag and > std::call_once() but not sure that's a convention yet. Hm, that would require a lambda or a new function, which I think would be harder to read. The problem is my code is not safe if called from multiple threads. I have no clue if that can happen, so might as well use it to be on the safe side, I suppose.
Comment on attachment 247194 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=247194&action=review Looks good to me, apart from a couple tiny nits. > Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:123 > + // Note that hint settings are overridden in FontCustomPlatformData::FontCustomPlatformData. You probably want to say something like, "Note that for web fonts, custom hinting is specifically disabled." The reason I'm suggesting this is that these settings might disable hinting completely, and FontCustomPlatformData::FontCustomPlatformData will not override that. > Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:316 > + if (m_pattern) > setCairoFontOptionsFromFontConfigPattern(options, m_pattern.get()); > + else > + setCairoFontOptionsFromFontConfigPattern(options, getDefaultFontconfigOptions()); > One thing you can do here to make this slightly simpler is: FcPattern* optionsPattern = m_pattern ? m_pattern.get() : getDefaultFontconfigOptions(); setCairoFontOptionsFromFontConfigPattern(options, optionsPattern);
Created attachment 247232 [details] Patch
Give it another quick lookover please, for the use of std::call_once
Comment on attachment 247232 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=247232&action=review Thank you! This looks good. > Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:128 > + static FcPattern* pattern = nullptr; > + static std::once_flag flag; > + std::call_once(flag, [](FcPattern*) { > + pattern = FcPatternCreate(); I have nothing to add here that won't make me sound like a hermit shaking my stick from the woods.
Comment on attachment 247232 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=247232&action=review > Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:135 > +} Yep, that's exactly what I was suggesting.
Comment on attachment 247232 [details] Patch Clearing flags on attachment: 247232 Committed r180563: <http://trac.webkit.org/changeset/180563>
All reviewed patches have been landed. Closing bug.
Comment on attachment 247232 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=247232&action=review > Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:307 > - cairo_font_options_t* options = getDefaultFontOptions(); > + cairo_font_options_t* options = getDefaultCairoFontOptions(); > + FcPattern* optionsPattern = m_pattern ? m_pattern.get() : getDefaultFontconfigOptions(); > + setCairoFontOptionsFromFontConfigPattern(options, optionsPattern); This properly checks for and works around the possibly-null m_pattern ... > Source/WebCore/platform/graphics/freetype/FontPlatformDataFreeType.cpp:324 > + for (int i = 0; FcPatternGetMatrix(m_pattern.get(), FC_MATRIX, i, &tempFontConfigMatrix) == FcResultMatch; i++) > + FcMatrixMultiply(&fontConfigMatrix, &fontConfigMatrix, tempFontConfigMatrix); ... but this doesn't anymore (though it used to), which leads to crashes. Should the FcPattern* from getDefaultFontconfigOptions() be used here as well, or should the code not execute at all when m_pattern is null?
It should be executed, and it should have crashed when I tested it. That was one of the last changes I made so my guess is I failed to test it at all.