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

RESOLVED FIXED in Thunderbird 3.3a1

Status

RESOLVED FIXED
9 years ago
7 years ago

People

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

Tracking

({regression})

Trunk
Thunderbird 3.3a1
regression
Bug Flags:
in-testsuite +

Thunderbird Tracking Flags

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

Details

(Whiteboard: [gs], URL)

Attachments

(2 attachments, 7 obsolete attachments)

(Assignee)

Description

9 years ago
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

9 years ago
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 ?

Comment 3

9 years ago
(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.
Duplicate of this bug: 568176

Updated

9 years ago
Duplicate of this bug: 540418

Updated

9 years ago
Keywords: regressionwindow-wanted
(Assignee)

Comment 6

9 years ago
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
Keywords: regressionwindow-wanted
OS: Windows Vista → All
Hardware: x86 → All
(Assignee)

Comment 7

9 years ago
Created attachment 447846 [details] [diff] [review]
patch

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

Comment 8

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

Comment 9

9 years ago
Created attachment 447863 [details] [diff] [review]
patch

This includes Jim's suggestion, and works just fine.
Attachment #447846 - Attachment is obsolete: true
Attachment #447863 - Flags: superreview?
Attachment #447863 - Flags: review?
(Assignee)

Updated

9 years ago
Attachment #447863 - Flags: superreview?(bienvenu)
Attachment #447863 - Flags: superreview?
Attachment #447863 - Flags: review?(bienvenu)
Attachment #447863 - Flags: review?

Comment 10

9 years ago
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

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

Comment 12

9 years ago
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

9 years ago
(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...
(Assignee)

Comment 14

9 years ago
Created attachment 448150 [details] [diff] [review]
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?
(Assignee)

Comment 15

9 years ago
Created attachment 448432 [details] [diff] [review]
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).
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)
(Assignee)

Updated

9 years ago
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
(Assignee)

Comment 16

9 years ago
Created attachment 448593 [details] [diff] [review]
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.
Attachment #448593 - Flags: superreview?(bienvenu)
Attachment #448593 - Flags: review?(bienvenu)

Comment 17

9 years ago
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)
(Assignee)

Comment 18

9 years ago
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

9 years ago
yeah, I've done all that, and mozmill used to work on this machine, but it mysteriously stopped.
(Assignee)

Updated

9 years ago
Status: NEW → ASSIGNED

Updated

9 years ago
Attachment #448432 - Flags: superreview?(bienvenu) → superreview+

Comment 20

9 years ago
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-
(Assignee)

Comment 21

9 years ago
Created attachment 449750 [details] [diff] [review]
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.
Attachment #448432 - Attachment is obsolete: true
Attachment #449750 - Flags: superreview?(bienvenu)
Attachment #449750 - Flags: review?(bugzilla)
Attachment #448432 - Flags: superreview?(bugzilla)
(Assignee)

Comment 22

9 years ago
Created attachment 449751 [details] [diff] [review]
with better test, for 1.9.2

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)
(Assignee)

Updated

9 years ago
Attachment #449750 - Attachment is obsolete: true
Attachment #449750 - Flags: superreview?(bienvenu)
Attachment #449750 - Flags: review?(bugzilla)
(Assignee)

Updated

9 years ago
Attachment #449751 - Attachment is obsolete: true
Attachment #449751 - Flags: superreview?(bienvenu)
Attachment #449751 - Flags: review?(bugzilla)
(Assignee)

Comment 23

9 years ago
Created attachment 449754 [details] [diff] [review]
[checked in] with better test, for trunk

Now even without random junk...
Attachment #449754 - Flags: superreview?(bienvenu)
Attachment #449754 - Flags: review?(bugzilla)
(Assignee)

Comment 24

9 years ago
Created attachment 449755 [details] [diff] [review]
with better test, for 1.9.2
Attachment #449755 - Flags: superreview?(bienvenu)
Attachment #449755 - Flags: review?(bugzilla)

Updated

9 years ago
Attachment #449754 - Flags: superreview?(bienvenu) → superreview+

Comment 25

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

thx, that fails w/o the core change.

Updated

9 years ago
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.
(Assignee)

Comment 27

9 years ago
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.

Updated

9 years ago
Duplicate of this bug: 571603
(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
Last Resolved: 9 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+
Keywords: checkin-needed
Attachment #449754 - Flags: approval-thunderbird3.1.1+
Attachment #449755 - Flags: approval-thunderbird3.1.1? → approval-thunderbird3.1.1+
Checked into 1.9.2: http://hg.mozilla.org/releases/comm-1.9.2/rev/071f4342377e
status-thunderbird3.1: ? → .1-fixed
Keywords: checkin-needed

Updated

8 years ago
Duplicate of this bug: 575217
Duplicate of this bug: 578989
Duplicate of this bug: 579132

Comment 35

7 years ago
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?
(Assignee)

Comment 36

7 years ago
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

7 years ago
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.