GNOME Bugzilla – Bug 713746
use document font in message viewer and composer
Last modified: 2014-07-09 00:53:24 UTC
---- Reported by adam@yorba.org 2011-12-16 16:56:00 -0800 ---- Original Redmine bug id: 4508 Original URL: http://redmine.yorba.org/issues/4508 Searchable id: yorba-bug-4508 Original author: Adam Dingle Original description: Not sure how controversial this will be, but I think we should increase some of Geary's font sizes slightly. We want Geary to be really easy on the eyes, and small fonts are hard to read. In particular, I think we should increase the following fonts by a point or so: * The font for displaying message previews in the message list. This one is really tiny. If changing this means that less preview text will be available per message, so be it. * The font for displaying the From, Subject and Date at the top of the message area. * The font for displaying message bodies. Thunderbird, for example, uses a slightly larger font and I find messages easier to read there. Related issues: related to geary - 5727: Update conversation list interface (Fixed) related to geary - Feature #7136: Conversation list doesn't honor OS font size (Open) duplicated by geary - 5512: Message content font is extremely large (Duplicate) duplicated by geary - 6911: Geary mail headers "tiny" (Duplicate) ---- Additional Comments From geary-maint@gnome.bugs 2013-09-23 14:19:00 -0700 ---- ### History #### #1 Updated by Adam Dingle over 1 year ago * **Target version** deleted (<strike>_0.1_</strike>) #### #2 Updated by Adam Dingle over 1 year ago * **Target version** set to _0.2_ #### #3 Updated by Jim Nelson over 1 year ago * **Subject** changed from _increase font sizes_ to _Increase message list, viewer font sizes_ * **Category** set to _client_ #### #4 Updated by Daniel Fore over 1 year ago Hey Adam, I'd recommend using the OS's "Document Font" property. This is typically larger than the regular ui font and may use a different typeface better suited for reading chunks of text (For example, in elementary this is 10pt Open Sans instead of 9pt Droid Sans). This also means it's a system-wide configurable option. #### #5 Updated by Adam Dingle about 1 year ago * **Assignee** set to _Daniel Fore_ #### #6 Updated by Adam Dingle about 1 year ago We've decided to use the document font for both the message viewer and the composer. #### #7 Updated by Adam Dingle about 1 year ago * **Assignee** deleted (<strike>_Daniel Fore_</strike>) #### #8 Updated by Adam Dingle about 1 year ago * **Subject** changed from _Increase message list, viewer font sizes_ to _use document font in message viewer and composer_ #### #9 Updated by Adam Dingle about 1 year ago * **Target version** deleted (<strike>_0.2_</strike>) #### #10 Updated by Jim Nelson 10 months ago * **Tracker** changed from _Bug_ to _Bite-sized_ * **Target version** set to _0.3.0_ #### #11 Updated by Jonathan Lumb 10 months ago * **File** 05.png added * **File** 18.png added I have a similar issue which I believe is related to this bug - please correct me if I'm wrong and I can open a separate bug: When I receive emails containing simplified Chinese characters in Geary, they are displayed in an ugly Chinese font. I have configured /etc/fonts/conf.avail/69-language-selector-zh-cn.conf so that programs in elementary OS fall back to an elegant font (that supports Chinese) when Chinese text is encountered. This means beautiful character display in most programs on the OS (text editors, file managers, terminal etc.) but unfortunately it seems that Geary is not drawing from these system-wide settings and so doesn't really integrate into the OS all that well. Ideally Geary should respect the system-wide font settings - maybe by using the "Document Font" property that Daniel has mentioned above? Likewise, it is desirable that Geary respects the font fallback settings in /etc/fonts/conf.avail/ so that non-Latin text is displayed more elegantly. If the above could be implemented then I am sure it would improve the UI experience for many people who use Asian or other non-Latin fonts. Apologies in advance if I have misunderstood how all this font business works! I have attached a couple of screenshots so you can see what I mean. #### #12 Updated by Jim Nelson 10 months ago I think this ticket encapsulates what you need to fix your problem, so no need for a new ticket. Hopefully we'll get this fixed in 0.3! #### #13 Updated by Jim Nelson 8 months ago * **Target version** changed from _0.3.0_ to _0.4.0_ #### #14 Updated by Timo Kluck 8 months ago * **File** 0001-Use-the-system-s-document-font-for-e-mail-bodies.patch added Here's a patch! I'm not entirely happy with it yet, for the following reasons: * IIRC, there is no graceful way to fail when a certain setting (org.gnome.desktop.interface) does not exist on the system. The GSettings library will just abort(). Maybe best practise is to ship it in our own schema too? Does that work at all? * Font rendering in webkit looks different from font rendering on the system. The hinting settings seem to be different. This was not noticeable before, because then the webkit part and the rest of the interface had different fonts anyway. I don't think there's much we can do about that. Many people will have a document font different from their UI font, anyway, so it may not be a very big issue. #### #15 Updated by Timo Kluck 8 months ago * **File** 0002-make-sure-changes-to-document-font-name-get-propagat.patch added And an extra patch to properly handle listen for settings changes. #### #16 Updated by Timo Kluck 8 months ago For reference, this is what Epiphany does: https://git.gnome.org/browse/epiphany/tree/embed/ephy-embed-prefs.c It just sets those desktop fonts as the default serif and sans-serif fonts for the webview. That's another way of handling this. It will still suffer from the same issues, though. #### #17 Updated by Jim Nelson 8 months ago * **Status** changed from _Open_ to _Review_ * **Assignee** set to _Timo Kluck_ Thanks for the patch! We'll take a look at this for 0.4. #### #18 Updated by Eric Gregory 8 months ago * **Status** changed from _Review_ to _Open_ Timo, A few issues with this patch: * For consistency, we like to keep all the settings in geary-config.vala. * Unnecessary variables are discouraged; for example, two of the methods in your patch have a "document" variable that could easily be removed. * The mystery constant of 96/72 is something we need to avoid. Can you investigate why this is necessary? * To simplify on_system_style_changed(), a little, you can eliminate the n variable and move system_style down so that it's declared and printf'd in one statement. Thanks again -- looking forward to the next iteration! #### #19 Updated by Jim Nelson about 1 month ago * **Assignee** deleted (<strike>_Timo Kluck_</strike>) * **Target version** changed from _0.4.0_ to _0.5.0_ --- Bug imported by chaz@yorba.org 2013-11-21 20:22 UTC --- This bug was previously known as _bug_ 4508 at http://redmine.yorba.org/show_bug.cgi?id=4508 Imported an attachment (id=260776) Imported an attachment (id=260777) Imported an attachment (id=260778) Imported an attachment (id=260779) Unknown milestone "unknown in product geary. Setting to default milestone for this product, "---". Setting qa contact to the default for this product. This bug either had no qa contact or an invalid one. Resolution set on an open status. Dropping resolution
Created attachment 266678 [details] [review] Updated version of this work Updated version; much cleaner way of accomplishing the same result, based on comparison with how Epiphany does this.
Created attachment 266679 [details] [review] Updated version of this work This version actually compiles; I had forgot to test that after pulling some magic constants out of the code. Sorry about that :) Hope this version is simple enough that we can merge it. Sorry for taking so long; geary has not been on my radar for a while.
Timo, this looks a lot better, I'm glad you stuck with this one. Code-wise this looks fine and takes care of the issues I described before. In fact, I'm surprised how clean this looks; bind_properties() is indeed a magical method. However, two issues remain: * It looks like this only affects the conversation viewer. We also want to respect the user's document font (for editing) in the composer. Is that much more work? Note that the composer uses a monospaced-font when not in Rich Text mode. * With this patch, the fonts in the conversation viewer really jump in size. My configured default is Sans 11 (org.gnome.desktop.interface.document-font). I have to set it to Sans 9 for a display size I'm comfortable with. So, first, is it possible the math is incorrect? When I look at Sans 11 in Character Map, the glyphs are smaller than Geary's, and look more like the size I would prefer for reading text. It's also possible that we added CSS in the past to muck with hard-coded values to make them "look" right. Are you seeing something similar on your machine? Can you investigate? I've created a branch at wip/713746-document-font if anyone else wants to try it out and comment. (Robert, can you take a look?) Timo, please make future patches against this branch. Thanks!
Jim, have you properly pushed to the wip branch? For me, it's showing up as just one commit behind master.
Ah, I forgot to commit and push. It's up now.
This change makes the text too big for me as well. I think what's happening is that the point->pixel transformation is being done twice, once by us and once by WebKit. My document font is Sans 11. My monitor dpi is (correctly reported as) 96. This new code works out a font size of 14 (11*4/3, rounded down), and passes that to webkit. Webkit then decides to use a font size of 18px. Testing other font sizes, I see that webkit is scaling by a factor of 4/3 from pt to px, so I assume it interpreted the setting we pass it in points, and got 18 by scaling to px. Complicating matters, we set "font-size: small" on the email. This scales the 18px default down to 16px to display the emails. But what we really wanted was 14px. (Before, I think "font-size: small" got us 13px/10pt.) So the attached patch switches the value we pass to webkit from px to pt and sets "font-size: medium" for .email. But before we go ahead with this, I think we should clear up a few other points with font sizing in the webview: 1) There's a "font-size: 10pt !important" on the body element. Does anyone know why? 2) Some font sizes are set absolutely, others relatively. If we're trying to respect the user's settings here, we should probably switch to relative sizing everywhere. While we're at it, we should also pick and document standard sizes for the various UI elements. I know that when I created some in the past, I just picked sizes that looked good, rather than going by any standard. Note that this may mess up places where we're trying to do pixel-perfect alignment. I think we're doing that to get the identicons lined up correctly, and we're also counting on the header being a fixed size in pixels when figuring out when to mark emails as read. This will become much more difficult in a relatively-sized world. 3) The composer font size has been larger, since we didn't set the small font size on it. I don't think there's any good reason for this, so lets set its font size the same way.
Created attachment 266930 [details] [review] To be applied on wip branch
This is in the ballpark, thanks Robert for looking into this. My conversation viewer font still "feels" a tad too large, but I suspect it's because I've been looking at the old size for almost three years now. It's certainly something I could get used to. > 1) There's a "font-size: 10pt !important" on the body element. Does anyone > know why? No idea. It appears to have been added in commit 5077bb9 (2012-02-15). I can't tell from the comment or the code changes why. If it was to fix a functional bug, I like to believe that would've been specified in the commit comment or ref'd ticket. > 2) Some font sizes are set absolutely, others relatively. If we're trying to > respect the user's settings here, we should probably switch to relative sizing > everywhere. While we're at it, we should also pick and document standard sizes > for the various UI elements. I agree with this for the most part (especially the documentation part, which I wish I'd been more hard-nosed about). I don't think we should be robotic about absolute -> relative. If some font sizes make life truly easier where we're trying to be pixel-perfect (I think the message header is the only place this would happen), then I say it's okay to make that call and stick with absolute sizes. To put it another way, my current concern is to respect the user's font sizes in the document portions of the interface: the message body and the composer editing widget. If there's a call to respect font sizes in the message header (or footer, I suppose, where the attachments are listed), then we can separate that out as a separate ticket. (Assuming it can be separated out and not intertwined with the document portions of the CSS.) > 3) The composer font size has been larger, since we didn't set the small font > size on it. I don't think there's any good reason for this, so lets set its > font size the same way. That may've been intentional. I recall Adam wanting the editing font to be larger than the reading font. I don't think that's a hard requirement, however, and if the editing font was the same size as the viewing font in your current patch, I wouldn't object. If GNOME has some setting for editing vs. viewing font sizes, then I say we respect those. I've looked and can't find anything like that, however, just the document and monospace fonts. I've committed and pushed your patch (honest, this time).
This may be another bug, but perhaps we should be using the font used elsewhere in the UI for the UI parts of the conversation view.
That's an interesting point. If the user's document font is serifed, then the conversation viewer probably looks goofy. I'd rather attack that separately than on this ticket though: bug #723205
Created attachment 276912 [details] [review] Patch based on alternate method Here's a patch that sets the default fonts through CSS, rather than through the WebSettings object. This allows us to set both the document and interface fonts, and it give WebKit the responsibility for doing the points to pixels conversion. Since a CSS pixel may not be a device pixel on high-res screens, this is probably a better way to handle things. I do notice a slight difference in sizes in some cases. If I request an 11 pt font, WebKit works out that it should be 15 px (11 * 4/3, rounded up). But GTK widgets use a font that looks closer to 14 px (11* 4/3, rounded down).
Created attachment 276913 [details] [review] Patch for the composer Strictly speaking, this doesn't belong here, but I don't think we have a separate bug for it yet. (And it ends up rewriting a bit of the previous patch, on which it depends.) This uses the system document font and the monospace font for the composer in HTML and plain text modes, respectively. The method is the same as with my preceding patch, although here we could use the WebSettings object.
Created attachment 277030 [details] [review] Patch setting default fonts After some more thought, I think Timo's original approach is more appropriate. If we set the default and monospace fonts from those in the system settings, they'll be used as appropriate everywhere. We can also get the system font via "font: caption". (Other options that give the same result for me: icon, menu, message-box, small-caption, status-bar, -webkit-mini-control, -webkit-small-control, -webkit-control. I don't know if one of these others is more appropriate.) I make a new class that inherits from WebView, impishly dubbed StylishWebView (we can change that), that sets the default and monospace fonts automatically. Then, we just have to set the CSS to call up the appropriate fonts with appropriate sizes. Unfortunately, this doesn't quite work. In the conversation view, I set the body font to be caption, since most elements are UI. I should be able to get the document font for the email body with "font-family: initial". This works in Firefox and in Chrome, but not in whatever version of WebKit I'm using. The next patch contains a fix for this issue.
Created attachment 277031 [details] [review] Patch fixing fonts in conversation view This should be applied on top of the previous patch to fix the bug discussed therein. It explicitly sets the font of the email body with CSS, and consequently this CSS must be updated each time the font is changed. It's not a particularly pretty solution, and it has me thinking again about displaying the email bodies in iframes. This would reset the fonts to the document font for us. It would also prevent CSS leakage out of messages and keep positioned elements from escaping the container. Potentially, it would let us to a lot of the UI stuff in Javascript while sandboxing the iframes, which might help us when we move to webkitgtk2. The downsides are more complications in assembling emails and the need to resize the iframes manually to get their content to fit. I'm not sure it's worth it.
These patches certainly do look right (visually) and seem right code-wise. Are you comfortable landing this, or do you want to keep hacking on this, potentially going down the iframe route?
I am confident enough that the current code is correct. I'm just not sure it's the best way to do things. So, what we do depends on how you feel about the iframe idea. If you think it's a good idea we should do right now, let's hold off on these patches. If you think it's probably not worth the hassle, let's go ahead with the patches. If you think it might be something to think about in the future, let's put the patches in but keep them separate. Then we can back out the second one if and when we go the iframe route.
Just a reminder of these patches. My recommendation would be to take them as two patches, so we can back out the second if we ever move to iframes. Before you accept them, though, we should probably move stylish-webview.vala from src/client/util/ to src/client/components.
I agree. Let's land the patches with the code moved as you suggest. Please commit!
Pushed to master: commits ef98b7d1 and 4cfd18b5