User-Agent: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8) Gecko/20051111 Firefox/1.5 Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8) Gecko/20051111 Firefox/1.5 Currently signatures have to be attached to the individual accounts. For example email account abc@yahoo.com has to have an external signature file. This could be seen as a potential security threat due to the malicious HTML that could be created in a 3rd party file and the fact that it isn't as handy as creating a signature inside Thunderbird. Reproducible: Always Steps to Reproduce: 1. Tools -> Account Settings 2. Select Account 3. Select "Attach this signature:" 4. Select signature Actual Results: Must create signature outside of Thunderbird Expected Results: Create signature inside Thunderbird like other mail programs (Eudora, Outlook, Mail.app for OS X). This behavior is on all versions of Thunderbird for all platforms.

If this is decided as "a good thing", I hope that external files are not blocked. Personally, I much prefer using Notepad to create plain text files, rather than, for example, the editor in Outlook. An internal-only editor also makes it harder to take a siganture from one PC/platform to another.

I think the "malicious HTML" idea here is a straw man; who uses 3rd-party sigs? If the idea is that some trojan might get in and change the sig, any program smart enough to change the sig for Mozilla is smart enough to change the sig inside the prefs.js file; there's little if any inherent safety from internalizing the sig's text. Bug 219197 is the rfe for the suite to provide internal sig-editing. Doubtless this is actually Core functionality.

You can take the mail.app approach and create mail sig files in the program, thus allowing export yet is easy enough to use in side the program.

Confirming...

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

Would like to have a built in HTML/Plaintext signature editor in the account preferences as opposed to current system which requieres users to go through complex amount of steps to setup an email signature. Reproducible: Always Steps to Reproduce: Goto Account Settings Actual Results: No options to create or edit a signature Expected Results: An option to create or edit a signature. The editor should include the ability to insert an image and do text formating as part of the signature (HTML version).

