452711 - use firefox default font chooser for display

Status: RESOLVED FIXED

(Thunderbird :: Preferences, defect)

Description

[Bryan Clark (DevTools PM) [:clarkbw]]

Attachment: port of firefox font display

Our current interface for changing the fonts in the Display preferences is scary and hidden.  An easy way to catch up to modern times would be to take what Firefox has done and begin using that for our font preferences.

This means that the default font option gets moved to a menulist on the main tab pane with a sizing option beside it.  The original font dialog is now launched from the (Advanced...) button.  The font related plain text items have been moved into the advanced dialog.

This is basically a straight port of the firefox code over to Thunderbird.  I didn't reuse the HTML font chooser from the Compose pane because it is quite awkward and likely could use to be converted to this system.

Comment 1

[Bryan Clark (DevTools PM) [:clarkbw]]

Attachment: screenshot of the font changes

Here's an after shot of the changes to the display preferences and the font dialog

Comment 2

[rsx11m]

Yes, this would be very helpful. Many users are trying to modify the font-size preferences in the Composition pane instead, as they don't find the "real" setting they need to find. Putting this upfront in the Display pane with an "Advanced" button would be beneficial for the novice users.

If this does provide an interface for the proportional font sizes only, should the monospace font be presented in a similar way, or can those two sizes be somehow connected? Messages or paragraphs received in preformatted style may be unaffected by the size selection otherwise.

Comment 3

[Magnus Melin [:mkmelin]]

rsx11m: that's why there is the "allow messages to use other fonts" option. These settings just affect fonts in general. For html mails the fonts they have specified are used, if none are specified, the defaults are used (I think).


How about getting rid of the radio buttons to make it
 "Use a fixed width font for plain text messages [ x ]"

Comment 4

[rsx11m]

I just did some testing based on Magnus' comment. The "allow messages to use other fonts" option does not change the distinction between variable and fixed fonts. Everything explicitly sent with "variable" font uses the pixel size in the upper box, whereas everything with "fixed" font (or sent with "preformat" style, also replies to non-flowed plain-text messages) uses the size specified in the lower box (you can set them to extreme values, e.g., 8 vs. 24 to see which one is used for a given text). Thus, it would be good to have both of those sizes visible in the main pane, perhaps

   Default font: [ sans-serif    v ]  [ Advanced... ]
   Size (pixels): Proportional [15v], Monospace [12v]

Comment 5

[Bryan Clark (DevTools PM) [:clarkbw]]

>    Default font: [ sans-serif    v ]  [ Advanced... ]
>    Size (pixels): Proportional [15v], Monospace [12v]

That's a pretty good change, but I'd still rather have a really simple font selector for people and avoid the mono vs. proportional confusion.  Though for the font control to work uniformly we'd have to default to a proportional font everywhere.

> How about getting rid of the radio buttons to make it
>  "Use a fixed width font for plain text messages [ x ]"

Yeah that would clean up that piece a lot.  I'll give it a try.

*Font Control*
[x] Allow messages to use other fonts
[ ] Use fixed width font for plain text messages

- Though I wonder if I should be using "Monospaced font" in this section as "Proportional" and "Monospaced" are both there in the section above.  I don't really see "fixed width" as more user friendly in this context.

Comment 6

[Bryan Clark (DevTools PM) [:clarkbw]]

Attachment: port of firefox font display v2

Here's an updated version of the patch with the changes described.  Screenshot to follow

Comment 7

[Bryan Clark (DevTools PM) [:clarkbw]]

Attachment: screenshot of the font changes v2

here's the screenshot of the updated patch

Comment 8

[Bryan Clark (DevTools PM) [:clarkbw]]

also, I just found bug 323517 which highlights the terminology issues of fixed/variable and proportional/monospaced

Comment 9

[Bryan Clark (DevTools PM) [:clarkbw]]

Just wanted to make a clear note on why the default preference change for mail.fixed_width_messages

Proportional and Monospaced fonts sizes can't be set to the same sizes with good results, monospace fonts will often look much larger.  So there isn't a way to have a single font size selector that changes all the fonts used in messages display.  However we need a single font size selector for the default case.

