762413 - Signature placed above quote in plain text, entire message body is considered signature

Status: RESOLVED FIXED

(MailNews Core :: Composition, defect)

Description

[toni]

User Agent: Mozilla/5.0 (Windows NT 6.1; rv:12.0) Gecko/20100101 Firefox/12.0
Build ID: 20120420145725

Comment 1

[John Veness]

Attachment: Screenshot showing grey text problem

I'm also seeing this in Thunderbird 13 on Windows XP. I don't recall seeing it in Thunderbird 12.

To be clear, this is when composing an email, and only occurs if composing as plain text, not as HTML.

Comment 2

[John Veness]

I have just noticed a possible data loss situation related to this bug, if you have multiple accounts in Thunderbird, or multiple Identities.

If you compose an email while in the state described in this bug with the body text in grey, then change the From address before sending, Thunderbird will wipe out all the grey body text. (Presumably it does this because it thinks it is part of the signature, and different accounts/identities can have different signatures.) This would be particularly destructive if the user changes the From address after composing a long email. Many users would not know how to recover from this situation, although luckily Ctrl+Z does bring back the text.

By the way, I expect the Component for this bug should be Message Compose Window, not Account Manager.

Comment 3

[rsx11m]

I don't see this on current trunk builds. There have been major changes in plain-text composition introduced with Thunderbird 13.0 by the new "Filelink" feature (bug 698925), thus adding some people for whom this may ring a bell.

Did you test without add-ons activated? Help > Restart with Add-ons Disabled
If you have signature-related extensions installed they may no longer work properly after those changes.

Comment 4

[rsx11m]

Ok, you need to set "reply above the quote" and "signature below my reply". Apparently the entire message is considered signature, even the part above dash-dash-space signature separator.

Also note that with this combination, there is no longer a blank line at the beginning of the composition, thus one has to add a new line manually which may separate the signature separator from the related HTML tag.

I am looking at a 20120603 15.0a1 build on Windows 7.

Comment 5

[Mike Conley (:mconley) (:⚙️)]

(In reply to rsx11m from comment #4)
> Ok, you need to set "reply above the quote" and "signature below my reply".
> Apparently the entire message is considered signature, even the part above
> dash-dash-space signature separator.
> 
> Also note that with this combination, there is no longer a blank line at the
> beginning of the composition, thus one has to add a new line manually which
> may separate the signature separator from the related HTML tag.
> 
> I am looking at a 20120603 15.0a1 build on Windows 7.

I've set reply above the quote, and signature below the reply, and I'm not seeing this regression...

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:14.0) Gecko/20120604 Thunderbird/14.0a2

Does anybody have a minimal set of steps to reproduce?

Comment 6

[rsx11m]

Mike: This may be related to what you mentioned in bug 756500 comment #9, I failed to notice that the fix for this was backed out again on trunk.

Minimum steps to reproduce:
1. Default to plain-text composition
2. Reply above the quote
3. Signature below my reply
4. Click "Write", no empty line before signature
5. Place cursor before signature seperator
6. Introduce manually a new line
7. Go back to the first line and enter text
8. New text typed appears gray as if it was signature

To reproduce comment #2, set up a 2nd identity with identical options.

Comment 7

[John Veness]