We need this UI for JP marketing. Because in Japan, the signature is always used by most people.(probably, it's good manner of e-mail in Japan.) Therefor, Japanese local mail clients (these are usually not cost-free) are having this UI.

putting in the 2.0 bucket for consideration per masayuki

Thank you, Scott. But this is very difficult. Kohei Yoshino created the 'signature editor' extension(*1) that is only creating the plain text signature. So, we can easy to implement the same it. But we need HTML editable UI... # And I think that we need also current UI. Because OE can use both UIs. *1 http://mozilla.minutedesign.com/extensions/sigedit/ http://mozilla.minutedesign.com/extensions/sigedit/install/sigedit-1.0.xpi

Can anyone post a couple screen shots of the editor for mail.app/outlook/eudora?

Screen shot of Outlook 2003 built-in editor. Note that "Advanced Edit" launches Word, and saves the result as an html file.

Added screenshot of Entourage signature UI. It is a seperate option under the Tools menu in Entourage, however it ties in VERY nicely with the program. You can create multipe sigs (and random sigs as well) and select a default one for a mail account in the Accounts settings. While Entourage is a slow mail program, it probably has the best signature management I have seen so far.

The signature should be allowed to be controlled via autoconfig. For example, in a large office enviroment a system admin can set everyone up with a standard plain text or HTML (with images) signature. Another good feature (more for sales and marketing people) is to allow the system admin to tag a disclaimer at the end of the signature.

This is probably a dupe of bug 73567 comment 14 which is Product=Core has some good ideas.

(In reply to comment #16) > This is probably a dupe of bug 73567 comment 14 which is Product=Core has some > good ideas. I don't think so. Because many UIs code are dupplicated. So we may need to work twice if we fixes both products.

An internal editor for signatures would help my users very much, because of the "UTF-8 BOM" bug; they only have access to OpenOffice, so in the past they've done a save-as-plain-text to create their signature. With the upgrade to Thunderbird 1.5, almost of these plain text files are now inserting the garbage UTF8 BOM directly into emails, which is causing Thunderbird to excessively badger the users about UTF-8 emailing.

Kevin: That is another bug. That will be fixed by bug 153855.

I don't know if this is really related, but this is code I wrote to allow sigs to be read from a pref. To hookup a UI to it, we'd just need a little text widget that allows text and/or html, and write that to the pref.

pinging about this review request...

pinging again...

Kohei actually had this working with a UI and everything. Kohei, did you ever turn that work into a patch? Maybe we can merge it with the back end work David did here.

Hi Scott, I'm sorry for being late for Tb2 shipping. As Masayuki said in comment 9, my extension doesn't have the capability to edit HTML signature. (because Japanese advanced users never read/write HTML messages.) I'll look into the text widget and make a mockup... until this summer.

(In reply to comment #24) > I'll look into the text widget and make a mockup... until this summer. s/until/by/ :P

Don't hurt me! It's from trial version! I wouldn't change my lovely Thunderbird for anything else!

leaving wanted, put in b2 since it involves ui changes.

david do you still want the sr? open for attachment 237618 [details] [diff] [review]?

Is this a scheduled enhancement for Beta2? I know a lot of TB users who would like to have this in TB3. For example (german forum): http://forum.macsofa.net/viewtopic.php?t=34122 Or is this project fizzled out?

has patch. needs love. I think david intended in comment 29 to target this for b2, marking so

This feature is a very good thing! It would be great if this feature will be implemented in thunderbird. Thanks!

(In reply to comment #30) > david do you still want the sr? open for attachment 237618 [details] [diff] [review]? This patch has been obsoleted by the fix for bug 435587. (In reply to comment #34) > This feature is a very good thing! It would be great if this feature will be > implemented in thunderbird. Thanks! Actually, the backend works already. In your preferences, just add a new string preference "mail.identity.id1.htmlSigText" (or whatever id# you need) and give it a value. It also allows formatting, e.g., "test with<br><i>two</i> lines". So far the backend works in beta 2. In addition to some possible polishing, it may "just" need an HTML-editor box to make this feature available somewhere in the UI for the account settings.

If we aim for a simple text edit box that accepted HTML markup I think we'd have an excellent improvement and I'd be willing to mark this blocking TB3.

ta (In reply to comment #36) > If we aim for a simple text edit box that accepted HTML markup I think we'd > have an excellent improvement and I'd be willing to mark this blocking TB3. FWIW I think it's a "harmless" addition that can only enhance the product. There's prob more urgent "Blockers", but surely this can be tested on a nightly and then added to the next beta, correct?

For comparison, any estimate how much effort it would be to plug in a text field or window using the HTML editor from the libeditor library?

(In reply to comment #38) > For comparison, any estimate how much effort it would be to plug in a text > field or window using the HTML editor from the libeditor library? A textbox is almost no effort at all, since the code to do that already exists within the account manager, so it's basically a bit of XUL and DTD. By comparison the HTML editor would want a full window including menus so that you could apply all the formatting as desired. In SeaMonkey you could alternatively add an "Edit..." button that would simply open a new Editor window for the selected file.

(In reply to comment #39) > By > comparison the HTML editor would want a full window including menus so that you > could apply all the formatting as desired. Couldn't a "cheaper" alternative be to a made-to-be-embedded HTML editor as are found in blog software, wikis, etc?

(In reply to comment #39) > A textbox is almost no effort at all, since the code to do that already exists > within the account manager, so it's basically a bit of XUL and DTD. By > comparison the HTML editor would want a full window including menus so that you > could apply all the formatting as desired. It wouldn't need all the formatting: probably just half the options that appear the Formatting Toolbar, and certainly not everything in the Compose window's menu structure. We don't need to be encouraging the use of TOCs in signatures. :) (In reply to comment #40) > Couldn't a "cheaper" alternative be to a made-to-be-embedded HTML editor as are > found in blog software, wikis, etc? Does Thunderbird ship with Midas support? Most of the editors use Midas. (Dijit.Editor uses contenteditable, and Mozile uses something else.)

Encouraged by comment #39, I've naively added the following to am-main.xul: <hbox align="center" class="indent"> <textbox wsm_persist="true" id="identity.sigtext" flex="1" multiline="true" rows="4" prefstring="mail.identity.%identitykey%.htmlSigText" class="uri-element"/> </hbox> which results in the desired UI element to add a text area for entering a plain signature without formatting tools. So far so good. What didn't work though is that it didn't pick up the preference, neither for initialization nor for writing it back to the preferences. Turns out that "htmlSigText" is only handled in nsMsgCompose.cpp, so that would require additions in (at least) nsIMsgIdentity.idl and am-identity-edit.js to make this work. That's where I've stopped digging further, but this looks certainly feasible as a minimal solution.

(In reply to comment #39) > A textbox is almost no effort at all, Then let me suggest to do this for now, and open a new bug (or change this one) if people do want to be able to use HTML in it. That RFE will probably come once normal users discover the new possibility. One step at a time.

(In reply to comment #42) > <hbox align="center" class="indent"> > <textbox wsm_persist="true" id="identity.sigtext" > flex="1" multiline="true" rows="4" > prefstring="mail.identity.%identitykey%.htmlSigText" > class="uri-element"/> > </hbox> > >which results in the desired UI element to add a text area for entering a plain >signature without formatting tools. So far so good. What didn't work though is >that it didn't pick up the preference, neither for initialization nor for >writing it back to the preferences. I think you need to do two things: a) change the id to identity.htmlSigText (to match the prefstring) and b) add the genericattr="true" attribute. See http://hg.mozilla.org/comm-central/rev/75f806a2fe31 for how I converted some elements to use the widget state manager instead of custom script.

> I think you need to do two things: a) change the id to identity.htmlSigText (to > match the prefstring) and b) add the genericattr="true" attribute. Great, this helped. I had to add (c) preftype="string" too, then it correctly picked up and wrote back the preference. This works already fine when composing in HTML mode. However, I'm running into an issue with plain-text composition, where the '\n' line breaks don't translate into line breaks of the signature. In this case, nsMsgCompose::ProcessSignature() uses ConvertBufToPlainText() to convert the HTML to plain text, which apparently treats it as a white space. Thus, to make this work also for plain-text composition, some conversion of the '\n' to "<br>" would be necessary before using prefSigText.

(In reply to comment #45) > However, I'm running into an issue with plain-text composition, > where the '\n' line breaks don't translate into line breaks of the signature. > In this case, nsMsgCompose::ProcessSignature() uses ConvertBufToPlainText() to > convert the HTML to plain text, which apparently treats it as a white space. In HTML, '\n' is white space... what were you expecting?

Right :-) a couple of prefSigText.ReplaceSubstring() should do the trick, I can work out a complete patch if we proceed as suggested in comment #43 to defer an HTML-edit solution to a follow-up RFE.

Why would you want to mangle the user's HTML? They may want to wrap it for readability, or because it was pasted from somewhere else, or something...

I was thinking of the unsuspecting user who assumes that the text box is just that - a place to enter plain text. Thus, a line break not showing up as a line break may be confusing. The textbox can also wrap long lines. To my surprise, in HTML edit mode, the '\n' in the preference are indeed translated into "<br>" already for the signature, so that needs attention either way to make the behavior consistent for both composition modes.

Ah, I see what's going on here. The HTML data is embedded in a <PRE> block. So of course '\n' isn't white space. But when converting the HTML to plain text, no <PRE> is emitted...

(In reply to comment #43) > (In reply to comment #39) > > A textbox is almost no effort at all, > > Then let me suggest to do this for now, and open a new bug (or change this one) > if people do want to be able to use HTML in it. That RFE will probably come > once normal users discover the new possibility. One step at a time. I think a better approach is the RFE as this is what other mail clients are offering already. Introducing functionality that isn't nearly as complete as what other clients are offering might not be the best way. What are the reasons against using Midas?

HTML files have the same problem too, I think... actually I couldn't get one to work in HTML compose mode at all, but in plain text compose mode the newlines are ignored.

The other option might be to employ some heuristics to distinguish plain-text from HTML signatures. If the signature contains neither '<' nor '>' but '\n', those are translated to "<br>" and treated as hard line breaks. If it appears that HTML tags are present, the signature is used verbatim and newlines are treated as white space. The <pre> wouldn't make sense in the latter case, yes.

(In reply to comment #51) > I think a better approach is the RFE as this is what other mail clients are > offering already. Introducing functionality that isn't nearly as complete as > what other clients are offering might not be the best way. The question is what can be realistically accomplished within the remaining timeframe to finalize the 3.0 features. A textbox solution may still be better for many use cases than providing the hidden preference only. > What are the reasons against using Midas? Has anybody looked into it yet? > (comment #52) HTML files have the same problem too, I think... Just tried that, using an HTML signature with content spread across multiple line, and there where no line breaks in either HTML or plain-text composition. Those are put into <div> rather than <pre>, thus any '\n' shouldn't bother.

(In reply to comment #50) > Ah, I see what's going on here. The HTML data is embedded in a <PRE> block. So > of course '\n' isn't white space. But when converting the HTML to plain text, > no <PRE> is emitted... Guess I figured out what's going on, htmlSig is initialized with PR_FALSE but not updated if prefSigText is present. Thus, in HTML composition, it is treated as a plain-text signature and put into <pre>. In contrast, when composing in plain-text mode, ConvertBufToPlainText() is always called for down-conversion from HTML regardless of the contents of prefSigText. So, that's upside-down. The possible solution would be to determine whether or not prefSigText provides an HTML signature per comment #53, then set htmlSig accordingly. In turn, the plain-text part should only call ConvertBufToPlainText() if htmlSig is set.

(In reply to comment #55) > ... So, that's upside-down. BTW: I wonder if that is related to HTML sigs currently not being stripped on reply. (caveat: it's just a wild guess)

(In reply to comment #55) > Guess I figured out what's going on, htmlSig is initialized with PR_FALSE but > not updated if prefSigText is present. I think it's even funkier than that. If you have both the file and the pref, then the pref is assumed to be HTML and converted to the format of the file, and then the result is converted to the message format. If you have the pref but no file, then the pref is converted to text and then to the message format...

(In reply to comment #56) > BTW: I wonder if that is related to HTML sigs currently not being stripped on > reply. (caveat: it's just a wild guess) I don't think so, the "-- " separator not being recognized when only an HTML part is present was already a bug before the signature pref was introduced. > (In reply to comment #57) If you have both the file and the pref, > then the pref is assumed to be HTML and converted to the format of the file, It's probably a fair assumption that if you specify both (whatever the use cases may be here), the pref should have the same format as the file. > pref but no file, then the pref is converted to text and then to the message > format... Yes and no, tested with mail.identity.id1.htmlSigText="<H1>testing</H1>" and get <pre class="moz-signature" cols="72">-- <br><h1>testing</h1></pre> as the resulting signature, with the formatting retained despite the <pre>.

You guys are busy doing the heavy lifting. Here is a (slightly updated) suggestion from bug 73567 comment 14 for the UI to hopefully help a bit with that part: +-- Signature Preferences ---------------------------+ | | | Select default signature: | | <o> Created in Thunderbird: [ Peter Reaper(txt) \/]| | < > Select from file: [____________________________] [ Choose... ] | <-- Current method | | |---- Manage Signatures -----------------------------| | | | [ ADD... ] | | | | [ Peter Reaper (txt) \/] [ DELETE ] [ EDIT ] | | | | +-- Preview of Selected Signature -------------+ | | | | | | | Sincerely, | | | | Peter Reaper | | | | | | | | | | | +----------------------------------------------+ | +----------------------------------------------------+ Pressing "Add" or "Edit" would bring up this: + -- Add / Edit Signature ---------------------------+ | | | Signature Name [ ] | <-- user definable name | | | ---------------------------------------------- | | < > Text only | | <o> HTML | | ----------------------------------------------- | | Type or Paste your signature below: | | [Variable Width] [Color] [Size] [B] [I] [U] | <-- grayed-out if "Text" | +------------------------------------------------+ | | | | | | | | | | | | | | | | | | | | | | +------------------------------------------------+ | | \\ WYSIWYG Mode // \ HTML Source / | <-- like NVU & Kompozer | [ OK ] [ CANCEL ] | +----------------------------------------------------+ It would be great to be able to edit or paste HTML & CSS code for more complex signatures (like many corporate sigs).

(comment #59) Nice, do you have a patch for this? ;-) I've found the <editor editortype="html" ...> XUL element by now, but was unsuccessful in making it show up in the preference pane, despite following the examples. It is also unclear how this would communicate with the preferences, especially given the unique identity.* setup in the account manager panes. https://developer.mozilla.org/en/XUL/editor The documentation page also pointed me to Midas, which I can run in an HTML page as an iframe (thus, not directly in an XUL environment), but the basic example won't come with any buttons or keyboard shortcuts on its own. There is a more comprehensive demo, also demonstrating that all the decoration would likely require a separate dialog for editing due to its size requirements: https://developer.mozilla.org/En/Midas http://www.mozilla.org/editor/midasdemo/ Looking at this, I don't think I'll go down this road. The backend code needs to be fixed for the HTML/plain-text issues regardless of which editing UI is eventually chosen, and I'll be happy to finalize a textbox patch. Beyond that, someone else would have to take it from there and replace the textbox with an HTML editor, if that's desired.

Neil, can you have a look at this if it covers all relevant cases? As proposed, this treats htmlSigText as an HTML signature if it contains a "<>" somewhere (thus, the signature ends up in a <div> environment), otherwise it is treated as plain text (in a <pre> environment with hard '\n' line breaks). For the case that both a (non-image) file and the pref are specified, htmlSig is determined from the file MIME type and retained for the pref, both contents are simply concatenated in this case. Note that I didn't touch the sequence of determining file MIME type, pref type, and final target composition mode. If you tried to catch all of those upfront, you'd end up with a roughly 24-case switch statement. Tested all cases crossing my mind, no more text/HTML mixups in either modes.

Follow-up: The one case I missed is having an image file with the signature preference producing a second "-- " delimiter between image and signature text in HTML composition. Thus, I'll add the following further down in that function to a next patch which resolves this issue and just leaves a space between both: > nsDependentSubstring firstFourChars(sigData, 0, 4); > > if (!(firstFourChars.EqualsLiteral("-- \n") || > - firstFourChars.EqualsLiteral("-- \r"))) > + firstFourChars.EqualsLiteral("-- \r") || > + (m_composeHTML && imageSig))) > { > sigOutput.AppendLiteral(dashes); The problem is that the image part is encoded separately from the text signature, thus the previously added delimiter is not recognized here.

This is the current code implementing the view in attachment 365113 [details] with a couple of modifications: 1. The textbox doesn't wrap and displays in fixed font, thus matching the <pre> appearance for plain-text signatures (e.g., alignment of ASCII art) and the "code" character for entering HTML signatures. 2. The rather generic label "Attach this signature:" was made more specific "Attach the signature from a text, HTML, or image file:" to indicate which file types can be used. 3. Between the checkbox/file combination and the new textbox, there is a new label "The signature text can also be entered below, allowing HTML code:" to make clear what can be done in this box. I ran into an issue with the Manage Identity dialog though, which doesn't appear to use the preference-mapping method of the Account Settings panes but explicit JavaScript functions to load and write back the pref contents, so this needs some more digging. Consequently, this patch will only present the signature box in the main account pane, not in the identity panes, but that should be sufficient for a discussion of its functionality. Feedback welcome.

Comment on attachment 365456 [details] [diff] [review] Work-in-progress patch for backend updates >+ // set htmlSig if we only have the pref and if it contains any HTML markup >+ if ((!useSigFile || imageSig) >+ && prefSigText.Find (NS_LITERAL_STRING("<")) != kNotFound >+ && prefSigText.Find (NS_LITERAL_STRING(">")) != kNotFound) >+ htmlSig = PR_TRUE; >+ > if (!m_composeHTML) > { >- ConvertBufToPlainText(prefSigText, PR_FALSE); >+ if (htmlSig) ConvertBufToPlainText(prefSigText, PR_FALSE); > sigData.Append(prefSigText); > } > else > { > sigData.Append(prefSigText); The problem here is that if we're composing in HTML then we always assume that the signature pref is in HTML, so a) we can't really choose not to interpret the pref as html and b) we need to turn on htmlSig so we don't get a <pre>.

I see. The other option would be to use something like ConvertTextToHTML() for the (m_composeHTML && !htmlSig) case, which would be more consistent with the ConvertBufToPlainText() treatment of an HTML-recognized signature pref in plain-text composition mode.

This patch fixes the issues raised in comment #62 (two signature separators if an image file is combined with the pref) and comment #64 (a plain-text string in the pref needs to be escaped in HTML composition) in the backend. Also, the Manage Identities > Edit dialog now contains a textbox for entering the signature as well (the remainder of the frontend is the same as in the first patch). It won't be functional though until I have included htmlSigText into the nsIMsgIdentity interface (I'll look into this over the weekend). Other than that, the patch should provide the full functionality intended in this step, if anybody wants to test it.

You could try using get/setUnicharAttribute. I'm not sure that using inline style for the monospace font is a good idea.

I've seen that David's initial patch was using it on the C-side, thus assuming the get/setUnicharAttribute() works equally well in JavaScript, which would of course simplify the patch. On the other hand, it's my impression that extending the IDL interface for new attributes would be "cleaner" in the long run. > style="font-family: -moz-fixed;" I was somewhat uncertain myself if that's a good thing to do in the XUL code, but thinking to provide a default in this way that a theme may override. This could go into accountManage.css as #identity.htmlSigText or some new class definition instead, it just implies to do it for all of the three Thunderbird and two SeaMonkey default themes (and all other themes out there).

(In reply to comment #68) > I've seen that David's initial patch was using it on the C-side, thus assuming > the get/setUnicharAttribute() works equally well in JavaScript, which would of > course simplify the patch. On the other hand, it's my impression that extending > the IDL interface for new attributes would be "cleaner" in the long run. Well, I guess that's a question for the module owner. > > style="font-family: -moz-fixed;" > I was somewhat uncertain myself if that's a good thing to do in the XUL code, > but thinking to provide a default in this way that a theme may override. It does unfortunately require an !important on the theme that wants to override.

This got a bit larger than anticipated, but now fully implements the textbox functionality envisioned in comment #36. Changes since attachment 365703 [details] [diff] [review]: (1) Textbox in Manage Identities dialog is now functional (comment #66). (2) Fixed font is specified as a class "signatureBox" in all default themes rather than inline (comment #67 and comment #69). (3) The "htmlSigText" preference was made an attribute of nsIMsgIdentity to be consistent with the related prefs (comment #68). This also slightly simplifies its access in am-main.xul and nsMsgCompose.cpp. (4) In the backend, there is the possibility that nsEscapeHTML2() may not return a value due to its conservative memory calculation (bug 464998 and related, thanks to Nomis101 for testing). In analogy to ConvertTextToHTML(), the original string is used as a fallback if the function call fails, which should still be better than not returning anything at all. (5) Finally, the option "and place my signature" becomes enabled now in the Composition & Addressing pane (including Manage Identities) when htmlSigText is set but the attachSignature checkbox is not.

Hm sorry, maybe its me, but I've tried the new patch in my build, but I have a similar behaviour than that I've described you at mozillazine. I enter a text in the signature box and compose a new HTML message, but there is no signature in the text field. There is also no signature if I reply to a message. Than I go to "Composition&Addressing" and uncheck "Compose messages in HTML format", to compose the email now in text mode. If I now compose a new message or reply to a message, than I have the signature in the text field. Do I miss something, do I have to enable the signature first? Or does it matter that this is a 3.1a1pre build? The patch was applied correctly (on a fresh peace of code). The only difference to the prior patch is, in the recent patch I can now choose the "and place my signature..." in the "Composition&Addressing" part. In the prior patch this was greyed out. Mac OS X 10.5.6, Intel

I have tested this only on Windows XP and 64-bit Linux with the mozilla-1.9.1 sources, thus I'm not sure what the implications for Mac OSX (don't have one) or against mozilla-central are. Do you see the same issue in the 3.1a1pre OSX nightlies (i.e., before the patch), and what happens when you build it using the mozilla-1.9.1 repository instead?

(comment #71) This appears to be an issue independent of this bug, which is rather a matter of using mozilla-central. The current Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b4pre) Gecko/20090307 Shredder/3.0b3pre nightly works fine with either the pref or a file, whereas Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090307 Shredder/3.1a1pre nightly shows the behavior described and does not add either text nor HTML signature from either pref or even file when in HTML composition mode. It's not obvious to me why there would be a difference between those branches, nsMsgCompose.cpp is the same. If the target build here is 3.0 rather than 3.1, I'd suggest to defer this apparent regression to another bug specific to the 1.9.2 builds.

(In reply to comment #73) > (comment #71) This appears to be an issue independent of this bug, which is > rather a matter of using mozilla-central. The current Mozilla/5.0 (Windows; U; > Windows NT 5.1; en-US; rv:1.9.1b4pre) Gecko/20090307 Shredder/3.0b3pre nightly > works fine with either the pref or a file, whereas Mozilla/5.0 (Windows; U; > Windows NT 5.1; en-US; rv:1.9.2a1pre) Gecko/20090307 Shredder/3.1a1pre nightly > shows the behavior described and does not add either text nor HTML signature > from either pref or even file when in HTML composition mode. It's not obvious > to me why there would be a difference between those branches, nsMsgCompose.cpp > is the same. Oh yes, you are right. If I use mozilla-1.9.1, than your patch works as intended. So its not your patch, its really a regression from mozilla-central. So this shouldn't constrain TB 3.0

Whoa, you mean HTML signatures are broken when building with mozilla-central? Is there a bug filed on that? We don't want to trip over the problem later.

Spun off as bug 482132.

(In reply to comment #76) > Spun off as bug 482132. Oh thanks, I was just also going to open a bug for this. I've added some steps to reproduce to bug 482132.

Comment on attachment 366129 [details] [diff] [review] Proposed patch for review Neil seems to have been involved with this patch/bug quite deeply already, therefore I think it'll be better for him to do this review than me.

this is awesome, i'll mark this blocking in a second. The only change I'd like to see is that we use a radio instead of the checkbox. The results of the state where you've entered some text in the widget and have a file selected seems very unknown. Instead of trying to insert the file if it exists or cat them together I think we can just make this either a file or a text entry. At least for now. Here's a quick mockup of what I mean.

Thanks Bryan. Keep in mind though that you can combine an image file with the signature text, thus it makes sense to a certain extent to have both options available. Making it an either/or choice would disable this variant, and imply some more work in the backend. There is also the option that you don't have any signature at all (in which case currently the box is not checked and the signature box kept empty).

If you want to have file and pref mutually exclusive, how about this solution: The textbox and the file are now switched, thus you see the text input first. Similar to the other fields (Organization, Reply-To, etc.) it is initially empty, thus implying no signature is set. You can enter the text as before, and then this is your signature. The file selector doesn't have any meaning. If you look at the alternative "Attach signature from file" below and check that box, the text box is disabled and in turn the file selector enabled, thus making it unambiguous. The backend would simply ignore the htmlSigText pref and only use the content of the file. Disable the checkbox and the preference text is used again. I'd like this better than the radio buttons, which would ask for a third button "Don't add a signature" and also have some overhead mapping the boolean pref to the radio selectors. Again, this would also exclude the image file + text pref option as a use case then.

(In reply to comment #81) > Created an attachment (id=366958) [details] I think the only logical and OS-consistent way is: | ------------------------------------------------------- | | [x] Attach a signature to my messages | | (o) Signature text (HTML allowed, e.g., <b>bold</b>) | | +-----------------------------------------------+ | | | | | | | | | | | | | | | | | | | | | | +-----------------------------------------------+ | | ( ) Signature from a file (text, HTML, or image) | | [_________________________________] [ Choose... ] | | [x] Attach my vCard to my messages | | ------------------------------------------------------- | Simple, clear, consistent, sufficient.

I'll leave this up to Bryan to decide, personally I would keep it as simple as possible. I definitely like the labels in comment #82 with the HTML example.

(In reply to comment #82) > I think the only logical and OS-consistent way is: Actually, I don't think that this would be more consistent than the layout in comment #81. After all, there is no checkbox in front of the "Reply-To" or "Organization" fields either, leaving them empty implies "don't use" too. Furthermore, without adding more preferences, comment #82 would imply quite a bit of logic to map this layout to the existing set of preferences.

(In reply to comment #81) > Created an attachment (id=366958) [details] > alternative arrangement with checkbox > > If you want to have file and pref mutually exclusive, how about this solution: This looks really good, nice work and I would use Peter's labels; I like that the added examples make it clear what is possible.

Ok, I'll work out a patch according to attachment 366958 [details] then. Renominating for blocking-thunderbird3 per comment #79. Neil, David: If you have any comments already, please let me know.

i'll target this for b3, this is pretty well defined now and a patch is in the works

So the text is grayed out even if you wrote something there and checked the "Attach the sig..."? The "HTML is allowed" is way geeky IMO. If you need it, can't it be in the instruction text? ("This is the signature...") Also, if you compose in plain text, the only thing that looks any good (works?) is a text file. So I'm not sure specifying the file types you "can" choose helps.

(In reply to comment #88) > So the text is grayed out even if you wrote something there and checked the > "Attach the sig..."? This will confuse "normal" users (who don't want to sit there analyzing what the "logic" might be). BTW: It is not like the "Reply-To" field, because that field doesn't "cancel out" another field. And that is why I still think the UI in comment #82 is more clear. > The "HTML is allowed" is way geeky IMO. If you need it, can't it be in the > instruction text? ("This is the signature...") I think it's important to know what's possible (e.g., So many bog's comment fields don't tell you what is and isn't allowed, and I have often spent a lot of time formatting my comment, only to discover that some/all the code I used didn't work. Conversely, I have *not* formatted a comment, only to find out that I could have.) > Also, if you compose in plain text, the only thing that looks any good (works?) > is a text file. So I'm not sure specifying the file types you "can" choose > helps. I Disagree. Many companies (and some private citizens) use *HTML* sigs because they are an important part of their corporate identities. Alternate suggestion for some of comment #82: | [x] Add a signature to my messages (e.g., Sincerely,...) | <-- "Add" & e.g.

(In reply to comment #88) > So the text is grayed out even if you wrote something there and checked the > "Attach the sig..."? Yes, and the file box actually behaves in this way as well. If you check the box, you can pick a file to be the signature. If you uncheck the box, the file name remains, but no signature is attached. So this would be fully consistent. While I don't see any ambiguity, the label could state this more explicitly: [ ] Attach the signature from a file instead (...) > The "HTML is allowed" is way geeky IMO. If you need it, can't it be in the > instruction text? ("This is the signature...") The current label "Attach this signature:" is not very descriptive, and many users are coming to the forums asking what they are supposed to do here. Thus, giving more guidance how to create a signature (including pointing out how to use basic HTML markup and which file types can be used) would be helpful. Now, for SeaMonkey the "geeky stuff" can go into the Help instructions. There is however bug 253334, consequently this cannot be done in Thunderbird at this time and such instructions have to be part of the label.

This is a revised version, reflecting the layout of attachment 366958 [details]. (1) Text box and file selector are now switched and are mutually exclusive. (2) Checking the box to attach a file disables the signature text box. (3) Labels have been changed similar to comment #82, updated by comment #90. (4) SeaMonkey help file is updated/extended, providing more details on usage. (5) Backend in nsMsgCompose.cpp could be simplified, cases that considered both file and preference have been removed.

Apologies for this quick follow-up patch, but I've noticed that the fix to ensure proper new-line endings in plain-text signatures (bug 428040) was done only for signatures read from file but not for those set by preference. Thus, (6) in nsMsgCompose.cpp, move plain-text signature post-processing from the signature-file handling after the preference text was inserted. Everything else is unchanged. Also, image signature file handling is not affected as this case would be only relevant in HTML composition.

Comment on attachment 237618 [details] [diff] [review] allow reading signature from prefs This old patch has bitrotted. $ patch --dry-run < ~/Desktop/tbTestPatches/324495.diff patching file nsMsgCompose.cpp Hunk #1 FAILED at 3604. Hunk #2 FAILED at 3662. Hunk #3 FAILED at 3725. 3 out of 3 hunks FAILED -- saving rejects to file nsMsgCompose.cpp.rej

Comment on attachment 367428 [details] [diff] [review] Proposed patch (v2a) I haven't applied this latest version but I think we're pretty clear on the idea.

Pinging for review, and requesting a priority bump and [b3ux] status. Big impact item IMO for b3 cc: dmose

(In reply to comment #95) > Pinging for review, and requesting a priority bump and [b3ux] status. > Big impact item IMO for b3 > cc: dmose Please do not change the priority unless one is a release driver.

Neil, any ETA for the technical review of attachment 367428 [details] [diff] [review]?

Neil says he's waiting on feedback from bienvenu about comment 69.

(In reply to comment #69) > (In reply to comment #68) > > I've seen that David's initial patch was using it on the C-side, thus assuming > > the get/setUnicharAttribute() works equally well in JavaScript, which would of > > course simplify the patch. On the other hand, it's my impression that extending > > the IDL interface for new attributes would be "cleaner" in the long run. > Well, I guess that's a question for the module owner. ah, sorry, didn't realize you were waiting for me - having it in the idl is cleaner, but I wouldn't let that get in the way of making this work.

Thanks for the feedback, it appeared natural to me to extend the IDL interface for htmlSigText, as it would be the only such pref without a corresponding attribute otherwise. This is how it is currently implemented in the patch. Also, I've noticed that the patch needs some minor changes to nsMsgCompose:: ProcessSignature(), thus updating as follows so that it still works. (7) removed bitrot caused by the patch checked in for bug 194788 today, (8) adjusted introductory comments to reflect what is actually done. Carrying forward ui-review+ from v2a patch. Everything else is unchanged.

Comment on attachment 370496 [details] [diff] [review] Proposed patch (v2b) >+<!ENTITY signatureText.label "Signature text (HTML is allowed, e.g., &lt;b&gt;bold&lt;/b&gt;):"> Hah, this confused me, because I thought "hey, you don't check for &s" ;-) While I'm on the subject, I'm not 100% convinced by the HTML detection. I'm tempted to suggest that either < or & should trigger HTML.

Re detection: I figured that '<' and '>' would be sufficient indication for an HTML signature. The problem with '&' would be that - especially in commercial signatures - you may see it rather frequently in a company name, thus would force the user to specify "&amp;" instead even though no other HTML is needed. Should be easy to learn that '<' and '>' means "use HTML"...

I'm not sure about that. I have '<' in my non-html sig... I'd say that's pretty common, esp. to surround urls.

With only the pref's content available to distinguish HTML from plain-text signature, I don't see a way how to easily distinguish these cases. The label at least should give a hint that a "<http://...>" would trigger HTML mode.

I don't think < > is sufficient for deciding a sig is html. What if we required them to put an html tag in the sig, i.e., <html>

If you think it would help, I'll give you review on everything except the html/plaintext guessing code, and you can do that in a followup bug.

It sure would help to get some feedback if anything else has to be done with the remaining code. If David is ok with deferring the HTML detection to a follow-up bug, this issue can certainly be separated from my point of view, or we can try to work out a solution here if it is feasible with limited effort.

David's suggestion of requiring <html> sounds good to me.

Ok, let's work towards limiting the "<>" which trigger HTML mode. Having to put "<html>" explicitly into the signature textbox may be a bit obscure though, how about defining a small positive list of tags to match the pref against? I'd think of <html>, <br>, <a href>, <b>/<i>/<u> (<small>/<big>/<font>/<div>?) Anything else? Or less than that? Don't want to overdo it...

"I'm <b>bold</b>" is HTML, "I'm <b>" is text, "All about &aring;" is impossible to say without asking the user about their intentions. Did I miss the reason why we don't want a radiobutton to let the user _tell_ us which it is?

The motivation was not to have yet another preference for this, but a checkbox "[x] Use HTML code" should certainly resolve any ambiguity. It appears we have three possible variations: (1) Require a strict "<html>" tag to indicate HTML encoding; (2) guess from a positive list of tags whether it is HTML code; (3) add an explicit checkbox and pref to let the user switch. All versions would work for me, thus I'd appreciate feedback from the reviewers on this. Keep in mind that only the textbox has the ambiguity, handling of file signatures is not affected in any way and remains the same.

Don't forget: regardless of whether the signature is detected/entered as HTML or text, an HTML signature needs to be properly applied as text to a text-only email, ignoring the HTML formatting tags.

Comment #112 is taken care of by the current patch, won't need any change.

Comment on attachment 370496 [details] [diff] [review] Proposed patch (v2b) >+ <textbox id="identity.htmlSigText" flex="1" multiline="true" wrap="off" rows="4" class="signatureBox"/> [Another text/html gotcha: html, but not plain text, should be forced LTR?] >+ // set htmlSig if the pref contains any HTML markup I don't like the <html> idea, since we're not building a web page here, just a document fragment. r=me given that we have something better than the current detection (presumably either no detection or a separate radio or checkbox). >+ if (htmlSig) ConvertBufToPlainText(prefSigText, PR_FALSE); Nit: should be on two lines i.e. if (htmlSig) ConvertBufToPlainText(prefSigText, PR_FALSE);

(In reply to comment #114) > [Another text/html gotcha: html, but not plain text, should be forced LTR?] My first design actually was using LTR for the textbox, but then - as you also mention below - that has more the nature of "text" than webpage "code" for that purpose, even though you may throw in HTML tags. Thus, keeping this box in the direction of the language appears to be a feasible choice. > I don't like the <html> idea, since we're not building a web page here, just a > document fragment. r=me given that we have something better than the current > detection (presumably either no detection or a separate radio or checkbox). Since we have both plain-text and HTML signatures as use cases, I wouldn't like enforcing any of those for the textbox. Thus, either improving the detection method or giving the user the explicit choice should be better. I've tested a bit for version (2) in terms of a positive tag list, where the following seems to do the necessary job of identifying such tags: > const char* triggerTags[] = { "<html>", "<br>", "</a>", "</b>", "</i>", "</u>", > "</big>", "</small>", "</font>", "</div>" }; > nsString prefSigLower; > PRUint32 tagIndex; > > ToLowerCase(prefSigText, prefSigLower); > > // set htmlSig if the pref contains any recognized HTML tags > for (tagIndex=0; !htmlSig && tagIndex<NS_ARRAY_LENGTH(triggerTags); tagIndex++) > if (prefSigLower.Find(triggerTags[tagIndex]) != kNotFound) > htmlSig = PR_TRUE; This uses the closing </xxx> for some of the tags to avoid the issue of any optional attributes in the opening tags and to reduce an accidental match with "real" text. Still, this wouldn't satisfy comment #110, but be a significant improvement over the current patch. Option (3) with an explicit "Use HTML" checkbox would require a bit more work, but should be straight-forward to implement. Bryan, any preference for either option in terms of user experience?

Comment on attachment 370496 [details] [diff] [review] Proposed patch (v2b) sr=me, modulo the html <> detection code. I'll defer to clarkbw about whether or not the user should just say they want html...

I'm trying to reread this in order to understand. What does a plaintext signature mean in an HTML composition? i.e. if I have HTML as my default for composing emails what would a plaintext sig do? Similarly, what would an HTML sig end up like in a plaintext composition? I'm kind of wondering if we should just be choosing HTML vs. plain off that pref... which is per account. I have to run, but I'll check in later today and tomorrow.

(In reply to comment #117) > I'm trying to reread this in order to understand. What does a plaintext > signature mean in an HTML composition? Combining different composition and signature modes works, most likely you'd see HTML composition with just a plain-text signature unless the user wants to format the signature in a specific way; the signature is shown "as is" in a <pre> environment. HTML signatures in plain-text composition are downconverted to plain text, thus removing any formatting. > I'm kind of wondering if we should just be choosing HTML vs. plain off that > pref... which is per account. Don't think that's a good idea. One can change the composition mode from message to message, and as said HTML composition going along with a plain-text signature may be a frequently encountered combination. If you tie it to the default composition mode, which by default is HTML, you would /require/ the user to also use HTML code in the signature field, including trivial things like escaping '&' and writing "<br>" to get a line break. To summarize, the idea of the current patch and option (2) is to auto-detect HTML tags, thus assuming that if the user types in "<b>Hi there</b>" he or she knows that this implies HTML formatting, whereas the unsuspecting user may just type in a plain text which doesn't match any of the trigger tags. In contrast, option (3) requires the user to check an explicit box to enable HTML, which would be considered HTML even if plain text was entered.

(In reply to comment #118) > (In reply to comment #117) > > I'm kind of wondering if we should just be choosing HTML vs. plain off that > > pref... which is per account. > Don't think that's a good idea. One can change the composition mode from > message to message, and as said HTML composition going along with a plain-text > signature may be a frequently encountered combination. Then there's the other alternative of having separate signatures for HTML and plain-text composition, but I guess that just makes transitioning to a full signature editor that much harder.

(In reply to comment #111) > The motivation was not to have yet another preference for this, but a checkbox > "[x] Use HTML code" should certainly resolve any ambiguity. > > It appears we have three possible variations: > (1) Require a strict "<html>" tag to indicate HTML encoding; > (2) guess from a positive list of tags whether it is HTML code; > (3) add an explicit checkbox and pref to let the user switch. I’d plead for (3). My experience is that detection magic is just confusing. Make things explicit. It is also cleaner if you want to change it later.

(In reply to comment #119) > Then there's the other alternative of having separate signatures for HTML and > plain-text composition, but I guess that just makes transitioning to a full > signature editor that much harder. Yes, let's keep it with a single signature for both cases please. You may compose in HTML but end up sending in plain text anyway, depending on what auto-detect decides to send it in. Thus, the actual format depends on several variables and not just the format of the signature itself.

Or just require it to be just plain text as long as you're using a plain text editor? People who can write in html are surely capable of creating an html file too.

I mean, requiring all people to enter <br> for line breaks seems overdone when the most people aren't really going to put a lot of html there.

This discussion is getting somewhat confusing. The backend code takes care of any conversion from plain text to HTML and HTML to plain text, depending on the individual circumstances. It would be good to allow some simple formatting in the box to avoid having to go with the file option for just a bold or italic. The only question is whether to guess the intention of the user from what was entered into the signature box or making it an explicit checkbox option, everything else should be taken care of already. > I mean, requiring all people to enter <br> for line breaks seems overdone when > the most people aren't really going to put a lot of html there. That wouldn't be the case. If the input is recognized as plain text, then any '\n' is correctly converted to "<br>" for HTML composition. You only need the "<br>" when entering HTML, either due to triggering tags (2) or if checked (3).

Comment on attachment 370496 [details] [diff] [review] Proposed patch (v2b) >diff -r 233c1c6f6853 mail/locales/en-US/chrome/messenger/am-main.dtd >+<!ENTITY signatureText.label "Signature text (HTML is allowed, e.g., &lt;b&gt;bold&lt;/b&gt;):"> >+<!ENTITY signatureText.accesskey "g"> Ehm, in terms of readability this is the worst character you can use as an accesskey. F.e. 'x' seems to be available for use instead.

Of course all the problem arise from the fact that try to use a plain text editor for something it's not supposed to do. Put a real editor there and all the problems are gone.

Making this a full HTML editor was deferred to a follow-up bug during the discussion here, so for now we go with a textbox. Certainly important though is that the solution implemented here has to go along with an extension of a full editor later (e.g., introducing a checkbox with pref would only make sense if the full editor is intended to give a choice between plain text and HTML too). If it is considered too confusing for a user to enter HTML tags, we could at least provide David's suggestion of an "<html>" tag to enable that mode as an easter-egg for the experienced user.

ok, I guess we should add a checkbox for HTML sigs. Part of our goal here was to get something simple done and I don't want to lose that. Thanks for pushing through this rsx11m. I don't think the "[ ] Use plain text only" option will be too bad, just not something my mom will put on the frig. I'd like the checkbox label to be aimed toward turning on "plain text" mode, not turning off HTML. People who want plain text tend to know what that option means, everyone else should get HTML behavior by default. I used "[ ] Use plain text only" as a possible phrase, suggestions welcome. We might want to add "(no HTML allowed)" to that.

Bryan, thanks for the verdict, so option (3) it is. > People who want plain text tend to know what that option > means, everyone else should get HTML behavior by default. Thus, your idea is to be in HTML mode by default and only use plain text if it is explicitly selected? I'd see it rather the opposite way, assume that a user prefers plain text and switch on HTML if needed and consequences are known. Note that HTML by default without any auto-detect as in option (2), the problem stated in comment #123 would apply that the user /has/ to enter HTML. I still think a positive-logic "[ ] Use HTML (e.g., <b>bold</b>)" is better. If someone doesn't understand the option, plain text is a safe default.

I agree with rsx11m in the sense that most users won't know how to write html, but then, if you don't put any tags in your sig, it doesn't matter, does it? So Bryan's point is that it's only going to be a select few who want to put tags in plain text, right?

It does matter, unless you combine (2) and (3), meaning auto-detection of certain tags with the ability to switch of the auto-detect and enforce a plain-text signature. Example: John & Jane Doe, Ltd. <lots of nice things> would work as written here with HTML switched off for (3) and in option (2). In option (3), having HTML checked by default, you have to enter instead: John &amp; Jane Doe, Ltd.<br> &lt;lots of nice things&gt; which I think was the point Magnus was making in comment #123. If we have (3) with plain-text default and someone wants to emphasize "nice", the "<i>nice</i>" string would only work with "[x] Use HTML" is checked, and if the user got that far, he or she would hopefully realize that escaping HTML relevant characters and newlines has to be done as well.

ah, right, the entity issue. To me, that means we have to default to plain text - normal users aren't going to understand that they need to type &amp instead of &.

Ok, unless Bryan disagrees, I'll proceed as stated in comment #129.

What about option 4. Embed "Dreamweaver" in the sig composition window :) Seriously, we are making this too complex, anyone with any html skills will recognize the tag clues in the original example: Enter signature..(HTML allowed e.g. <b>bold</b>) The sig itself is going to be in the mode that TB decides the composition to be sent in. (But that's another bug)

Too late, I've already added the new pref to the IDL definitions. ;-) > anyone with any html skills will recognize the tag clues ... Yes, but those users without won't have a clue what's going on, thus I understand the arguments for giving an explicit choice here. David, judging from comment #116 and comment #132 I assume that you don't want another sr? for a new patch. I would ask Bryan and Neil though to review the updates for the checkbox in appearance and backend.

This patch changes the signature label from before Signature text (HTML is allowed, e.g., <b>bold</b>): to the following with the HTML checkbox added Signature text: [ ] Use HTML (e.g., <b>bold</b>) Note that the checkbox is aligned with the grid of the other items to get a consistent appearance. It looked a bit odd to either have it directly after the "Signature text:" label or all the way to the right. In nsMsgCompose::ProcessSignature(), the guessing code for "htmlSig" is now replaced by directly determining it from the boolean htmlSigFormat preference. This also includes the change in access key recommended in comment #125, which is now 'x' for the signature textbox and 'L' for the HTML checkbox. Carrying forward sr+ from v2b.

Comment on attachment 372061 [details] [diff] [review] Proposed patch (v3) ah, right that makes sense then. This looks good. Thanks a lot for all the hard work.

m4 is tomorrow at midnight PDT time but this already has most of the review it needs so I'm aiming it for that.

Comment on attachment 372061 [details] [diff] [review] Proposed patch (v3) The confusing names of the attributes concern me but I guess those can easily be tweaked to something less confusing later.

...the original htmlSigText attribute name was already asking for trouble ;-)

Oh, I forgot to mention that there were a couple of blank lines that the patch added that weren't completely blank, because they contained blanks ;-)

Thanks for the reviews. This is the v3 patch against current hg tip, with white spaces removed in empty lines per comment #141.

Push attachment 372737 [details] [diff] [review] for comm-central please.

Checked in: http://hg.mozilla.org/comm-central/rev/12e923a82a10 rsx11m: Thanks for doing this.

Good, thus we are done here. As discussed, I've opened follow-up bug 488469 on adding HTML-edit capabilities (couldn't transfer the votes though).

Thanks for your persistence, rsx11m; great to see this land!

I think that something like outlook 2007 would be great ! You can edit your signature and switch with another one very simpily ! Another thing is great : we can choose where to put it

This bug is closed, switching signatures is covered by bug 73567.