By defaulting to using proportional fonts for all messages being displayed we can have a single font sizer that changes all the displayed fonts.  In a default scenario a person could receive a plain text message or an html message and use the display font preference to change the font size of both messages to a larger or smaller value.

In the use case where a person has set monospaced fonts for plain text messages they can then continue using the main interface for changing proportional font sizes and the advanced font popup for changing either of the font sizes.

Our users require a single place to change all font sizes.  For the people who don't understand or don't care about the difference between monospaced and proportional fonts we need to have a way to change the font size.  We could try to educate users about the difference in fonts however this kind of user education often returns very limited rates of success.  If we have the two font size selectors users are likely to set the font sizes to the same larger value; this will render some messages very large and some messages smaller.  And the visual difference between the plain text message and the html message is often not distinguishable.

It takes 5 total clicks to change this preference back to the current default -> Edit/Tools, Preferences, Display, (Fonts) Advanced, and [x] ...Plain Text.  However I think there are several things we can do to make this preference irrelevant in the longer term.

* bugzilla mail expects monospaced fonts
** bug mail can be easily detected and should likely default to a monospace font
** this would require some of the planned changes to the filtering system
* newsgroups should likely default to monospace fonts
** this will probably require a pref for newsgroups

It might be good to open bugs for some of these cases as this change moves forward.

Comment 10

[Peter Lairo]

Somewhat unrelated, but a pet peeve of mine (and for corporate users):

When "Tools / Options / Composition / HTML / Font = ARIAL" is chosen, and the user does any of the following:

- creates bullet/numbered list
- interspersed quoting
- other?

Then the following text will change to Times New Roman (serif).

This makes e-mails not look the way the user intended, not to mention unprofessional and a conglomeration of sans-serif and serif fonts.

Comment 11

[Ronald Killmer]

I looked at the V2 screenshot and think it's a good improvement over the current dialog. One reason I say that is the placement of the Advance button. It is clear there are more personalization possibilities. 

How does this relate to the Compose tab for selection of Write window default fonts?  Are You using another bug for rework of the compose tab?

Comment 12

[Bryan Clark (DevTools PM) [:clarkbw]]

I don't really like the compose tab font chooser because it's asking the person to help select a default font for writing mail.  I'd much rather we just remember the last font used for writing mail and keep that as the default font for composing mail.

There are obvious consequences of this kind of remember the last choice default but having a preference for defaults rarely actually resolves those consequences, it just moves it around.  People with real font-changing needs would be helped more by a good template system.

So I guess my composer tab font bug would be a separate bug: 'Remove compose font preferences, remember fonts used in composer'

Comment 13

[Ronald Killmer]

When You open such bug would You put Me on the CC list please.

Comment 14

[Bryan Clark (DevTools PM) [:clarkbw]]

Attachment: udpated patch

I'm pretty sure this is ready for review but I don't want to slow down the reviewers with this.  I'll target this bug for b4 so I remember to get it in by then.

Comment 15

[Mark Banner (:standard8)]

Bryan, can you request review now so we can get this bug fixed?

Comment 16

[Ronald Killmer]

Before this goes final for review, one minor tweak suggestion.  Since the length of the strings sans-serif, etc. are shorter than the width of the picker, narrow the pickers width. You will then have less congestion in that row to accommodate the Advance button.

Comment 17

[Bryan Clark (DevTools PM) [:clarkbw]]

