Bug 178624

Created attachment 324503 [details] Patch

Comment on attachment 324503 [details] Patch The layout test is not passing, I'll check why is that.

After adding some debug-prints, it turns out that instead of “system-ui”, the string “system-uie” is being received, which of course is wrong: operator(): family 'system-uie' fontRangesForFamily: family 'system-uie' resolveGenericFamily: family 'system-uie' getFamilyNameStringFromFamily: 'system-uie' The first two log lines are already in the WebCore CSS code, so it looks like I'll have to go deeper in there to find out what is going on here with this fishy value for the font family name.

Created attachment 452730 [details] Patch Working patch, with added handling of a few more cases.

Comment on attachment 452730 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=452730&action=review > Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:335 > + // TODO: Add support for ui-serif, ui-monospace, and ui-rounded. I would remove this TODO, because I don't think we can actually do it. > Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:341 > + || equalLettersIgnoringASCIICase(family, "ui-sans-serif")) I would remove this line. There is no guarantee that the default GTK font will be Cantarell or sans serif. We should stick to implementing the font names that really make sense with our platform. E.g. WPE has no concept of UI fonts distinct from Fontconfig's defaults, so we simply don't implement system fonts there. GNOME does have a UI monospace font (it's currently Source Code Pro) distinct from the Fontconfig defaults, but not sans or serif, while GTK has no such distinctions at all. Since there are no corresponding UI fonts, implementing them here doesn't make a ton of sense. We *could* choose to map these names to Fontconfig defaults, but then there would be no difference between e.g. "ui-sans-serif" vs. normal "sans," so I do not see much point in doing so.

(In reply to Michael Catanzaro from comment #5) > Comment on attachment 452730 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=452730&action=review > > > Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:335 > > + // TODO: Add support for ui-serif, ui-monospace, and ui-rounded. > > I would remove this TODO, because I don't think we can actually do it. For ui-monospace we could try to get the monospace font set system wide for GNOME, if possible. I have left that out of this patch because it would need some changes in GtkSettingsManager so I would rather do that in a separate patch. > > Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:341 > > + || equalLettersIgnoringASCIICase(family, "ui-sans-serif")) > > I would remove this line. There is no guarantee that the default GTK font > will be Cantarell or sans serif. We should stick to implementing the font > names that really make sense with our platform. E.g. WPE has no concept of > UI fonts distinct from Fontconfig's defaults, so we simply don't implement > system fonts there. GNOME does have a UI monospace font (it's currently > Source Code Pro) distinct from the Fontconfig defaults, but not sans or > serif, while GTK has no such distinctions at all. Since there are no > corresponding UI fonts, implementing them here doesn't make a ton of sense. Yes, I was myself doubting whether to include ui-sans-serif here or not, and in the end went for it while reworking the patch a couple of days ago as the default is typically some sans serif one. Re-reading the CSS spec today (and being a bit more rested) it looks like it's probably better. Quoting the spec: “Note: ui-serif is not expected to map to any font on platforms without an appropriate system font.” (And the same note is attached to all the ui-* fonts.) > We *could* choose to map these names to Fontconfig defaults, but then there > would be no difference between e.g. "ui-sans-serif" vs. normal "sans," so I > do not see much point in doing so. At least one would get a sans font when the site designer intended to have some sans font, and so on, but given the note in the spec about better not mapping to any font when a suitable one is not available in the platform, the responsibility for planning a fallback is in the hands of the people writing the CSS (i.e. “font-family: ui-sans-serif, sans-serif;”). I'll reupload with ui-sans-serif removed from the list of matches.

Created attachment 452737 [details] Patch Working patch, with match on ui-sans-serif removed.

Created attachment 452738 [details] Patch Now it also builds, after adding the missing closing paren 8-)

Comment on attachment 452738 [details] Patch OK, but note this breaks some tests. You'll need to investigate before landing.

(In reply to Michael Catanzaro from comment #9) > Comment on attachment 452738 [details] > Patch > > OK, but note this breaks some tests. You'll need to investigate before > landing. I think most (if not all) the tests with pixel differences need a rebaseline: before this patch they would pick a serif font (which I think was the last resort fallback) before getting to pick a sans serif, but I am not 100% sure. I'll have to take a look and understand well if my changes are (partially?) to blame, I suspect doing “return family;” instead of “return "";” as last thing in the modified function could be the reason for some of the failures. But I am not sure if returning an empty string there is the best we can do, because I think that's what causes the serif font to be picked. I'll take a look at what Cocoa does, to have a “second opinion“ on this matter, too.