Closed Bug 566825 Opened 14 years ago Closed 14 years ago

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

Categories

(Thunderbird :: Message Compose Window, defect)

defect
Not set
normal

Tracking

(thunderbird3.1 .1-fixed, thunderbird3.0 unaffected)

RESOLVED FIXED
Thunderbird 3.3a1
Tracking Status
thunderbird3.1 --- .1-fixed
thunderbird3.0 --- unaffected

People

(Reporter: steffen.wilberg, Assigned: steffen.wilberg)

References

()

Details

(Keywords: regression, Whiteboard: [gs])

Attachments

(2 files, 7 obsolete files)

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)
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.
How is this different from bug 218346 ?
(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.
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.
Assignee: nobody → steffen.wilberg
Blocks: 468708
OS: Windows Vista → All
Hardware: x86 → All
Attached patch patch (obsolete) — Splinter Review
Here's the obvious patch. Will request review once I've built and tested it.
Yeah that works for me, though you might want to switch the <BR> to lowercase on line 4132 as well, for consistency.
Attached patch patch (obsolete) — Splinter Review
This includes Jim's suggestion, and works just fine.
Attachment #447846 - Attachment is obsolete: true
Attachment #447863 - Flags: superreview?
Attachment #447863 - Flags: review?
Attachment #447863 - Flags: superreview?(bienvenu)
Attachment #447863 - Flags: superreview?
Attachment #447863 - Flags: review?(bienvenu)
Attachment #447863 - Flags: review?
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...
I'm wondering why the comparison is case sensitive to start with, couldn't you use LowerCaseEqualsLiteral() instead to avoid case conflicts?
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?
(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...
Attached patch with mozmill test, wip (obsolete) — Splinter Review
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?
Attached patch patch with mozmill test (obsolete) — Splinter Review
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).
Attachment #447863 - Attachment is obsolete: true
Attachment #448150 - Attachment is obsolete: true
Attachment #448432 - Flags: superreview?(bienvenu)
Attachment #448432 - Flags: review?(bienvenu)
Attachment #447863 - Flags: superreview?(bienvenu)
Attachment #447863 - Flags: review?(bienvenu)
Summary: Multiple signatures when switching between accounts (signature of previously selected account is not removed) → Multiple signatures when switching between accounts (signature of previously selected account is not removed) in plaintext editor
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.
Attachment #448593 - Flags: superreview?(bienvenu)
Attachment #448593 - Flags: review?(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.
Attachment #448593 - Flags: superreview?(bienvenu)
Attachment #448593 - Flags: superreview+
Attachment #448593 - Flags: review?(bugzilla)
Attachment #448593 - Flags: review?(bienvenu)
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.
yeah, I've done all that, and mozmill used to work on this machine, but it mysteriously stopped.
Status: NEW → ASSIGNED
Attachment #448432 - Flags: superreview?(bienvenu) → superreview+
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.
Attachment #448432 - Flags: superreview?(bugzilla)
Attachment #448432 - Flags: superreview+
Attachment #448432 - Flags: review?(bienvenu)
Attachment #448432 - Flags: review-
Attached patch with better test, for trunk (obsolete) — Splinter Review
This should now actually test this bug...
I didn't yet check if the test manages to fail on an unpatched build.
Attachment #448432 - Attachment is obsolete: true
Attachment #449750 - Flags: superreview?(bienvenu)
Attachment #449750 - Flags: review?(bugzilla)
Attachment #448432 - Flags: superreview?(bugzilla)
Attached patch with better test, for 1.9.2 (obsolete) — Splinter Review
The test is slightly different for 1.9.2.
Attachment #448593 - Attachment is obsolete: true
Attachment #449751 - Flags: superreview?(bienvenu)
Attachment #449751 - Flags: review?(bugzilla)
Attachment #448593 - Flags: review?(bugzilla)
Attachment #449750 - Attachment is obsolete: true
Attachment #449750 - Flags: superreview?(bienvenu)
Attachment #449750 - Flags: review?(bugzilla)
Attachment #449751 - Attachment is obsolete: true
Attachment #449751 - Flags: superreview?(bienvenu)
Attachment #449751 - Flags: review?(bugzilla)
Now even without random junk...
Attachment #449754 - Flags: superreview?(bienvenu)
Attachment #449754 - Flags: review?(bugzilla)
Attachment #449755 - Flags: superreview?(bienvenu)
Attachment #449755 - Flags: review?(bugzilla)
Attachment #449754 - Flags: superreview?(bienvenu) → superreview+
Comment on attachment 449754 [details] [diff] [review]
[checked in] with better test, for trunk

thx, that fails w/o the core change.
Attachment #449755 - Flags: superreview?(bienvenu) → superreview+
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.
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.
(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 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).
Attachment #449754 - Attachment description: with better test, for trunk → [checked in] with better test, for trunk
Attachment #449754 - Flags: review?(bugzilla) → review+
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.2a1
Flags: in-testsuite+
Attachment #449755 - Flags: review?(bugzilla)
Attachment #449755 - Flags: review+
Attachment #449755 - Flags: approval-thunderbird3.1.1?
Attachment #449754 - Flags: approval-thunderbird3.1.1+
Attachment #449754 - Flags: approval-thunderbird3.1.1+
Attachment #449755 - Flags: approval-thunderbird3.1.1? → approval-thunderbird3.1.1+
Whiteboard: [gs]
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?
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
You are right. One of my extensions must be the cause. It's OK in safe mode.
You need to log in before you can comment on or make changes to this bug.