566825 - Multiple signatures when switching between accounts (signature of previously selected account is not removed) in plaintext editor

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[Steffen Wilberg]

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2.4) Gecko/20100518 Lightning/1.0b2pre Lanikai/3.1pre

STR:
0. Have multiple accounts, each which a signature.
1. In the compose window, click on the "From:" menu list and switch to another account.

Actual result:
The signature of the new account is added at the bottom of the message.
However, the signature of the previous account is not removed.

Expected result:
The old signature should be removed.

This doesn't happen with 3.0 on the same profile.
(Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1.10) Gecko/20100518 Lightning/1.0b2pre Shredder/3.0.6pre)

Comment 1

[rsx11m]

With a plain-text signature, I see this in plain-text composition mode. Composing in HTML removes the signature but leaves a space, i.e., with each change of identity the space between the top and signature increases.

Comment 2

[Ludovic Hirlimann [:Usul]]

How is this different from bug 218346 ?

Comment 3

[rsx11m]

(Quoting bug 218346 comment #1)
> With settings:
> - start my reply above the quote
> - and place my sig below my reply
> - and multiple identities

The problem here occurs already with a new composition, i.e., not just when replying. The underlying issue in the other bug is that the DDS separator isn't added for the combination of points #1 and #2, thus the signature cannot be recognized. Here, the separator is present but the old signature not removed when the identity is changed. Thus, it's a much broader and significant issue.

Comment 6

[Steffen Wilberg]

This regressed almost a year ago :-(

WORKS:  Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-US; rv:1.9.2a1pre) Gecko/20090608 Shredder/3.1a1pre

BROKEN: Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-US; rv:1.9.2a1pre) Gecko/20090609 Shredder/3.1a1pre

So this is a regression from bug 468708, which changed localName to returning lower case. But we're still checking for "BR" instead of "br" in nsMsgCompose::SetIdentity.

Comment 7

[Steffen Wilberg]

Attachment: patch

Here's the obvious patch. Will request review once I've built and tested it.

Comment 8

[Jim Porter (:squib)]

Yeah that works for me, though you might want to switch the <BR> to lowercase on line 4132 as well, for consistency.

Comment 9

[Steffen Wilberg]

Attachment: patch

This includes Jim's suggestion, and works just fine.

Comment 10

[David :Bienvenu]

Looks reasonable, thx for the patch, but we should probably have a mozmill test for this so we don't get broken again. Or an xpcshell test, if that's possible/easier...

Comment 11

[rsx11m]

I'm wondering why the comparison is case sensitive to start with, couldn't you use LowerCaseEqualsLiteral() instead to avoid case conflicts?

Comment 12

[Steffen Wilberg]