(In reply to comment #16)
> Before this goes final for review, one minor tweak suggestion.  Since the
> length of the strings sans-serif, etc. are shorter than the width of the
> picker, narrow the pickers width. You will then have less congestion in that
> row to accommodate the Advance button.

I think that menulist is given a flex=1

I'll try removing that and see what it looks like.

Comment 18

[David Ascher (:davida)]

hoping blake can whip through this.

Comment 19

[Blake Winton (:bwinton) (:☕️)]

Attachment: The menulist with flex="1" removed.

(In reply to comment #17)
> > Before this goes final for review, one minor tweak suggestion.  Since the
> > length of the strings sans-serif, etc. are shorter than the width of the
> > picker, narrow the pickers width. You will then have less congestion in that
> > row to accommodate the Advance button.
> I think that menulist is given a flex=1
> I'll try removing that and see what it looks like.

It doesn't look so good, particularly on the right side.  :)

It's true that "sans-serif" is smaller than the space given, but "Bitstream Vera Sans Mono" and "Hiragino Maru Gothic ProN" both seem to be larger than the space given, so I don't think we can reduce the space given to the dropdown all that much.  I did throw in a 15px spacer before the Advanced button, to try and loosen it up a little, and I'll throw up a picture of that next.

Later,
Blake.

Comment 20

[Blake Winton (:bwinton) (:☕️)]

Attachment: The dialog with the 15px spacer.

Comment 21

[Blake Winton (:bwinton) (:☕️)]

Attachment: The patch for the previous screenshot. (Heavily based off of Bryan's.)

Bryan, let me know how you like this version, and I'll clean up the javascript and post the new patch for code-review.

Comment 22

[Ronald Killmer]

The new screen shot looks cleaner to me, befits from losing the graphic in the button and fixed the crowding I commented on.

Comment 23

[Magnus Melin [:mkmelin]]

Re the "(pixels)" label we currently have, is there a reason not to put the uniti inside the dropdown like 
[ 16 px ]
| 17 px |
| 18 px |

Comment 24

[Blake Winton (:bwinton) (:☕️)]

Attachment: The previous patch, cleaned up and with mkmelin's suggestions

(In reply to comment #23)
> Re the "(pixels)" label we currently have, is there a reason not to put the
> uniti inside the dropdown like 
> [ 16 px ]
> | 17 px |
> | 18 px |

Changed.

(My only potential question would be how it looked from a localization point of view.  Is "px" for pixel common enough for people who don't speak english to understand?  It's everywhere in css/html, but is that good enough?)

Later,
Blake.

Comment 25

[Axel Hecht [:Pike]]

I don't think that hardcoding "n px" works well.

You could do a plural-dependent localizable property instead, but asking :tb-l10n for wider input.

For Brian's ui-review, I'm not sure if the font size is really that ambiguous that it requires adding a unit to it. Didn't investigate this in-depth of where it got added and why.

Comment 26

[Bryan Clark (DevTools PM) [:clarkbw]]

(In reply to comment #25)
> For Brian's ui-review, I'm not sure if the font size is really that ambiguous
> that it requires adding a unit to it. Didn't investigate this in-depth of where
> it got added and why.

Yeah, I agree.  We could put the unit of measure in the title for the label so it shows on hover but the majority of people are looking for "bigger or smaller than currently", not 14 pixels of height exactly; which is pretty inaccurate anyway.

Comment 27

[Blake Winton (:bwinton) (:☕️)]

Attachment: The previous patch, without the "px"s.

(In reply to comment #26)
> (In reply to comment #25)
> > For Brian's ui-review, I'm not sure if the font size is really that
> > ambiguous that it requires adding a unit to it. Didn't investigate this
> > in-depth of where it got added and why.
> Yeah, I agree.  We could put the unit of measure in the title for the label so
> it shows on hover but the majority of people are looking for "bigger or
> smaller than currently", not 14 pixels of height exactly; which is pretty
> inaccurate anyway.

Okay, I've removed the px-s, and the (pixel)-s.  If it's not accurate, I'ld rather not put it in the title, even.

Thanks,
Blake.

Comment 28

[Phil Ringnalda (:philor)]

It was added (very roughly speaking) in bug 89842, because bz used a single profile on different machines with different monitor resolutions and by having it say whether it was pixels or points he would know whether he needed to choose something larger than perfect for one system so it would still be readable on another one. I'm sure there must be fives, though probably not tens, of users in that same situation and class of knowledge ;)

Comment 29

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 394964 [details] [diff] [review]
The previous patch, without the "px"s.

looks good to me.

Comment 30

[Bryan Clark (DevTools PM) [:clarkbw]]

The next part of this, which may be an additional bug, is setting the correct language according to the message being displayed.  Currently we have an issue for many users who may set the default font but it only sets for their language.  

Any message which arrives as utf-8 or "Other Languages" will use a different font.  The advanced font chooser allows a person to edit this but I think for the not so advanced person we need a better system.  It was suggested that we could look for the Character encoding of the displayed message and use that for the "default" we would be changing.  This way a person changing the font size in the prefs would always see the change being applied to the message being displayed (something that doesn't always happen otherwise).

With this change we might also want to at least show the character encoding as a label in this default view.

Comment 31

[Ronald Killmer]

Reply to Comment #30

Yes, You have hit on one of the sophisticated ability's of Moz with the extensive list of hidden prefs. I ended up using Config Edit / about:config to maualy reset dozens of the font prefs and sizes.

One solution is more usage of *.otf (Open Type Format) fonts with very wide arrays of language encodings. The picking of one font can deal with display of many languages with a consistent look and feel. The limit is e-mail generators properly MIME typing the text encodings throughout a mixed encoded message.

Comment 32

[Mark Banner (:standard8)]

The Thunderbird drivers wish to release Thunderbird 3 as soon as possible. As a
result, we feel that this bug shouldn't stand in the way of all the other good
work getting into the hands of users sooner rather than later. Therefore we are
retargeting it for 3.1. See http://ccgi.standard8.plus.com/blog/archives/242
for more details. The 3.1 release is expected to be a quick release soon after
Thunderbird 3.

Comment 33

[Magnus Melin [:mkmelin]]

Comment on attachment 394964 [details] [diff] [review]
The previous patch, without the "px"s.

>+pref("mail.fixed_width_messages", false); // plain text messages use variable width font

Don't want this change, we'd get huge complaints. 

>-    this.mTagListBox = document.getElementById('tagList');    

Make that double quotes while you're at it ;)

>+  readRDFString: function(aDS,aRes,aProp)

While you're touching it, add spaces between the args

>+  writeFixedWidthForPlainText: function ()
>+  {
>+    var mailFixedWidthMessages = document.getElementById("mailFixedWidthMessages");
>+    return mailFixedWidthMessages.checked ? 1 : 0;
>+  },

It's a boolean pref, so return true : false
 
>-<!ENTITY  sizes.label                             "Size (pixels)">
>+<!ENTITY  size.label                              "Size:">
> <!ENTITY  proportionalSize.accesskey              "e">
> <!ENTITY  monospaceSize.accesskey                 "x">

The x doesn't work nicely anymore.

r=mkmelin with those fixed

Comment 34

[Axel Hecht [:Pike]]

Comment on attachment 394964 [details] [diff] [review]
The previous patch, without the "px"s.

size.label should change name if localizers are expected to pick this change up.

Comment 35

[Magnus Melin [:mkmelin]]

It did (it was sizes.label, is now size.label)

Comment 36

[Blake Winton (:bwinton) (:☕️)]

Attachment: The previous patch, with mkmelin's suggestions.

(In reply to comment #33)
> (From update of attachment 394964 [details] [diff] [review])
> >+pref("mail.fixed_width_messages", false); // plain text messages use variable width font
> Don't want this change, we'd get huge complaints. 

Strongly agreed!  Fixed.

> >-    this.mTagListBox = document.getElementById('tagList');    
> Make that double quotes while you're at it ;)

Fixed, and all the other ones in the file, cause I was there.

> >+  readRDFString: function(aDS,aRes,aProp)
> While you're touching it, add spaces between the args

Fixed.

> >+  writeFixedWidthForPlainText: function ()
> >+  {
> >+    var mailFixedWidthMessages = document.getElementById("mailFixedWidthMessages");
> >+    return mailFixedWidthMessages.checked ? 1 : 0;
> >+  },
> It's a boolean pref, so return true : false

Changed to "return mailFixedWidthMessages.checked;".  I also changed "return useDocumentFonts.checked;", because it followed the same pattern.

> >-<!ENTITY  sizes.label                             "Size (pixels)">
> >+<!ENTITY  size.label                              "Size:">
> > <!ENTITY  proportionalSize.accesskey              "e">
> > <!ENTITY  monospaceSize.accesskey                 "x">
> The x doesn't work nicely anymore.

I changed it to "i", cause that's the only character that wasn't taken.  Well, I guess I could have changed it to ":".  ;)

> r=mkmelin with those fixed

Thanks,
Blake.

Comment 37

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/1d88547281a4