Summary: | [GTK] Support the "system" CSS font family | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Adrian Perez <aperez> | ||||||
Component: | WebKitGTK | Assignee: | Adrian Perez <aperez> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | bugs-noreply, buildbot, cgarcia, commit-queue, darin, mark.lam, mcatanzaro, mmaxfield, sam, timothy | ||||||
Priority: | P2 | ||||||||
Version: | Other | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=178388 | ||||||||
Bug Depends on: | |||||||||
Bug Blocks: | 178624, 177814 | ||||||||
Attachments: |
|
Description
Adrian Perez
2017-10-02 10:43:32 PDT
(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” Created attachment 322423 [details]
Patch
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.
(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. 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(). 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 (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 :-) Created attachment 322510 [details]
Patch
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: graouts@apple.com, mcatanzaro@igalia.com, and ryanhaddad@apple.com) The commit-queue is continuing to process your patch. Comment on attachment 322510 [details] Patch Clearing flags on attachment: 322510 Committed r222800: <http://trac.webkit.org/changeset/222800> All reviewed patches have been landed. Closing bug. *** Bug 174027 has been marked as a duplicate of this bug. *** 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. 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. (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) (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? |