Bug 177755 – [GTK] Support the "system" CSS font family

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Adrian Perez]

Some info about this feature:

  https://webkit.org/blog/3709/using-the-system-font-in-web-content/
  https://jonathantneal.github.io/system-font-css/
  https://lists.w3.org/Archives/Public/www-style/2015Jul/0169.html

The TL;DR is that using “font-family: -webkit-system” (I think WebKit only
has a prefixed implementation) should select the system UI font. In GTK+
we would obtain this from the “GtkSettings::gtk-font-name” property.

[Adrian Perez]

(In reply to Adrian Perez from comment #0)
> Some info about this feature:
> 
>   https://webkit.org/blog/3709/using-the-system-font-in-web-content/
>   https://jonathantneal.github.io/system-font-css/
>   https://lists.w3.org/Archives/Public/www-style/2015Jul/0169.html
> 
> The TL;DR is that using “font-family: -webkit-system” (I think WebKit only
> has a prefixed implementation) should select the system UI font. In GTK+
> we would obtain this from the “GtkSettings::gtk-font-name” property.

Correction: it's “font-family: -webkit-system-font”

[Adrian Perez]

Created attachment 322423 [details]
Patch

[Build Bot]

Attachment 322423 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:334:  Line contains only semicolon. If this should be an empty statement, use { } instead.  [whitespace/semicolon] [5]
Total errors found: 1 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adrian Perez]

