107877 - [RFE] Ability to Specify Font, Style Size in Mail Composition and Replies

Status: VERIFIED FIXED

(MailNews Core :: Composition, enhancement, P1)

Description

[Thomas Swan]

Be able to specify styles for new mail composition and another for replies, 
e.g. font, color, size.

Comment 1

[Samir Gehani]

-> MailNews

Comment 2

[Thomas Swan]

As a preference of course

Comment 3

[Keyser Sose]

Marking nEW.

Comment 4

[Jean-Francois Ducarroz]

-->varada

Comment 5

[Joel Nelson (don't send email)]

Change Platform from PC to ALL to add support for Macintosh.

Comment 6

[Niranjani Raya]

Are there any prefs that we could set through the GUI to acheive this?

Comment 7

[Sheela Ravindran]

*** Bug 119593 has been marked as a duplicate of this bug. ***

Comment 8

[Thomas Swan]

Changed platform per comment #5

Comment 9

[Joel Nelson (don't send email)]

OS is now changed to "All" but Platform is still listed as "PC".   Platform 
should be changed to "All".

Comment 10

[Joel Nelson (don't send email)]

This should include ability to choose from any font installed on the 
computer and not just the "Variable Width", "Fixed Width", "Arial, Helvetica", 
"Times", and "Courier".

Comment 11

[Thomas Swan]

You could do the font chooser, and select a fall-back font to display if that
font is not available.  Or use the pref from the "view mail using font..." to
decide the fall-back font.

Personally, I'd just be happy with choosing between the basic fonts, but I think
"doing it right" should list the installed fonts on the computer.

Comment 12

[Thomas Swan]

Additional thoughts: 

Use your default compose text color as your reply text color.   On those
multithreaded email conversations, it would be nice to have some color coded
messages :)

Comment 13

[jglick]

Attachment: Example1

Comment 14

[jglick]

Attachment: Example2

Comment 15

[Joel Nelson (don't send email)]

Attachments look good; any idea on when we can expect to begin seeing this in 
Mozilla builds?  

Comment 16

[scottputterman]

If it fits, I like #2 better with the separate group box.

Comment 17

[jglick]

Lets try for Example2 if the content will fit properly without being cropped. 

Note to Shuehan: If you work on this bug, please note the "Character Coding" 
dropdown pref that currently appears on this panel is moved to a new "Language" 
panel.

http://www.mozilla.org/mailnews/specs/prefs/PreferencesP.html

Comment 18

[scottputterman]

reassigning to shliang.

Comment 19

[Joel Nelson (don't send email)]

Will the default font actually be applied to the message (so that the 
recipient also sees the same font) or is it for display purposes on the 
sender's machine only (so that the sender would have to choose a font for 
each message)?     I assume this would require that HTML or 
Multipart/Alternative mail be enabled in preferences.    If Plaintext was 
selected in preferences for message format would this tool be grayed 
out?

Comment 20

[scottputterman]

The default font and styles would get applied to any new text entered in the
message and would be seen by the receiver.  This would only occur in the html
compose window.

Comment 21

[Joel Nelson (don't send email)]

I also prefer Example 2.   Any idea on if this will make 0.9.9?   (I know 
that's the target.)

Comment 22

[Joel Nelson (don't send email)]

Suggesting Mozilla1.0 keyword...
   What is the status on this bug?

Comment 23

[Joel Nelson (don't send email)]

With the release of 1.0 within a few weeks away, will this bug be fixed in time 
to be included in 1.0?  Or is this more likely to be in 1.1?

Comment 24

[Joel Nelson (don't send email)]

Sorry about "spamming" this bug comment area again...

In response to Comment #11:

>>You could do the font chooser, and select a fall-back font to display if that
font is not available.  Or use the pref from the "view mail using font..." to
>decide the fall-back font.
>
>Personally, I'd just be happy with choosing between the basic fonts, but I >think
>"doing it right" should list the installed fonts on the computer.

  I may be missing something here, but why would a fall-back font be necessary?
 If the font is installed on the computer, why would it not be available?  
   The font chooser in the Preferences>Appearance>Fonts preference panel lists
every font on the computer.   This same font chooser should be used both for
default font for outgoing message and for the font chooser in the compose
window.  For that matter, also use it in Composer... web page creators will
probably want to have more than 3 fonts available for use.  
   This would involve  having the font chooser read the system fonts folder to
see which fonts are installed.  Currently it uses a preset list of fonts. 
  One possibility would be to use current list of three fonts but then beneath
that list (or in another menu opening off the current) a "Local Fonts" section.
 Another would be to replace the current font chooser and just use the "Local
Fonts" section.   The advantage of keeping the current list of three and also
showing all installed fonts is that if a sender is uncertain as to which fonts
the recipient has, they can use one of the three presets which actually uses
multiple fonts (i.e., "Times" is listed as "Times New Roman, Times" in the HTML
code).    
    There are a few ways this could be done but I would strongly recommend
adding to the font chooser to list local fonts and not just a preset list. 
  

Comment 25

[Thomas Swan]

As far as I can tell, I was thinking of the standard "web fonts" vs. any fonts
available on your personal system.   A fall back would be one of the standard
font families or the ever basic serif and sans serif fonts.

Comment 26

[R.K.Aa.]

isn't this a duplicate of bug 26288?

Comment 27

[Thomas Swan]

Not particularly.   This deals with a preference setting for fonts/colors to use
when composing an email message or replying to a message.

Comment 28

[Joel Nelson (don't send email)]

This bug is to specify a default font/size/style/color for outgoing messages. 
Bug 26288 is to make more fonts available when composing e-mail or creating a
web page.   

Comment 29

[Joel Nelson (don't send email)]

Which version of Mozilla is likely to have this enhancement added?

Comment 30

[scottputterman]

Changing nsbeta1+ [adt3] bugs to nsbeta1- on behalf of the adt.  If you have any
questions about this, please email [e-mail].  You can search for
"changing adt3 bugs" to quickly find and delete these bug mails.

Comment 31

[scottputterman]

Changing nsbeta1+ [adt3] bugs to nsbeta1- on behalf of the adt.  If you have any
questions about this, please email [e-mail].  You can search for
"changing adt3 bugs" to quickly find and delete these bug mails.

Comment 32

[(not reading, please use [e-mail] instead)]

this isn't going to be fixed for 1.0, let's move it to future.

I think the general feeling is that we've got more important bugs and feature to
do first.

Based on some things that shuehan's told me, I believe we'll need to fix some
issues / make some enhancements to editor to get this to work.

Shuehan, if you have a patch, consider attaching it, but marking it as "need work".

Comment 33

[(not reading, please use [e-mail] instead)]

Attachment: shuehan's initial patch (needs work)

Comment 34

[Joel Nelson (don't send email)]

We should get this in at least by 1.2, if not (hopefully) sooner.

Comment 35

[James Rome]

There is something else going on here. The font sizes are different in the mail
reading window and the composition window, I use non-html composition, and
Mozilla (7/29 - Linux) seems to use the fixed-width font for message
composition. But it is MUCH smaller in the compose window than in the read
window. As a result, I cannot set the font sizes so that they are right everywhere.

Comment 36

[Joel Nelson (don't send email)]

Any possibility of getting this bug in for 1.2 or 1.3?

Comment 37

[shliang]

Attachment: updated patch

updated patch, also adds ability to select from every font installed on the
computer

Comment 38

[Joel Nelson (don't send email)]

*** Bug 137451 has been marked as a duplicate of this bug. ***

Comment 39

[shliang]

Attachment: updated patch

Comment 40

[Andreas Kunz]

AFAICT this will also fix bug 129317, bug 158707 and possible duplicates.

Comment 41

[Chris Fisher]

How is this listed as an enhancement? This is something that used to work and
was broken by later code releases. This severity level this may be why this BUG
has persisted for more than a year.

Comment 42

[[e-mail]]

Comment on attachment 109493 [details] [diff] [review]
updated patch

This looks as if it will need someone familiar with editor to look at it.

Comment 43

[Kathleen :Brade]

Regarding the patch (from 2002-12-16):
What is the performance impact on mail compose window opening?  Has this been
tested with the cached mail compose window?

Since most of these fixes are mail-related, I'm not comfortable doing the
review.  Can we do the composer/editor changes in a separate bug or separate patch?

Joe Francis should also be involved with editor side stuff since he has been
working on core apis to set default font/size for apps.  I don't know the bug #
for that work.

Jag should know about this since he's also done some work in editor recently to
enable all fonts.

Comment 44

[shliang]

Attachment: patch

revised patch due to bug #26288 and cleaned up

Comment 45

[(not reading, please use [e-mail] instead)]

Comment on attachment 119217 [details] [diff] [review]
patch

seeking r from neil, I'll do the sr.

being editor, this is right up neils alley.

but here are some questions I had:

1)

take a look at what jag did in #26288.	does that affect your font code,
anything from his code that might change how you do this?
also, if by default, I choose to compose with font "foobar", does his UI show
the right thing?

2)

+	 try { var fontFace = gPrefs.getIntPref("mailnews.font_face"); } catch
(e) {}

are the editor people ok with mailnews specific code in editor.js?

maybe, this would be better (and would work for editor, im, and mailnews)

from the window, get the windowtype, and use that to build the pref:

so for mail compose, see messengercompose.xul, the type is "msgcompose".

then, we'd use this for the pref:  msgcompose.font_face

did 4.x have this feature or this pref?  we should use the same name, if so.


3)

remove the dump

+{  dump("init font face menu\n");

4)

+	 try { size = gPrefs.getIntPref("mailnews.compose.font_size"); } catch
(e) {}

see comment #2.

5)

does your code cause us to enumerate all fonts, when the compose window (or
editor) starts up?

if so, what's the performance impact on the mail compose window (and editor)

6) does this change play nice with the cached compose window?

see http://www.mozilla.org/mailnews/arch/compose/cached.html

7) does some of this font js you added get executed on the plain text mail
compose window, or the plain text editor?

8)

does this play nice, when I use an html signature?  what about reply?

Comment 46

[[e-mail]]

Comment on attachment 119217 [details] [diff] [review]
patch

I don't like your editor.js changes; I know editor can do all this stuff
(although it's probably disabled by default in message compose).

>Index: mailnews/compose/prefs/resources/content/pref-composing_messages.xul
>===================================================================
>RCS file: /cvsroot/mozilla/mailnews/compose/prefs/resources/content/pref-composing_messages.xul,v
>retrieving revision 1.39
>diff -u -r1.39 pref-composing_messages.xul
>--- mailnews/compose/prefs/resources/content/pref-composing_messages.xul	17 Mar 2003 04:59:08 -0000	1.39
>+++ mailnews/compose/prefs/resources/content/pref-composing_messages.xul	2 Apr 2003 22:52:41 -0000
>@@ -36,6 +47,42 @@
>         quotebox.lastChild.setAttribute("disabled","true");
>       }
>     }
>+
>+    function setColorWell(aPicker)
>+    {
>+      var colorRef = aPicker.nextSibling;
>+      colorRef.setAttribute( "value", aPicker.color );
>+    }
>+
>+    function getColorFromWellAndSetValue(aPickerId)
>+    {
>+      var picker = document.getElementById(aPickerId);
>+      var colorRef  = picker.nextSibling;
>+      var color = colorRef.getAttribute("value");
>+      picker.color = color;
>+      return color;
>+    }
>+
>+    function populateFonts() {
>+      var fontsList = document.getElementById("FontFacePopup");
>+      try {
>+        var enumerator = Components.classes["@mozilla.org/gfx/fontenumerator;1"]
>+                                   .getService(Components.interfaces.nsIFontEnumerator);
>+        var localFontCount = { value: 0 }
>+        var localFonts = enumerator.EnumerateAllFonts(localFontCount);
>+        for (var i = 0; i < localFonts.length; ++i) {
>+          if (localFonts[i] != "") {
>+            var itemNode = document.createElement("menuitem");
>+            itemNode.setAttribute("label", localFonts[i]);
>+            itemNode.setAttribute("value", i+7);
>+            itemNode.setAttribute("observes", "cmd_renderedHTMLEnabler");
Not required here. Also, this use of values looks odd - what if someone
installs a font beginning with A it will renumber all the entries. Same goes
for the other prefs, they should use a meaningful name rather than an opaque
index.

>Index: mailnews/compose/resources/content/MsgComposeCommands.js
>===================================================================
>RCS file: /cvsroot/mozilla/mailnews/compose/resources/content/MsgComposeCommands.js,v
>retrieving revision 1.322
>diff -u -r1.322 MsgComposeCommands.js
>--- mailnews/compose/resources/content/MsgComposeCommands.js	26 Mar 2003 01:54:27 -0000	1.322
>+++ mailnews/compose/resources/content/MsgComposeCommands.js	2 Apr 2003 22:52:41 -0000
>@@ -237,6 +237,8 @@
>     window.content.focus();
>     ComposeStartup(true, params);
>     enableEditableFields();
>+    
>+    loadHTMLMsgPrefs();
Wrong place for this, I think.

>@@ -1231,8 +1233,12 @@
>           document.getElementById("insertMenu").setAttribute("hidden", true);
>           document.getElementById("menu_showFormatToolbar").setAttribute("hidden", true);
>         }
>+
>+        var fontsList = document.getElementById("FontFacePopup");
>+        initLocalFontFaceMenu(fontsList);
Won't be needed if you don't apply font by number.

>@@ -1309,6 +1315,7 @@
>       var editor = GetCurrentEditor();
>       if (editor && GetCurrentCommandManager() == aSubject)
>         gMsgCompose.initEditor(editor, window.content);
>+      loadHTMLMsgPrefs();
Wrong place, try in composeFieldsReady just before adjusting focus.

+  if (paragraph) {
+    var paragraphPopup = document.getElementById("ParagraphPopup");
+    var paragraphChildren = paragraphPopup.childNodes;
+    var paragraphSelect = document.getElementById("ParagraphSelect");
+    paragraphSelect.selectedIndex = paragraph;
+    doStatefulCommand("cmd_paragraphState",
paragraphChildren[paragraph].getAttribute("value"));
+  }
I can't see how this will work if your prefs are to select the reply text (also
doStatefulCommand may require the editor to have focus...)

Comment 47

[(not reading, please use [e-mail] instead)]

Comment on attachment 119217 [details] [diff] [review]
patch

clearing the sr review request, until shuehan has neil's r=.

Comment 48

[shliang]

Attachment: patch

seth's comments:
1. jag's stuff has been incorporated into this patch
2. using windowtype for pref string now
3. dump removed
4. same as (2)
5. it does enumerate all fonts when the pref panel and compose window start up.
should i change this to fake having the menuitem selected without populating
the list, then populate the list if it is clicked?
6. seems to work fine with the cached compose window
7. fixed this in this patch
8. html signatures are fine

neil's comments:
>I don't like your editor.js changes; I know editor can do all this stuff
these are different now, i think everything i put in needs to be there but then
again, i don't really know the editor code so could you be more specific?

>+	      itemNode.setAttribute("label", localFonts[i]);
>+	      itemNode.setAttribute("value", i+7);
>+	      itemNode.setAttribute("observes", "cmd_renderedHTMLEnabler");
changed this

moved loadHTMLMsgPrefs() to in composeFieldsReady()

>+	  var fontsList = document.getElementById("FontFacePopup");
>+	  initLocalFontFaceMenu(fontsList);
Won't be needed if you don't apply font by number.
this is there to populate the menulist

+  if (paragraph) {
+    var paragraphPopup = document.getElementById("ParagraphPopup");
+    var paragraphChildren = paragraphPopup.childNodes;
+    var paragraphSelect = document.getElementById("ParagraphSelect");
+    paragraphSelect.selectedIndex = paragraph;
+    doStatefulCommand("cmd_paragraphState",
paragraphChildren[paragraph].getAttribute("value"));
+  }
I can't see how this will work if your prefs are to select the reply text (also

doStatefulCommand may require the editor to have focus...)
hmm. heading tags in editor only work on a single line basis, so what this does
with the reply text is apply the paragraph format to the first line only. this
seems to make sense based on how it currently works now (?)

Comment 49

[shliang]

Attachment: editor changes

Comment 50

[Kathleen :Brade]

Comment on attachment 120033 [details] [diff] [review]
editor changes

In editorOverlay.xul, is there some reason why <menulist
class="toolbar-focustarget" ...> is in this file?  Is it for Netscape's IM
client or only for mail?

I don't understand the changes in onFontFaceChange; it seems to me like you are
working around a bug when you should just fix the bug.

General style note: it is not consistent in editor JS files to have extra
parens around booleans such as "if ((gHasFontSizeDefault) && ..."
Please remove the extra parens.

I don't think we want to Set font size to 3 here:
+    EditorSetTextProperty("font", "size", "3");

Around line 1046 you added spaces to a blank line; please remove them so they
don't appear in the diff.

In initLocalFontFaceMenu, I don't understand what is going on in the block you
have added and there isn't a comment to explain it.

OK wait... this diff seems out of date since it isn't lining up with what I
expect in lxr.	It's *VERY* difficult to review this with so little context. 
Please update to latest versions in cvs and do a cvs diff -u10 to give more
context for future comments.

Also, I won't be able to give r=brade until jfrancis and jag are both ok with
the strategy going on here.

(not that it matters, but I'm not happy that so much of this is going into
composer files when it is mail-specific.

I haven't looked for recent comments, but I do expect that Composer and
Netscape's IM client have been tested and have no side effects or regressions
from these changes.

Comment 51

[[e-mail]]

Comment on attachment 119941 [details] [diff] [review]
patch

>+pref("msgcompose.paragraph",                0);
Another meaningless value?

>+        <menulist class="toolbar-focustarget" id="ParagraphSelect" prefstring="msgcompose.paragraph">
You don't want this class, it's to turn off the focus :-)

>+          <menupopup>
>+            <menuitem value="0" label="&bodyTextCmd.label;"/>
>+            <menuitem value="1" label="&paragraphParagraphCmd.label;"/>
>+            <menuitem value="2" label="&heading1Cmd.label;"/>
>+            <menuitem value="3" label="&heading2Cmd.label;"/>
>+            <menuitem value="4" label="&heading3Cmd.label;"/>
>+            <menuitem value="5" label="&heading4Cmd.label;"/>
>+            <menuitem value="6" label="&heading5Cmd.label;"/>
>+            <menuitem value="7" label="&heading6Cmd.label;"/>
>+            <menuitem value="8" label="&paragraphAddressCmd.label;"/>
>+            <menuitem value="9" label="&paragraphPreformatCmd.label;"/>
>+            <menuseparator value="10"/>
>+            <menuitem value="11" label="&paragraphBlockquoteCmd.label;"/>
I think you should change these to meaningful values as well (like for the
fonts).

>+            <menuitem value="0" label="&size-xx-smallCmd.label;"/>
>+            <menuitem value="1" label="&size-x-smallCmd.label;"/>
>+            <menuitem value="2" label="&size-smallCmd.label;"/>
>+            <menuitem value="3" label="&size-mediumCmd.label;"/>
>+            <menuitem value="4" label="&size-largeCmd.label;"/>
>+            <menuitem value="5" label="&size-x-largeCmd.label;"/>
>+            <menuitem value="6" label="&size-xx-largeCmd.label;"/>
Again, I think it would be better to use the font size name if that's possible.

>+        var fontsList = document.getElementById("FontFacePopup");
>+        initLocalFontFaceMenu(fontsList);
Don't think you need this any more.

I understand that jfrancis is working on some backend code that would make it
easier to set default styles.

Comment 52

[shliang]

Attachment: editor changes, revised

so, i realized that most of the stuff that i put in the earlier patches isn't
needed anymore (but seemed to be necessary when i first worked on this bug, a
year ago) 
the only remaining change to editor.js is to the initLocalFontFaceMenu function
- 1. i am using this to populate the menulist in the compose window, which
doesn't need type="radio" for it's menuitems, and 2. after the first time this
function is called it doesn't need to enumerate the fonts again but it still
needs to append the menuitems if called with the other menu.

Comment 53

[shliang]

Attachment: mailnews changes, revised

Comment 54

[jag (Peter Annema)]

Comment on attachment 120143 [details] [diff] [review]
editor changes, revised

>Index: editor/ui/composer/content/editor.js
>===================================================================
>RCS file: /cvsroot/mozilla/editor/ui/composer/content/editor.js,v
>retrieving revision 1.210
>diff -u -r1.210 editor.js
>--- editor/ui/composer/content/editor.js	7 Apr 2003 08:11:21 -0000	1.210
>+++ editor/ui/composer/content/editor.js	11 Apr 2003 00:17:57 -0000
>@@ -1038,7 +1038,7 @@
>   }
> }
> 
>-function initLocalFontFaceMenu(menuPopup)
>+function initLocalFontFaceMenu(menuPopup, useRadioType)

Don't need this if you're checking the menu type below.

>@@ -1049,21 +1049,27 @@
>                                  .getService(Components.interfaces.nsIFontEnumerator);
>       var localFontCount = { value: 0 }
>       gLocalFonts = enumerator.EnumerateAllFonts(localFontCount);
>-      for (var i = 0; i < gLocalFonts.length; ++i)
>+    }
>+    catch(e) { }
>+  }
>+  var fixedMenuItems = 7;

Make this const.

>+        if (useRadioMenuitems)
>           itemNode.setAttribute("type", "radio");
>-          itemNode.setAttribute("name", "2");

I think you can leave this under the |useRadioMenuitems|, the menulist doesn't
need it.

sr=jag with that.

Comment 55

[[e-mail]]

Comment on attachment 120147 [details] [diff] [review]
mailnews changes, revised

Hmm... where did your prefs go?

> <?xml-stylesheet href="chrome://communicator/skin/" type="text/css"?>
>+<?xml-stylesheet href="chrome://messenger/skin/prefPanels.css" type="text/css"?>
Don't need two stylesheets

>-<!DOCTYPE page SYSTEM "chrome://messenger/locale/messengercompose/pref-composing_messages.dtd">
>+<!DOCTYPE window [
Don't change doctype to window

>+      var fontsList = document.getElementById("FontFacePopup");
>+      try {
>+        var enumerator = Components.classes["@mozilla.org/gfx/fontenumerator;1"]
>+                                   .getService(Components.interfaces.nsIFontEnumerator);
>+        var localFontCount = { value: 0 }
>+        var localFonts = enumerator.EnumerateAllFonts(localFontCount);
>+        for (var i = 0; i < localFonts.length; ++i) {
>+          if (localFonts[i] != "") {
>+            var itemNode = document.createElement("menuitem");
>+            itemNode.setAttribute("label", localFonts[i]);
>+            itemNode.setAttribute("value", localFonts[i]);
>+            fontsList.appendChild(itemNode);
As the FontSelect element is a menulist and therefore implements
nsIDOMXULSelectControlElement you could consider using its appendItem method.

>+            <menuitem value="varwidth" label="&fontVarWidth.label;"/>
>+            <menuitem value="b" label="&bodyTextCmd.label;"/>
I don't see why you can't copy EditorOverlay's use of value="" here.

>+        var fontsList = document.getElementById("FontFacePopup");
>+        initLocalFontFaceMenu(fontsList);
Surely this only applies to HTML mail?

>+function loadHTMLMsgPrefs() {
>+  var pref = GetPrefs();
>+
>+  var paragraph;
>+  var fontFace;
>+  var fontSize;
>+  var textColor;
>+  var bgColor;
>+  
>+  try { paragraph = pref.getCharPref("msgcompose.paragraph"); } catch (e) {}
>+  try { fontFace = pref.getCharPref("msgcompose.font_face"); } catch (e) {}
>+  try { fontSize = pref.getCharPref("msgcompose.font_size"); } catch (e) {}  
>+  try { textColor = pref.getCharPref("msgcompose.text_color"); } catch (e) {}
>+  try { bgColor = pref.getCharPref("msgcompose.background_color"); } catch (e) {}
>+  
>+  if (paragraph) {    
>+    if (paragraph == "b")
>+      paragraph = "";
>+    doStatefulCommand("cmd_paragraphState", paragraph);
>+  }
>+
>+  if (fontFace) {
>+    if (fontFace == "varwidth") 
>+      fontFace = "";
>+    doStatefulCommand('cmd_fontFace', fontFace);
>+  }
>+
>+  if (fontSize) {
>+    EditorSetFontSize(fontSize);
>+  }
>+
>+  var bodyElement = GetBodyElement();
>+
>+  if (textColor) {
>+    bodyElement.setAttribute("text", textColor);
>+    gDefaultTextColor = textColor;
>+    document.getElementById("cmd_fontColor").setAttribute("state", textColor);
>+    goUpdateCommand("cmd_fontColor");
>+  }
>+
>+  if (bgColor) {
>+    bodyElement.setAttribute("bgcolor", bgColor);
>+    gDefaultBackgroundColor = bgColor;
>+    document.getElementById("cmd_backgroundColor").setAttribute("state", bgColor);
>+    goUpdateCommand("cmd_backgroundColor");
>+  }
>+}
>+
Would calling onFont/BackgroundColorChange help here?

Also for all of these, could you put each one in the relevant try/catch e.g.

try {
  var fontSize = pref.getCharPref("msgcompose.font_size");
  EditorSetFontSize(fontSize);
} catch (e) {}	

Sorry for not spotting some of these before. At least the patch gets smaller
each time :-)

Comment 56

[[e-mail]]

Comment on attachment 120143 [details] [diff] [review]
editor changes, revised

>+  var fixedMenuItems = 7;
>+  if (menuPopup.childNodes.length < gLocalFonts.length + fixedMenuItems) 
I've twigged that this gets called once for the format font menu and once for
the compose font dropdown but I don't see it getting called any more times so
this check should be pointless.

Comment 57

[[e-mail]]

Comment on attachment 120143 [details] [diff] [review]
editor changes, revised

>+  var fixedMenuItems = 7;
>+  if (menuPopup.childNodes.length < gLocalFonts.length + fixedMenuItems) 
Oh I see now, this gets called every time the font menu is opened, in which
case you need to init the font dropdown in the same way, so that the list is
only filled if the dropdown is actually opened. Also, why not check for
menuPopup.childNodes.length == kFixedMenuItems?

Comment 58

[jag (Peter Annema)]

See bug 198546 for a rewrite of that code you're touching there.

Comment 59

[[e-mail]]

Or do you need the entire menulist precreated for some reason?

Comment 60

[shliang]

Attachment: editor changes

Comment 61

[shliang]

Attachment: mailnews changes

addressed comments

just one thing, for paragraph state it seems to need all those lines, and the
one line that is outside of the try/catch seems to screw things up if i move it
inside (like reset the font face)

Comment 62

[[e-mail]]

Comment on attachment 120252 [details] [diff] [review]
mailnews changes

Bad news. My longest font name, adobe-new century schoolbook-iso10406-1 pushes
the style menulist off the edge of the prefwindow :-( Swapping the size and
style menulists fixed the problem for me, but only in my theme (Classic and
Modern themes take up too much space...)

>+      var prefService = Components.classes["@mozilla.org/preferences-service;1"]
>+                                  .getService(Components.interfaces.nsIPrefService);
>+      var prefs = prefService.getBranch(null);
>+
>+      var fontsList = document.getElementById("FontSelect");
>+      if (!prefs.getCharPref("msgcompose.font_face"))
>+        fontsList.selectedIndex = 0;
>+
>+      var paragraphList = document.getElementById("ParagraphSelect");
>+      if (!prefs.getCharPref("msgcompose.paragraph"))
>+        paragraphList.selectedIndex = 0;
>+
This is not the way to fix a bug in the widget state manager. Line 246 of
nsWidgetStateManager.js should say element.value = aDataObject.value; not the
crap that's currently there, so I filed bug 201964.

>+          <menupopup id="FontFacePopup">
Probably don't need this id.

>+
>+        var fontsList = document.getElementById("FontFacePopup");
>+        initLocalFontFaceMenu(fontsList);
>+
Don't need this in plain text mail.

When I tried this, it appeared to work (my font menulist showed the right font)
but the font and size settings didn't "stick". The colours and paragraph format
were fine. Perhaps you're setting the styles in the wrong order, or something?
Or maybe jfrancis can figure it out.

Comment 63

[shliang]

Attachment: mailnews changes

hmm...the font face/size settings seem to stick for me. 
i modified the font face block a bit, taking from what they do in aim, so
hopefully that will fix that for you. not sure what to do about font size
though because they do it differently for aim (only have smaller/larger) and
also because i can't see the problem on my end.

Comment 64

[[e-mail]]

Comment on attachment 120490 [details] [diff] [review]
mailnews changes

OK, the font width is nicer now, although compose isn't quite usable at 640x480
(the default window size!) any more :-/

And you'll take out the comment I hope.

Comment 65

[Kathleen :Brade]

Comment on attachment 120251 [details] [diff] [review]
editor changes

kFixedMenuItems is not very specific in this file; I think you need to have
"Font" or "FontFace" in there.

I don't really like the addition of the menu separator in editorOverlay.xul in
case some other user of the menu wanted just those 3 (is there such a case?). 
Instead, I'd want the separator hidden and shown only when appropriate.  As
long as this doesn't break Netscape IM, I'm ok with it as is.

r=brade with the constant name change.

Comment 66

[shliang]

Attachment: complete patch

Comment 67

[shliang]

Comment on attachment 120659 [details] [diff] [review]
complete patch

sr=sspitzer

Comment 68

[shliang]

resolving

Comment 69

[rbs]

 pref("mailnews.message_display.disable_remote_image", false);
 pref("mailnews.message_display.allow.plugins", true);
 
+// default msg compose font prefs
+pref("msgcompose.font_face",                "");
+pref("msgcompose.paragraph",                "");
+pref("msgcompose.font_size",                "medium");
+pref("msgcompose.text_color",               "#000000");
+pref("msgcompose.background_color",         "#FFFFFF");


==================

Might have been nicer to use mailnews.msgcompose.* so that about:config groups
nicely (and will fit better if/when there is a +/- clickable tree view).

Comment 70

[[e-mail]]

Hey, the paragraph select menulist went missing!

Comment 71

[(not reading, please use [e-mail] instead)]

I think this caused a regression

by default, even for a simple html message, we can't auto-convert to plain text
(like we used to).  So the user will see the "HTML Mail Question" dialog, where
we ask them to send in text, text & html, or just html.

by simple html message, I mean compose an html message, but do no html

here's what I think we are generating:

<body text="#000000" bgcolor="#ffffff">
<font face="">test</font><br>
</body>

I'm going to look into this, to avoid the "HTML Mail Question" dialog, when ever
possible.

Comment 72

[(not reading, please use [e-mail] instead)]

this isn't new, but it's related.

in editorUtils.js, we have GetDefaultBrowserColors(), which we use to set the
default fg and bg colors of an editor document, based on the users choices for
web page fg and bg.

so, those impact mail compose.  (digression, they also afftect message display!
but I think ssu has a bug on that.  he has a bug on making seperate prefs for
mail display, which will fix that problem.)

to avoid the "HTML Mail Question" dialog, I have a change to loadHTMLMsgPrefs(),
to avoid doing work if we are going to use the defaults.

but because of some callers to GetDefaultBrowserColors() in editor*.js, we'll
now default our html mail compose to the browser defaults.

Comment 73

[(not reading, please use [e-mail] instead)]

I'll just log a spin off bug on the HTML Mail Question dialog issue, and assign
to shuehan.

Comment 74

[(not reading, please use [e-mail] instead)]

*** Bug 38286 has been marked as a duplicate of this bug. ***

Comment 75

[(not reading, please use [e-mail] instead)]

for completeness, the related bugs about message display, are:

bug #30896  [no UI for "Use my default fonts" pref for mail / message display]
bug #63809  [Message display background color ignores preferences]
bug #202560 [browser default colors (for fg / bg color) affect mail / message
display]

the bug about the HTML Mail Question dialog is bug #202561

Comment 76

[[e-mail]]

Well at least the font size/face settings stick for me now...

Comment 77

[shliang]

closing again...i decided to leave out the paragraph pref at the last moment
because it was interfering with font face and size, i.e. it resets them and
makes them appear not to be working. also it didn't seem to make that much sense
to have in there anyway.

Comment 78

[nebigredfan]

>closing again...i decided to leave out the paragraph pref at the last moment
>because it was interfering with font face and size, i.e. it resets them and
>makes them appear not to be working. also it didn't seem to make that much sense
>to have in there anyway.

 We use the Paragraph drop-down menu often.   Some of our contacts use e-mail
software that does not handle "Preformat" text well.   When we are cutting and
pasting a plain-text quotation into a HTML message we always change "Preformat"
to "Paragraph" or "Body Text" so the recipients of the message can properly view
the message (due to an error in their mail software that does not render the
<TT> tag properly).    Some programs leave the preformat text all on one long
line instead of wrapping the text to window width.  

Comment 79

[esther]

Using trunk build 20030428 on winxp, macosx and linux I did a basic test of this
feature, changing the font style, font color, font size and background color.  I
sent the message, exited and relaunched to see that this was still the default.
 Checked to see that this default didn't appply to the Composer.  I clicked
Reply and Forwared and found side affect of this bug regarding prepopulated text
and logged the bug 203810 .   Verified 

Comment 80

[Brian Heinrich]

While I realise the bug's been VERIFIED FIXED, I'd suggest that the current
implementation is incomplete.  (Read:  I'm not spamming the bug . . . honest!)

Previously, font, size, font colour, and background colour were all, by default,
determined by the recipient's MUA (tho' there was some limited ability to
manipulate them).

Personally, I liked this functionality, and fail to see why my inability
potentially to annoy my recipients with my choice of font, size, colour, and
background colour was considered a bad thing.

Now, however, I can still avoid defining a font and size, which will then be
determined by my recipient's MUA; however, there is no way to avoid defining
text and background colours, since defaults are already defined.

Would it not make more sense to have defaults that are not defined? -- that is,
in the UI, text and background colours would be given as [default] unless I
otherwise define them and would not be given in the underlying HTML /until/ or
/unless/ I define them; /i.e./, the current |<body text="#000000"
bgcolor="#ffffff">| would just be |<body>| unless/until I defined black as the
text colour and white as the background colour.

Such an implementation would retain the old behaviour (for those of us who
preferred it) while still giving users the ability to define font, size, and
text and background colours to their hearts' content if they desire to do so.

Comment 81

[nebigredfan]

Another comment; I don't intend to spam this bug -- shall a new one(s) be opened
for the minor issues that have come up since it was fixed.

Regarding Comment #80  --   This would be a good addition.   It might be best to
implement it as a preference ("Send plain text if no formatting is selected").

If a text color is selected it also changes the color of the original text in
replies and forwards.   If a user has selected a default font color it might be
best if the font color only were applied to /their/ text and not the quoted
text, making the reply text stand out.

Comment 82

[Thomas Swan]

The ability to specify font colors, sizes, etc is present.  Therefore, this
should remained closed.  As for Comment 81, the changing of the received text
color should be considered a new bug.

Comment 83

[Thomas Swan]

Bug 204974 was filed regarding replying to messages. See Comment 81 and Comment 82

Comment 84

[Brian Heinrich]

In re. comment 81:  I was apparently unclear.  Moz already sends plain text far
more often then I would like, even when some HTML formatting has occurred
(since, amongst other things, it is a possible source of confusion amongst
users); what I was suggesting was that a previously existing functionality -- a
certain /transparency/ of HTML as regards font face/size, text colour, and
background colour -- has been lost and that I would like to see it restored (as
default behaviour (since not selecting a font currently maintains the current
behaviour)).

In re. comment 82:  I respectfully disagree.  While the ability to specify font
face/size, text colour, and background colour is indeed pesent, on the first
(font face/size) remains 'transparent' (or indeterminate) unless specified by
the user; this transparency or indeterminacy should be extended to include text
and background colour -- that is, they should remain unspecified unless they
have been specified.

This might sound a bit niggling, but it would both maintain consistency across
the options with which this bug is concerned and maintain the previously
existing 'transparent', 'indeterminate' behaviour as the default behaviour
unless the user decides elsewise.  Indeed, it would seem that this would be the
only way in which to accommodate both those users who desire the new behaviour
and those who refer the older behaviour.

(To give an example:  With the older behaviour, I could send an HTML message to
someone who preferred light text on a dark background w/o having to worry about
how things would display on the recipent's end; this becomes an issue with the
new behaviour, and I find it a bit troubling that Moz would disrespect someone's
display preferences in this manner. . . .)

Comment 85

[Joel Nelson (don't send email)]

This has still not been implemented in Thunderbird 0.1 (based on the Mozilla 1.5 beta branch).   Is 
there any reason why not?

Comment 86

[u32858]

*** Bug 125633 has been marked as a duplicate of this bug. ***

Comment 87

[Mike Cowperthwaite]

*** Bug 129317 has been marked as a duplicate of this bug. ***