(In reply to rsx11m from comment #3)
> Did you test without add-ons activated? Help > Restart with Add-ons Disabled

I still observe this after restarting without add-ons activated.

(In reply to Mike Conley (:mconley) from comment #5)
> Does anybody have a minimal set of steps to reproduce?

In TB 13, from a brand new TB profile:

1, Instead of gandi.net or hover.com, "Skip this and use my existing email"
2, Setup access to email (in my case, IMAP to a MS Exchange server, but I don't think this matters)
3, Tools, Account Settings
4, Fill in anything in the Signature box, e.g. "test"
5, Composition & Addressing:
	a, Untick "Compose messages in HTML format"
	b, Tick "Automatically quote the original message when replying"
	c, Choose "start my reply above the quote"
	d, and place my signature "below my reply (above the quote)"
6, Click OK
7, Click Write
8, Tab into the message body and observe the problem

It seems odd to me that the settings about where to put the signature when replying are relevant to this, as the problem is observed when composing a brand new message, not a reply.

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Ah, yes, reproduced. Thank you.

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Neil:

I have a feeling this affects SeaMonkey as well. Can you confirm?

-Mike

Comment 10

[rsx11m]

Yes, it's core code thus should affect SeaMonkey as well. However, there is no special color for signatures during composition, hence one wouldn't notice.

Comment 11

[Mike Conley (:mconley) (:⚙️)]

Alright, I'll take a crack at this this afternoon.

Comment 12

[rsx11m]

(In reply to rsx11m from comment #4)
> Also note that with this combination, there is no longer a blank line at the
> beginning of the composition, thus one has to add a new line manually which
> may separate the signature separator from the related HTML tag.

I can reproduce this part on a recent SeaMonkey 2.11 build, thus MailNews Core.

Comment 13

[rsx11m]

That's what I see with DOM Inspector in the composition window:

BODY
  DIV class=moz-signature
    #text (message text)
    BR
    #text "-- " (signature separator)
    BR
    #text (signature text)
  BR
  BR (13.0 build only)

Comment 14

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1

This patch seems to fix the issue for me. Can one of your fine folks try it out for me while I work on some tests?

Comment 15

[rsx11m]

This should resolve the issue of the missing blank line with the signature above the quote setting, but I'm actually able to squeeze a #text node between the <div> and the signature separator in any mode (i.e., also when the signature is place at the bottom) by just placing the cursor before the "-- " and starting to type, then creating a new line. This also works in HTML mode, the text entered goes into the <pre class=moz-signature> part before the "-- " at least with a plain-text signature.

I'm wondering if there is any way to prevent adding anything between the <div>/<pre> opening tag and the #text node for the signature separator, but this appears to be tricky as the "-- " effectively would have to be part of the <div>/<pre> construct somehow or otherwise would have to be ensured to always be the first child of that node.

> else if (htmlEditor)

So, this doesn't apply to the plain-text editor or is plain-text editor now the HTML editor just being restricted to accept plain-text activity only? Apparently I'm confusing this with the aHTMLEditor parameter passed into nsMsgCompose::ConvertAndLoadComposeWindow()...

Given that the observed behavior of being able to squeeze something as a first child to the <div>/<pre> node existed before already, even though just restricted to HTML composition, ensuring that there is an empty line at the begin of the document where the user should start typing is probably sufficient to avoid running into this issue, and the patch seems to do that.

Comment 16

[Mike Conley (:mconley) (:⚙️)]

(In reply to rsx11m from comment #15)
 
> > else if (htmlEditor)
> 
> So, this doesn't apply to the plain-text editor or is plain-text editor now
> the HTML editor just being restricted to accept plain-text activity only?
> Apparently I'm confusing this with the aHTMLEditor parameter passed into
> nsMsgCompose::ConvertAndLoadComposeWindow()...

It's a bit strange. We can query the editor to be both an HTML editor and a text editor, but aHTMLEditor is a boolean for whether or not we should be *considered* a HTML editor. Bleh.

> 
> Given that the observed behavior of being able to squeeze something as a
> first child to the <div>/<pre> node existed before already, even though just
> restricted to HTML composition, ensuring that there is an empty line at the
> begin of the document where the user should start typing is probably
> sufficient to avoid running into this issue, and the patch seems to do that.

Cool, thanks - still writing tests...

Comment 17

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v2 with regression test

Added a regression test, and hardened one of our other tests to check for the initial BR node under default compose circumstances.

Try builds are coming in here: https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=58dc6e74c993

Comment 18

[David :Bienvenu]

ugh, apologies, sorry for the delay, mozmill tests are crashing for me, unrelated to your change.

Comment 19

[David :Bienvenu]

Comment on attachment 631390 [details] [diff] [review]
Patch v2 with regression test

r+ based on try build mozmill successes...

Comment 20

[Joe Sabash [:JoeS1]]

Don't want to muddy the waters on this one.
But in the case of pop3 reply.
5, Composition & Addressing:
	a, Untick "Compose messages in HTML format"
	b, Tick "Automatically quote the original message when replying"
	c, Choose "start my reply above the quote"
	d, and place my signature "below my reply (above the quote)"
The -- delimiter is not included in the sig

xref bug https://bugzilla.mozilla.org/show_bug.cgi?id=62429#c296
I don't recall that being specifically changed.

Comment 21

[Joe Sabash [:JoeS1]]

Sorry about the extraneous comment above.
I somehow thought this was about replying, not original composition.

Comment 22

[rsx11m]

The issue depends on the reply settings even if observed with a new message. And, no "-- " present when putting the signature above the quote is expected behavior for replies.

I get two empty lines on top of the signature, though: The first one (with the cursor) is part of the text, the second line (below the cursor) already within the <div>, thus a similar behavior /could/ be observed when typing anything into the 2nd line. However, either case requires some active interaction by the user to be encountered, and (at least on Thunderbird) there is user feedback given by graying out any text that accidentally makes it into the <div> for moz-signature, thus I'd consider it not perfect but manageable.

Comment 23

[rsx11m]

Mike, is this ready to land on comm-central? It would be nice to get this into a 13.0.1 (if there is one) given that the scenario of top-poster/plain-text user is likely not an isolated one.

Comment 24

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 631390 [details] [diff] [review]
Patch v2 with regression test

I don't think this is too risky to take for aurora or beta, but I'll let the powers that be decide.

Comment 25

[Mike Conley (:mconley) (:⚙️)]

comm-central: https://hg.mozilla.org/comm-central/rev/7447698d6473

Comment 26

[David :Bienvenu]

Comment on attachment 631390 [details] [diff] [review]
Patch v2 with regression test

sure, we've got several weeks to catch any regressions.

Comment 27

[Mike Conley (:mconley) (:⚙️)]

comm-aurora: https://hg.mozilla.org/releases/comm-aurora/rev/7b9e46c36e2f
comm-beta: https://hg.mozilla.org/releases/comm-beta/rev/97daafffb827

If we roll a 13.0.1, do we want to take this?

Comment 28

[David :Bienvenu]

I think it would be OK to take this for 13.0.1 as a ride-along.

Comment 29

[Mark Banner (:standard8)]

Comment on attachment 631390 [details] [diff] [review]
Patch v2 with regression test

[Triage Comment]
I think we should be safe enough to take this for a 13.0.1 as well. As the risk area is a small set of use cases.

Comment 30

[Mark Banner (:standard8)]

Fixed in 13.0.1:

https://hg.mozilla.org/releases/comm-release/rev/1085f63aa2f3

Comment 31

[John Veness]

Although the bug as described in comment #7 is now fixed in TB 13.0.1, there are some variations of this bug still present, at least in TB 13.0.1 on Windows. I wasn't sure whether to file these as new bugs or not, but I have decided to describe the new variants in this bug as they are so similar to the original bug as described in comment #7. I think they are probably from the same original problem which was introduced into TB 13 as I don't recall seeing them prior to 13 or 13.0.1 (although I don't have an easy way to check earlier versions). If instead these should be reported as new bugs, let me know.

Steps to reproduce variant number 1:

In TB 13.0.1, from a brand new TB profile:

1, Click "Skip this and use my existing email" and setup access to email (in my case, IMAP to a MS Exchange server, but I don't think this matters)
2, Tools, Account Settings
3, Important: Leave the signature box blank
4, Composition & Addressing:
	a, Untick "Compose messages in HTML format"
	b, Tick "Automatically quote the original message when replying"
	c, Choose "start my reply above the quote"
	d, and place my signature "below my reply (above the quote)"
5, Click OK, then go into Tools, Account Settings again (important to do this rather than just clicking on the account name in the top-left, as you want the identity that you're just about to setup to have the same composition settings as the main account)
6, Click Manage Identities, then Add
7, Fill in a new name and email address (doesn't really matter what, but presumably must be different to the other email address)
8, Fill in "test" in the signature
9, OK, Close
10, Click Write
11, Change the From address to the one that you made in the additional identity
12, Tab into the message body and observe that there are no blanks lines above the signature, and that message body text placed above the signature is grey

The important thing about the above variant is that the problem only shows up if the main account has a blank signature, and an additional identity has a non-blank signature. (Incidentally, you will also notice while doing step 12 the appearance of bug 118050 - "Caret (cursor) is positioned after the signature if you change the identity (From: address)").

Steps to reproduce variant number 2:

In TB 13.0.1, from a brand new TB profile:

1, Click "Skip this and use my existing email" and setup access to email (in my case, IMAP to a MS Exchange server, but I don't think this matters)
2, Tools, Account Settings
3, Fill in "test" in the signature box, *or* leave blank (bug occurs with either)
4, Composition & Addressing:
	a, Untick "Compose messages in HTML format"
	b, Tick "Automatically quote the original message when replying"
	c, Choose "start my reply above the quote"
	d, and place my signature "below my reply (above the quote)"
5, Click OK, then go into Tools, Account Settings again (important to do this rather than just clicking on the account name in the top-left, as you want the identity that you're just about to setup to have the same composition settings as the main account)
6, Click Manage Identities, then Add
7, Fill in a new name and email address (doesn't really matter what, but presumably must be different to the other email address)
8, Fill in "test 2" in the signature box
9, OK, Close, OK
10, Find a message in your inbox or a folder that has been sent to the main email address (not to the identity email address)
11, Click Reply
12, Change the From address to the one that you made in the additional identity
13, Tab into the message body and observe that although there is one blank line above the signature, any message body text placed above the signature is grey

To re-iterate, I'm pretty sure variant 1 didn't exist in TB 12 or earlier so was probably new in TB 13 or TB 13.0.1. I'm more certain in thinking that variant 2 may have been introduced in TB 13.0.1, that is, was not present in TB 13.

Comment 32

[Mark Banner (:standard8)]

Hi John, please can you file new bugs for your issues, commenting on closed bugs doesn't let us track your issues as well and they may get lost.

Comment 33

[John Veness]

Thanks for letting me know the correct thing to do. I have filed these as bug 766520 and bug 766521.