Last Comment Bug 324495 - put signature editing in UI (rather than select a file)
: put signature editing in UI (rather than select a file)
Status: RESOLVED FIXED
[b3ux][m4]
: jp-critical
Product: Thunderbird
Classification: Client Software
Component: Account Manager (show other bugs)
: Trunk
: All All
: P2 enhancement with 18 votes (vote)
: Thunderbird 3.0b3
Assigned To: rsx11m
:
Mentors:
: 325275 426537 428717 480693 (view as bug list)
Depends on: 499558
Blocks: 488469
  Show dependency treegraph
 
Reported: 2006-01-24 00:47 PST by Yvo van Doorn
Modified: 2010-01-08 09:59 PST (History)
35 users (show)
clarkbw: blocking‑thunderbird3+
mkmelin+mozilla: wanted‑thunderbird3+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Outlook 2003 Signature Editor Screenshot (43.20 KB, image/png)
2006-03-30 12:11 PST, Joseph Wright
no flags Details
OS X Mail.App signatures preferences pane... (52.31 KB, image/png)
2006-03-30 14:43 PST, mike castleman
no flags Details
...and OS X Mail.App "edit signature" box (11.02 KB, image/png)
2006-03-30 14:44 PST, mike castleman
no flags Details
Signature UI of Entourage 2004 (297.67 KB, image/png)
2006-03-30 18:00 PST, Yvo van Doorn
no flags Details
allow reading signature from prefs (4.24 KB, patch)
2006-09-10 09:51 PDT, David :Bienvenu
no flags Details | Diff | Review
Adding new signature in Evolution (32.19 KB, image/png)
2007-07-12 06:36 PDT, Rafal Ryba
no flags Details
Microsoft Outlook 2007 - Signatures and Stationery (24.81 KB, image/png)
2007-07-12 06:42 PDT, Rafal Ryba
no flags Details
Mockup for plain textbox in Shredder 3.0 (10.02 KB, image/png)
2009-03-02 19:17 PST, rsx11m
no flags Details
Work-in-progress patch for backend updates (1.63 KB, patch)
2009-03-04 09:33 PST, rsx11m
no flags Details | Diff | Review
Work-in-progress patch for textbox frontend (6.13 KB, patch)
2009-03-04 16:03 PST, rsx11m
no flags Details | Diff | Review
Work-in-progress patch, almost fully functional (11.83 KB, patch)
2009-03-05 11:30 PST, rsx11m
no flags Details | Diff | Review
Proposed patch for review (22.25 KB, patch)
2009-03-07 13:22 PST, rsx11m
no flags Details | Diff | Review
account signature mockup (105.73 KB, image/png)
2009-03-11 13:47 PDT, Bryan Clark (DevTools PM) [@clarkbw]
no flags Details
alternative arrangement with checkbox (8.84 KB, image/png)
2009-03-11 17:58 PDT, rsx11m
no flags Details
Proposed patch (v2) (25.01 KB, patch)
2009-03-13 12:05 PDT, rsx11m
no flags Details | Diff | Review
Proposed patch (v2a) (26.02 KB, patch)
2009-03-14 14:37 PDT, rsx11m
clarkbw: ui‑review+
Details | Diff | Review
Proposed patch (v2b) (27.96 KB, patch)
2009-04-01 13:55 PDT, rsx11m
neil: review+
mozilla: superreview+
rsx11m.pub: ui‑review+
Details | Diff | Review
Proposed patch (v3) (29.74 KB, patch)
2009-04-10 09:21 PDT, rsx11m
neil: review+
rsx11m.pub: superreview+
clarkbw: ui‑review+
Details | Diff | Review
Final patch for checkin (29.73 KB, patch)
2009-04-14 16:32 PDT, rsx11m
no flags Details | Diff | Review