According to the doc (https://developer.mozilla.org/En/DOM/Node.localName), localName always returns lower case, starting with Gecko 1.9.2.

Will look into writing a Mozmill test, but haven't done so before.
I already found
http://mxr.mozilla.org/comm-central/source/mail/test/mozmill/composition/test-attachment-reminder.js and
http://mxr.mozilla.org/comm-central/source/mail/test/mozmill/shared-modules/test-compose-helpers.js

Setting up multiple accounts with different signatures is probably the hardest part here.
http://mxr.mozilla.org/comm-central/source/mail/test/mozmill/account/test-mail-account-setup-wizard.js#88 shows how to remove an account...

But can't we tweak the test framework to have multiple accounts with different signatures to begin with?

Comment 13

[rsx11m]

(In reply to comment #12)
> localName always returns lower case, starting with Gecko 1.9.2.

Yes, you said that in comment #6 already (sigh), sorry for sleeping...

Comment 14

[Steffen Wilberg]

Attachment: with mozmill test, wip

Adding a second identity and signatures to both identities to the default profile was the easiest thing. Figuring out how to access the content frame and the signature was much harder.

This tests signature switching in html compose mode.
What's the best way to open a plaintext compose window in mozmill? Just flipping the mail.identity.id1.compose_html pref?

Comment 15

[Steffen Wilberg]

Attachment: patch with mozmill test

This tests both plaintext and html compose windows, switching from a plaintext signature to a html signature by changing to a second identity.

I sorted mozmilltests.list alphabetically to make it easier to find certain directories/files.

Patch and test tested on comm-central Linux.
Patch tested on comm-1.9.2 Linux.
(branch doesn't compile without --disable-tests here, and the IDE of the Mozmill addon doesn't work either).

Comment 16

[Steffen Wilberg]

Attachment: patch with mozmill test, for 1.9.2 branch

I managed to build Lanikai with --disable-crashreporter.

The test is slightly different between trunk and branch for the html compose window:

On trunk, the lines in the signature are separated by "\n"s:
"-- \nfirst line\nsecond line"

On branch, the lines are separated by <br> tags.

Comment 17

[David :Bienvenu]

Comment on attachment 448593 [details] [diff] [review]
patch with mozmill test, for 1.9.2 branch

Thx for the test. I can't run mozmill tests at the moment, so I'm going to switch the test part of the review to Standard8. If I get mozmill working on my system soon, I'll switch the request back.

Comment 18

[Steffen Wilberg]

On Ubuntu 10.4, I installed (sudo apt-get install) python-setuptools and ran the commands listed on https://developer.mozilla.org/en/Thunderbird/Thunderbird_MozMill_Testing#Mac.2fLinux,

then run "make SOLO_TEST=composition/test-signature-updating.js mozmill-one" in the objdir.

Comment 19

[David :Bienvenu]

yeah, I've done all that, and mozmill used to work on this machine, but it mysteriously stopped.

Comment 20

[David :Bienvenu]

Comment on attachment 448432 [details] [diff] [review]
patch with mozmill test

from what I can tell, this test doesn't fail in the bug case, i.e., with an unpatched version of nsMsgCompose.cpp

this is not a comment style that we use:

+/** Test that the plaintext compose window has a signature initially,
+    and has the correct signature after switching to another identity. */

should use

/**
 *
 */

style.

I'd like to run the reordering of momilltests.list past Standard8.

Comment 21

[Steffen Wilberg]

Attachment: with better test, for trunk

This should now actually test this bug...
I didn't yet check if the test manages to fail on an unpatched build.

Comment 22

[Steffen Wilberg]

Attachment: with better test, for 1.9.2

The test is slightly different for 1.9.2.

Comment 23

[Steffen Wilberg]

Attachment: [checked in] with better test, for trunk

Now even without random junk...

Comment 24

[Steffen Wilberg]

Attachment: with better test, for 1.9.2

Comment 25

[David :Bienvenu]

Comment on attachment 449754 [details] [diff] [review]
[checked in] with better test, for trunk

thx, that fails w/o the core change.

Comment 26

[Mark Banner (:standard8)]

Comment on attachment 449754 [details] [diff] [review]
[checked in] with better test, for trunk

I'm still trying this out, but I get the feeling that you should also test for switching back to the identity that doesn't have the signature.

Comment 27

[Steffen Wilberg]

I already made the first identity have a plaintext signature and added a second identity with a html signature. So it's pretty straight-forward to add a third identity without a signature, and test switching to that.

I can do that after I come back from vacation in two weeks - I have no dev environment with me. Would be nice to see this bug fixed in the meantime though.

Comment 29

[Mark Banner (:standard8)]

(In reply to comment #27)
> I already made the first identity have a plaintext signature and added a second
> identity with a html signature.

Oh so actually, I got my thinking wrong about the test, and now I re-read it, I'm happy with what it is doing.

Comment 30

[Mark Banner (:standard8)]

Comment on attachment 449754 [details] [diff] [review]
[checked in] with better test, for trunk

Thanks for this, I've now checked it in as well:

http://hg.mozilla.org/comm-central/rev/db911c5871d2

(I had to fix a bit of bitrot in mozmilltests.list, but otherwise it was all fine).

Comment 31

[Mark Banner (:standard8)]

Checked into 1.9.2: http://hg.mozilla.org/releases/comm-1.9.2/rev/071f4342377e

Comment 35

[earlpiggot]

It's still present in 6.0 beta 1 and it makes using multiple accounts pretty useless in message composition.

Will it be fixed in the next 6.0 beta, please?

Comment 36

[Steffen Wilberg]

This worked in Thunderbid 3.0, was broken in 3.1, and is fixed since 3.1.1.
You must be seeing a different bug, so please file one with clear steps to reproduce:
https://bugzilla.mozilla.org/enter_bug.cgi?product=Thunderbird&component=Mail%20Window%20Front%20End

Comment 37

[earlpiggot]

You are right. One of my extensions must be the cause. It's OK in safe mode.