13.25 KB, patch
|Details | Diff | Splinter Review|
13.38 KB, patch
|Details | Diff | Splinter Review|
Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:184.108.40.206) 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:220.127.116.11) 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
OS: Windows Vista → All
Hardware: x86 → All
Created attachment 447846 [details] [diff] [review] patch 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.
Created attachment 447863 [details] [diff] [review] patch This includes Jim's suggestion, and works just fine.
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...
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?
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).
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
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.
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.
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.
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.
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.
Created attachment 449751 [details] [diff] [review] with better test, for 1.9.2 The test is slightly different for 1.9.2.
Created attachment 449754 [details] [diff] [review] [checked in] with better test, for trunk Now even without random junk...
Created attachment 449755 [details] [diff] [review] with better test, for 1.9.2
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).
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.2a1
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
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.