(In reply to Build Bot from comment #3)
 
> ERROR: Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:334: 
> Line contains only semicolon. If this should be an empty statement, use { }
> instead.  [whitespace/semicolon] [5]

This style checker failure is expected. I cannot think of a nice way of
adding the conditional compilation here without having the hanging semicolon
and without adding the elements somewhere in the middle. If that's okay with
reviewers, I suggest to land the patch despite this.

[Carlos Garcia Campos]

Comment on attachment 322423 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=322423&action=review

> Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:333
> +#if PLATFORM(GTK)
> +        || equalLettersIgnoringASCIICase(familyNameString, "-webkit-system-font")
> +        || equalLettersIgnoringASCIICase(familyNameString, "-webkit-system-ui")
> +#endif

You can mode the ifdefed code to the beginning, since the order doesn't matter, right?

> Source/WebCore/platform/graphics/gtk/GdkCairoUtilities.cpp:61
> +String getDefaultGtkSystemUiFont()

I would call this just defaultGtkSystemFont().

> Source/WebCore/platform/graphics/gtk/GdkCairoUtilities.cpp:67
> +    auto spaceChar = strrchr(fontString.get(), ' ');

auto*

> Source/WebCore/platform/graphics/gtk/GdkCairoUtilities.cpp:68
> +    return String::fromUTF8(fontString.get(), spaceChar - fontString.get());

I know we are always expecting a valid font name - space - size string, but I would either assert if that's not the case or handling that case, I don't know what happens if the user sets an an empty string in the dconf setting, for example. Since we have a duplicated string, maybe it's easier to set '\0' in the place of the last space (if found), and then just use String::fromUTF8().

[Michael Catanzaro]

Comment on attachment 322423 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=322423&action=review

I agree with the r+, but I have more comments to address before landing.

This should go in GdkUtilities.[h,cpp], not GdkCairoUtilities. It has nothing to do with Cairo.

Both the remote inspector and Epiphany should be updated to use -webkit-system-font instead of that stupid confusing menu font.

It should be implemented for WPE as well, just hardcode the result as "sans".

You get bonus points for the layout test, thank you!

> Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:168
> +#endif

Implement it for WPE as well:

#if PLATFORM(GTK)
    return defaultGtkSystemFont();
#else
    return "sans-serif";
#endif

>> Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:333
>> +#endif
> 
> You can mode the ifdefed code to the beginning, since the order doesn't matter, right?

Just remove the ifdefs, this can be implemented on all platforms. The non-GTK way to get the default system font would be "fc-match sans" and we can accomplish that by just returning "sans-serif".

>> Source/WebCore/platform/graphics/gtk/GdkCairoUtilities.cpp:61
>> +String getDefaultGtkSystemUiFont()
> 
> I would call this just defaultGtkSystemFont().

And in any case, we do capitalize two-letter acronyms in WebKit, so it should have been getDefaultGtkSystemUIFont

[Adrian Perez]

(In reply to Michael Catanzaro from comment #6)
> Comment on attachment 322423 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=322423&action=review
> 
> I agree with the r+, but I have more comments to address before landing.
> 
> This should go in GdkUtilities.[h,cpp], not GdkCairoUtilities. It has
> nothing to do with Cairo.

Probably you mean using GtkUtilities.{h,cpp} (there is no GdkUtilities.{h,cpp}).

The code was added in GdkCairoUtilities.{h,cpp} because there was already
font-related code there. I'll move it.


> It should be implemented for WPE as well, just hardcode the result as "sans".

I'm not sure about this, check my reasoning below.
 
> >> Source/WebCore/platform/graphics/freetype/FontCacheFreeType.cpp:333
> >> +#endif
> > 
> > You can mode the ifdefed code to the beginning, since the order doesn't matter, right?

Yes, it just does not look beautiful because then there are a few lines
with checks for non-prefixed items, then the two prefixed items in the
middle, then more non-prefixed ones... I'll just move it to the middle,
but aesthetically it looks a bit “meh”.
 
> Just remove the ifdefs, this can be implemented on all platforms. The
> non-GTK way to get the default system font would be "fc-match sans" and we
> can accomplish that by just returning "sans-serif".

Thing is... “sans-serif” does not necessarily return the “system font”
for WPE. Actually, there is no concept of “system font” for WPE because
there is no toolkit or no environment always known to be available and
that has a system font setting. Therefore it seems more correct to me
to *not* implement this for WPE. Authors can still fallback to picking
“sans-serif” iff that is their intention:

  font-family: -webkit-system-font, sans-serif;

(Which they should be ding anyway, if they want to support web engines
other than WebKit!)

> >> Source/WebCore/platform/graphics/gtk/GdkCairoUtilities.cpp:61
> >> +String getDefaultGtkSystemUiFont()
> > 
> > I would call this just defaultGtkSystemFont().
> 
> And in any case, we do capitalize two-letter acronyms in WebKit, so it
> should have been getDefaultGtkSystemUIFont

I'll update the patch to use “defaulGtkSystemFont()”. Simple is good :-)

[Adrian Perez]

Created attachment 322510 [details]
Patch

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 322510 [details]:

media/modern-media-controls/seek-backward-support/seek-backward-support.html bug 174916 (authors: [e-mail], [e-mail], and [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 322510 [details]
Patch

Clearing flags on attachment: 322510

Committed r222800: <http://trac.webkit.org/changeset/222800>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Adrian Perez]

*** Bug 174027 has been marked as a duplicate of this bug. ***

[Myles C. Maxfield]

I don't know how GTK does it, but on macOS and iOS, the system font actually represents a whole fallback list (so it appears to handle every language). We have support for this in WebKit. If GTK's system font works the same way, you can use the same infrastructure.

[Michael Catanzaro]

No, for WebKitGTK+ the system font is one font family stored by GtkSettings. For WPE, it's just "sans" since that's an alias that will be resolved to something appropriate by Fontconfig.

[Adrian Perez]

(In reply to Michael Catanzaro from comment #14)
> No, for WebKitGTK+ the system font is one font family stored by GtkSettings.
> For WPE, it's just "sans" since that's an alias that will be resolved to
> something appropriate by Fontconfig.

Sorry, but that is not correct. This patch has *NOT* added support for WPE
because there is no “system” environment (and hence no toolkit, no settings,
no nothing), and therefore it's not reasonable to provide a default.

What I am thinking may make sense is making the “system-ui” generic family
configurable via the public API, in the same way that the default sans-serif,
cursive, etc. generic fonts can be — I am working on a patch which does that
but still needs some love (*very* WIP version at http://sprunge.us/IfGI which
I think will be for bug #178624 once I have some time to finish it)

[Myles C. Maxfield]

(In reply to Adrian Perez from comment #15)
> (In reply to Michael Catanzaro from comment #14)
> > No, for WebKitGTK+ the system font is one font family stored by GtkSettings.
> > For WPE, it's just "sans" since that's an alias that will be resolved to
> > something appropriate by Fontconfig.
> 
> Sorry, but that is not correct. This patch has *NOT* added support for WPE
> because there is no “system” environment (and hence no toolkit, no settings,
> no nothing), and therefore it's not reasonable to provide a default.
> 
> What I am thinking may make sense is making the “system-ui” generic family
> configurable via the public API, in the same way that the default sans-serif,
> cursive, etc. generic fonts can be — I am working on a patch which does that
> but still needs some love (*very* WIP version at http://sprunge.us/IfGI which
> I think will be for bug #178624 once I have some time to finish it)

Maybe this is a stupid question, but if there’s “no nothing,” how does system integration work? Things like form controls?