Bug 178624 – [GTK] Support CSS4 generic "system-ui" font family

• Status: ASSIGNED
• Component: WebKitGTK

[Adrian Perez]

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

[Adrian Perez]

Created attachment 324503 [details]
Patch

[Adrian Perez]

Comment on attachment 324503 [details]
Patch

The layout test is not passing, I'll check why is that.

[Adrian Perez]

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]

Created attachment 452730 [details]
Patch

Working patch, with added handling of a few more cases.

[Michael Catanzaro]

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]

(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]

Created attachment 452737 [details]
Patch

Working patch, with match on ui-sans-serif removed.

[Adrian Perez]

Created attachment 452738 [details]
Patch

Now it also builds, after adding the missing closing paren 8-)

[Michael Catanzaro]

Comment on attachment 452738 [details]
Patch

OK, but note this breaks some tests. You'll need to investigate before landing.

[Adrian Perez]

(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.