Description Yvo van Doorn 2006-01-24 00:47:25 PST
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.
Comment 1 Joseph Wright 2006-01-24 04:53:26 PST
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.
Comment 2 Mike Cowperthwaite 2006-01-24 08:10:29 PST
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.
Comment 3 Yvo van Doorn 2006-01-24 12:05:31 PST
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. 
Comment 4 Magnus Melin 2006-01-26 09:29:40 PST
Confirming...
Comment 5 Philip Puryear 2006-01-30 16:49:09 PST
*** Bug 325275 has been marked as a duplicate of this bug. ***
Comment 6 Daniel Morante 2006-01-30 21:30:52 PST
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).
Comment 7 Masayuki Nakano [:masayuki] (Mozilla Japan) 2006-03-30 07:19:26 PST
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.
Comment 8 Scott MacGregor 2006-03-30 07:42:13 PST
putting in the 2.0 bucket for consideration per masayuki
Comment 9 Masayuki Nakano [:masayuki] (Mozilla Japan) 2006-03-30 07:57:23 PST
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
Comment 10 Scott MacGregor 2006-03-30 08:16:36 PST
Can anyone post a couple screen shots of the editor for mail.app/outlook/eudora? 
Comment 11 Joseph Wright 2006-03-30 12:11:03 PST
Created attachment 216773 [details]
Outlook 2003 Signature Editor Screenshot

