Bug 178624

Summary: [GTK] Support CSS4 generic "system-ui" font family
Product: WebKit Reporter: Adrian Perez <aperez>
Component: WebKitGTKAssignee: Adrian Perez <aperez>
Status: ASSIGNED    
Severity: Normal CC: bugs-noreply, buildbot, calvin.walton, cgarcia, clopez, dvpdiner2, mcatanzaro, mmaxfield
Priority: P2    
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=221445
Bug Depends on: 177755    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
ews-feeder: commit-queue-
Patch mcatanzaro: review+, ews-feeder: commit-queue-

Adrian Perez
Reported 2017-10-21 10:27:57 PDT
This was implemented for “-webkit-system-font” and “-webkit-system-ui” in bug #177755 but support for the (unprefixed) “system-ui” font family was left out. We should support this as well because it's specified as part of CSS4, see https://www.w3.org/TR/css-fonts-4/#system-ui-def
Attachments
Patch (4.67 KB, patch)
2017-10-21 12:51 PDT, Adrian Perez
no flags
Patch (7.45 KB, patch)
2022-02-21 07:43 PST, Adrian Perez
no flags
Patch (7.11 KB, patch)
2022-02-21 08:18 PST, Adrian Perez
ews-feeder: commit-queue-
Patch (7.11 KB, patch)
2022-02-21 08:36 PST, Adrian Perez
mcatanzaro: review+
ews-feeder: commit-queue-
Adrian Perez
Comment 1 2017-10-21 12:51:17 PDT
Adrian Perez
Comment 2 2017-10-21 14:59:49 PDT
Comment on attachment 324503 [details] Patch The layout test is not passing, I'll check why is that.
Adrian Perez
Comment 3 2017-10-22 15:16:10 PDT
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.
Adrian Perez
Comment 4 2022-02-21 07:43:22 PST
Created attachment 452730 [details] Patch Working patch, with added handling of a few more cases.
Michael Catanzaro
Comment 5 2022-02-21 07:54:45 PST
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.
Adrian Perez
Comment 6 2022-02-21 08:15:29 PST
(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.
Adrian Perez
Comment 7 2022-02-21 08:18:43 PST
Created attachment 452737 [details] Patch Working patch, with match on ui-sans-serif removed.
Adrian Perez
Comment 8 2022-02-21 08:36:03 PST
Created attachment 452738 [details] Patch Now it also builds, after adding the missing closing paren 8-)
Michael Catanzaro
Comment 9 2022-02-21 14:34:15 PST
Comment on attachment 452738 [details] Patch OK, but note this breaks some tests. You'll need to investigate before landing.
Adrian Perez
Comment 10 2022-02-24 09:24:10 PST
(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.
Note You need to log in before you can comment on or make changes to this bug.