Bug 15378

Summary: WebKit GTK+ should use Font info from GtkStyle, and aliases, by default
Product: WebKit Reporter: Rodney Dawes <dobey>
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED WONTFIX    
Severity: Normal CC: christian, mrobinson, mrowe, zl29ah
Priority: P2 Keywords: Gtk
Version: 523.x (Safari 3)   
Hardware: All   
OS: Linux   
Attachments:
Description Flags
Patch to update default font settings in GTK+
none
Updated patch to match style guidelines better
none
Updated with inline declarations
aroben: review-
Updated patch to resolve the issues in Adam's review mrowe: review-

Description Rodney Dawes 2007-10-05 05:23:14 PDT
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.
Comment 1 Rodney Dawes 2007-10-05 05:24:16 PDT
Created attachment 16543 [details]
Patch to update default font settings in GTK+
Comment 2 Rodney Dawes 2007-10-05 05:57:58 PDT
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.
Comment 3 Rodney Dawes 2007-10-05 08:19:49 PDT
Created attachment 16545 [details]
Updated with inline declarations

Changed the defsize and family variables to be declared inline, as suggested by bdash on irc.
Comment 4 Adam Roben (:aroben) 2007-10-06 23:13:31 PDT
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.
Comment 5 Rodney Dawes 2007-10-10 10:52:16 PDT
(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.

Comment 6 Rodney Dawes 2007-10-10 10:53:07 PDT
Created attachment 16615 [details]
Updated patch to resolve the issues in Adam's review
Comment 7 Adam Roben (:aroben) 2007-10-10 11:26:10 PDT
Comment on attachment 16615 [details]
Updated patch to resolve the issues in Adam's review

r=me
Comment 8 Mark Rowe (bdash) 2007-10-14 04:32:34 PDT
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.
Comment 9 Holger Freyther 2007-10-14 13:51:58 PDT
(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.
Comment 10 Mark Rowe (bdash) 2007-10-14 14:40:25 PDT
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.
Comment 11 Christian Dywan 2007-10-14 14:46:28 PDT
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.
Comment 12 Alp Toker 2007-10-22 01:34:38 PDT
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.
Comment 13 Martin Robinson 2010-08-25 09:17:02 PDT
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!