Screen shot of Outlook 2003 built-in editor.  Note that "Advanced Edit" launches Word, and saves the result as an html file.
Comment 12 mike castleman 2006-03-30 14:43:22 PST
Created attachment 216785 [details]
OS X Mail.App signatures preferences pane...
Comment 13 mike castleman 2006-03-30 14:44:14 PST
Created attachment 216787 [details]
...and OS X Mail.App "edit signature" box
Comment 14 Yvo van Doorn 2006-03-30 18:00:16 PST
Created attachment 216807 [details]
Signature UI of Entourage 2004

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.
Comment 15 Daniel Morante 2006-03-31 22:39:29 PST
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.
Comment 16 Peter Lairo 2006-04-06 04:07:51 PDT
This is probably a dupe of bug 73567 comment 14 which is Product=Core has some good ideas.
Comment 17 Masayuki Nakano [:masayuki] (Mozilla Japan) 2006-04-08 10:05:14 PDT
(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.
Comment 18 Kevin Boyd 2006-05-30 09:16:07 PDT
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.
Comment 19 Masayuki Nakano [:masayuki] (Mozilla Japan) 2006-05-30 09:24:08 PDT
Kevin:

That is another bug. That will be fixed by bug 153855.
Comment 20 David :Bienvenu 2006-09-10 09:51:12 PDT
Created attachment 237618 [details] [diff] [review]
allow reading signature from prefs

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.
Comment 21 David :Bienvenu 2007-01-14 09:18:46 PST
pinging about this review request...
Comment 22 David :Bienvenu 2007-05-17 08:26:54 PDT
pinging again...
Comment 23 Scott MacGregor 2007-05-31 18:01:02 PDT
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. 
Comment 24 Kohei Yoshino [:kohei] 2007-06-06 09:34:15 PDT
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.
Comment 25 Kohei Yoshino [:kohei] 2007-06-06 10:34:38 PDT
(In reply to comment #24)
> I'll look into the text widget and make a mockup... until this summer.

s/until/by/ :P
Comment 26 Rafal Ryba 2007-07-12 06:36:49 PDT
Created attachment 272002 [details]
Adding new signature in Evolution
Comment 27 Rafal Ryba 2007-07-12 06:42:53 PDT
Created attachment 272007 [details]
Microsoft Outlook 2007 - Signatures and Stationery

Don't hurt me! It's from trial version! I wouldn't change my lovely Thunderbird for anything else!
Comment 28 Phil Ringnalda (:philor) 2008-04-12 16:02:27 PDT
*** Bug 428717 has been marked as a duplicate of this bug. ***
Comment 29 David :Bienvenu 2008-08-21 11:31:52 PDT
leaving wanted, put in b2 since it involves ui changes.
Comment 30 Wayne Mery (:wsmwk, NI for questions) 2008-09-01 11:26:57 PDT
david do you still want the sr? open for attachment 237618 [details] [diff] [review]?
Comment 31 Nomis101 2008-12-02 08:55:52 PST
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?
Comment 32 Wayne Mery (:wsmwk, NI for questions) 2008-12-19 09:07:10 PST
has patch. needs love.

I think david intended in comment 29 to target this for b2, marking so
Comment 33 Mark Banner (:standard8) 2009-02-28 02:25:41 PST
*** Bug 480693 has been marked as a duplicate of this bug. ***
Comment 34 markusd112 2009-02-28 05:10:12 PST
This feature is a very good thing! It would be great if this feature will be implemented in thunderbird. Thanks!
Comment 35 rsx11m 2009-02-28 08:57:35 PST
(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.
Comment 36 Bryan Clark (DevTools PM) [@clarkbw] 2009-03-02 11:04:35 PST
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.
Comment 37 Michael K 2009-03-02 14:37:02 PST
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?
Comment 38 rsx11m 2009-03-02 15:56:01 PST
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?
Comment 39 neil@parkwaycc.co.uk 2009-03-02 16:02:06 PST
(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.
Comment 40 David Ascher (:davida) 2009-03-02 16:23:05 PST
(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?
Comment 41 Minh Nguyễn 2009-03-02 18:03:35 PST
(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.)
Comment 42 rsx11m 2009-03-02 19:17:06 PST
Created attachment 365113 [details]
Mockup for plain textbox in Shredder 3.0

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.
Comment 43 Hendrik Maryns 2009-03-02 23:43:09 PST
(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.
Comment 44 neil@parkwaycc.co.uk 2009-03-03 01:52:35 PST
(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.
Comment 45 rsx11m 2009-03-03 05:26:39 PST
> 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.
Comment 46 neil@parkwaycc.co.uk 2009-03-03 05:59:27 PST
(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?
Comment 47 rsx11m 2009-03-03 06:08:14 PST
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.
Comment 48 neil@parkwaycc.co.uk 2009-03-03 06:16:48 PST
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...
Comment 49 rsx11m 2009-03-03 06:27:53 PST
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.
Comment 50 neil@parkwaycc.co.uk 2009-03-03 06:39:43 PST
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...
Comment 51 Yvo van Doorn 2009-03-03 06:40:54 PST
(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?
Comment 52 neil@parkwaycc.co.uk 2009-03-03 06:47:01 PST
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.
Comment 53 rsx11m 2009-03-03 07:18:51 PST
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.
Comment 54 rsx11m 2009-03-03 07:41:32 PST
(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.
Comment 55 rsx11m 2009-03-03 08:14:39 PST
(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.
Comment 56 Peter Lairo 2009-03-03 08:39:28 PST
(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)
Comment 57 neil@parkwaycc.co.uk 2009-03-03 08:52:13 PST
(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...
Comment 58 rsx11m 2009-03-03 09:26:19 PST
(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>.
Comment 59 Peter Lairo 2009-03-03 09:35:31 PST
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 60 rsx11m 2009-03-03 19:43:36 PST
(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.
Comment 61 rsx11m 2009-03-04 09:33:23 PST
Created attachment 365456 [details] [diff] [review]
Work-in-progress patch for backend updates

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.
Comment 62 rsx11m 2009-03-04 10:28:23 PST
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.
Comment 63 rsx11m 2009-03-04 16:03:31 PST
Created attachment 365561 [details] [diff] [review]
Work-in-progress patch for textbox frontend

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 64 neil@parkwaycc.co.uk 2009-03-04 16:04:53 PST
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>.
Comment 65 rsx11m 2009-03-04 16:12:10 PST
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.
Comment 66 rsx11m 2009-03-05 11:30:28 PST
Created attachment 365703 [details] [diff] [review]
Work-in-progress patch, almost fully functional

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.
Comment 67 neil@parkwaycc.co.uk 2009-03-05 14:16:40 PST
You could try using get/setUnicharAttribute.

I'm not sure that using inline style for the monospace font is a good idea.
Comment 68 rsx11m 2009-03-05 16:02:04 PST
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).
Comment 69 neil@parkwaycc.co.uk 2009-03-05 16:10:45 PST
(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.
Comment 70 rsx11m 2009-03-07 13:22:14 PST
Created attachment 366129 [details] [diff] [review]
Proposed patch for review

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.
Comment 71 Nomis101 2009-03-07 16:56:17 PST
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
Comment 72 rsx11m 2009-03-07 18:13:10 PST
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 73 rsx11m 2009-03-07 19:58:54 PST
(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.
Comment 74 Nomis101 2009-03-08 05:04:40 PDT
(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
Comment 75 neil@parkwaycc.co.uk 2009-03-08 08:24:21 PDT
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.
Comment 76 rsx11m 2009-03-08 08:54:33 PDT
Spun off as bug 482132.
Comment 77 Nomis101 2009-03-08 09:08:27 PDT
(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 78 Mark Banner (:standard8) 2009-03-08 15:03:08 PDT
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.
Comment 79 Bryan Clark (DevTools PM) [@clarkbw] 2009-03-11 13:47:08 PDT
Created attachment 366890 [details]
account signature mockup

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.
Comment 80 rsx11m 2009-03-11 14:24:11 PDT
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).
Comment 81 rsx11m 2009-03-11 17:58:46 PDT
Created attachment 366958 [details]
alternative arrangement with checkbox

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.
Comment 82 Peter Lairo 2009-03-11 18:26:58 PDT
(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.
Comment 83 rsx11m 2009-03-11 18:35:55 PDT
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.
Comment 84 rsx11m 2009-03-11 18:53:16 PDT
(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.
Comment 85 Bryan Clark (DevTools PM) [@clarkbw] 2009-03-11 19:01:14 PDT
(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.
Comment 86 rsx11m 2009-03-11 19:35:28 PDT
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.
Comment 87 Bryan Clark (DevTools PM) [@clarkbw] 2009-03-11 22:22:56 PDT
i'll target this for b3, this is pretty well defined now and a patch is in the works
Comment 88 Magnus Melin 2009-03-12 00:20:45 PDT
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.
Comment 89 Peter Lairo 2009-03-12 03:04:16 PDT
(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.
Comment 90 rsx11m 2009-03-12 06:47:19 PDT
(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.
Comment 91 rsx11m 2009-03-13 12:05:34 PDT
Created attachment 367265 [details] [diff] [review]
Proposed patch (v2)

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.
Comment 92 rsx11m 2009-03-14 14:37:08 PDT
Created attachment 367428 [details] [diff] [review]
Proposed patch (v2a)

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 93 Gary Kwong [:gkw] [:nth10sd] 2009-03-15 03:31:42 PDT
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 94 Bryan Clark (DevTools PM) [@clarkbw] 2009-03-24 15:26:17 PDT
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.
Comment 95 Joe Sabash [:JoeS1] 2009-03-28 20:23:38 PDT
Pinging for review, and requesting a priority bump and [b3ux] status.
Big impact item IMO for b3
cc: dmose
Comment 96 Gary Kwong [:gkw] [:nth10sd] 2009-03-28 20:27:55 PDT
(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.
Comment 97 rsx11m 2009-03-28 20:43:15 PDT
Neil, any ETA for the technical review of attachment 367428 [details] [diff] [review]?
Comment 98 Dan Mosedale (:dmose) 2009-04-01 13:29:35 PDT
Neil says he's waiting on feedback from bienvenu about comment 69.
Comment 99 David :Bienvenu 2009-04-01 13:41:58 PDT
(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.
Comment 100 rsx11m 2009-04-01 13:55:18 PDT
Created attachment 370496 [details] [diff] [review]
Proposed patch (v2b)

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 101 neil@parkwaycc.co.uk 2009-04-07 07:15:40 PDT
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.
Comment 102 rsx11m 2009-04-07 07:50:23 PDT
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"...
Comment 103 Magnus Melin 2009-04-07 08:00:13 PDT
I'm not sure about that. I have '<' in my non-html sig... I'd say that's pretty common, esp. to surround urls.
Comment 104 rsx11m 2009-04-07 08:19:19 PDT
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.
Comment 105 David :Bienvenu 2009-04-07 08:36:32 PDT
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>
Comment 106 neil@parkwaycc.co.uk 2009-04-07 09:26:49 PDT
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.
Comment 107 rsx11m 2009-04-07 09:35:08 PDT
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.
Comment 108 Magnus Melin 2009-04-07 10:10:14 PDT
David's suggestion of requiring <html> sounds good to me.
Comment 109 rsx11m 2009-04-07 10:32:19 PDT
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...
Comment 110 Phil Ringnalda (:philor) 2009-04-07 11:16:58 PDT
"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?
Comment 111 rsx11m 2009-04-07 11:55:29 PDT
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.
Comment 112 IU 2009-04-07 12:27:06 PDT
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 113 rsx11m 2009-04-07 12:32:19 PDT
Comment #112 is taken care of by the current patch, won't need any change.
Comment 114 neil@parkwaycc.co.uk 2009-04-07 12:47:42 PDT
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);
Comment 115 rsx11m 2009-04-07 14:34:30 PDT
(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 116 David :Bienvenu 2009-04-07 15:50:13 PDT
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...
Comment 117 Bryan Clark (DevTools PM) [@clarkbw] 2009-04-07 17:09:20 PDT
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.
Comment 118 rsx11m 2009-04-07 17:41:39 PDT
(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.
Comment 119 neil@parkwaycc.co.uk 2009-04-08 01:26:34 PDT
(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.
Comment 120 Hendrik Maryns 2009-04-08 05:09:17 PDT
(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.
Comment 121 rsx11m 2009-04-08 06:47:37 PDT
(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.
Comment 122 Magnus Melin 2009-04-08 11:20:26 PDT
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.
Comment 123 Magnus Melin 2009-04-08 11:22:51 PDT
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.
Comment 124 rsx11m 2009-04-08 11:41:37 PDT
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 125 Vlado Valastiak [:wladow] @ Mozilla.sk 2009-04-08 12:04:10 PDT
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.
Comment 126 Magnus Melin 2009-04-09 11:15:50 PDT
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.
Comment 127 rsx11m 2009-04-09 11:29:47 PDT
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.
Comment 128 Bryan Clark (DevTools PM) [@clarkbw] 2009-04-09 15:29:02 PDT
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.
Comment 129 rsx11m 2009-04-09 15:51:32 PDT
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.
Comment 130 David :Bienvenu 2009-04-09 16:19:13 PDT
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?
Comment 131 rsx11m 2009-04-09 16:30:43 PDT
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.
Comment 132 David :Bienvenu 2009-04-09 16:39:37 PDT
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 &.
Comment 133 rsx11m 2009-04-09 16:47:23 PDT
Ok, unless Bryan disagrees, I'll proceed as stated in comment #129.
Comment 134 Joe Sabash [:JoeS1] 2009-04-09 16:59:38 PDT
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)
Comment 135 rsx11m 2009-04-09 17:20:13 PDT
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.
Comment 136 rsx11m 2009-04-10 09:21:53 PDT
Created attachment 372061 [details] [diff] [review]
Proposed patch (v3)

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 137 Bryan Clark (DevTools PM) [@clarkbw] 2009-04-10 10:10:16 PDT
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.
Comment 138 Bryan Clark (DevTools PM) [@clarkbw] 2009-04-13 22:27:42 PDT
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 139 neil@parkwaycc.co.uk 2009-04-14 16:07:03 PDT
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.
Comment 140 neil@parkwaycc.co.uk 2009-04-14 16:08:12 PDT
...the original htmlSigText attribute name was already asking for trouble ;-)
Comment 141 neil@parkwaycc.co.uk 2009-04-14 16:11:10 PDT
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 ;-)
Comment 142 rsx11m 2009-04-14 16:32:31 PDT
Created attachment 372737 [details] [diff] [review]
Final patch for checkin

Thanks for the reviews. This is the v3 patch against current hg tip, with white spaces removed in empty lines per comment #141.
Comment 143 rsx11m 2009-04-14 16:34:51 PDT
Push attachment 372737 [details] [diff] [review] for comm-central please.
Comment 144 Mark Banner (:standard8) 2009-04-15 01:27:46 PDT
Checked in: http://hg.mozilla.org/comm-central/rev/12e923a82a10

rsx11m: Thanks for doing this.
Comment 145 rsx11m 2009-04-15 04:50:42 PDT
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).
Comment 146 Dan Mosedale (:dmose) 2009-04-15 12:37:07 PDT
Thanks for your persistence, rsx11m; great to see this land!
Comment 147 [:Aureliano Buendía] 2009-07-31 05:13:28 PDT
*** Bug 426537 has been marked as a duplicate of this bug. ***
Comment 148 raths 2009-09-02 00:02:02 PDT
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
Comment 149 rsx11m 2009-09-02 04:07:17 PDT
This bug is closed, switching signatures is covered by bug 73567.

Note You need to log in before you can comment on or make changes to this bug.