Bug 15378 – WebKit GTK+ should use Font info from GtkStyle, and aliases, by default

• Component: WebCore Misc.
• Status: RESOLVED WONTFIX

[Rodney Dawes]

WebKit currently has some odd defaults in the GTK+ port for font names and sizes. This patch changes the default font setting code to use the user's preferred application font which is accessible through the GtkStyle object. It also changes the default minimum size to 7, as 5 is a bit insanely small (coming from someone who uses insanely small fonts by default), and changes the default names to use the Sans/Monospace/Serif aliases, rather than the MS Core Font names. This let's the best suitable font be chosen by default from fontconfig, for monospace/serif/sans fonts, with the standard font being set to the user's theme font. This also allows a user to theme the WebKit widget's font specifically, through the gtkrc.

[Rodney Dawes]

Created attachment 16543 [details]
Patch to update default font settings in GTK+

[Rodney Dawes]

Created attachment 16544 [details]
Updated patch to match style guidelines better

Here's an updated version of the patch to match the style guidelines better, as mentioned by bdash on irc.

[Rodney Dawes]

Created attachment 16545 [details]
Updated with inline declarations

Changed the defsize and family variables to be declared inline, as suggested by bdash on irc.

[Adam Roben (:aroben)]

Comment on attachment 16545 [details]
Updated with inline declarations

+    int defsize = pango_font_description_get_size(GTK_WIDGET(page)->style->font_desc);

Does GtkStyle store a separate default font size for fixed vs. variable width fonts?

"defsize" should be renamed "defaultSize" to adhere to our coding style guidelines <http://webkit.org/coding/coding-style.html>.

+        defsize /= 1024;

Can we get this constant from a Pango header? If not, this constant should be a static const int at the top of the file.

You'll need to add a ChangeLog entry before this can be landed. See <http://webkit.org/coding/contributing.html> for instructions.

[Rodney Dawes]

(In reply to comment #4)
> Does GtkStyle store a separate default font size for fixed vs. variable width
> fonts?

It only provides one PangoFontDescription, so unfortunately no, it doesn't provide preferred monospace font information. I would certainly prefer to access the font settings from GNOME, but these are only available in gconf.

> "defsize" should be renamed "defaultSize" to adhere to our coding style
> guidelines <http://webkit.org/coding/coding-style.html>.
> 
> +        defsize /= 1024;
> 
> Can we get this constant from a Pango header? If not, this constant should be a
> static const int at the top of the file.

This constant does appear to be defined in pango-types.h. I'm about to upload a patch which fixes this and the variable name.

> You'll need to add a ChangeLog entry before this can be landed. See
> <http://webkit.org/coding/contributing.html> for instructions.

Also in updated patch that I'm about to attach.

[Rodney Dawes]

Created attachment 16615 [details]
Updated patch to resolve the issues in Adam's review

[Adam Roben (:aroben)]

Comment on attachment 16615 [details]
Updated patch to resolve the issues in Adam's review

r=me

[Mark Rowe (bdash)]

I'm still not convinced this patch is a good thing.  Other platforms all default to the hard-coded values, but allow the user to explicitly override them.  WebKit on Windows, for instance, does not default to the OSs default UI font.  It defaults to the standard web fonts.

[Holger Freyther]

(In reply to comment #8)
> I'm still not convinced this patch is a good thing.  Other platforms all
> default to the hard-coded values, but allow the user to explicitly override
> them.  WebKit on Windows, for instance, does not default to the OSs default UI
> font.  It defaults to the standard web fonts.

I'm not convinced as well:
   - You should not change the font names. Fontconfig is supposed to take care of that
   - Changing the default sizes is probably not good as well
   - You do not listen to the style change signal. If you would do you would in some situations overwrite users changes to the Settings. So if you still want to work on it feel free to implement webkitsettings (using properties) and show how to react on changing the style using GtkLauncher.

[Mark Rowe (bdash)]

Comment on attachment 16615 [details]
Updated patch to resolve the issues in Adam's review

I'm going to mark this r- based on Holger's comments.

[Christian Dywan]

In my opinion it is preferrable to use the user's font and the aliases respectively, as this is what the user expects with other widgets/ programs and trying to use fonts which are not installed feels a bit ugly in the first place.

The widget must definitely react on style changes unless the (unimplemented) font property has been changed by client code.

As for the implementation, this might not be the right way, but I'd like to see this done eventually.

[Alp Toker]

I'm not too comfortable with this just yet -- it might make sense to hold back here, but to keep the bug open. #15608 should probably be implemented first before we worry about this one, anyway.

[Martin Robinson]

I think we want to shoot for compatibility with the web here. This patch is also many years out of date. We can address this issue in a fresh bug if it comes up. Thanks for the patch!