545859 - Signature is misplaced when the compose window was invoked by a mailto: link with body parameter and signature position set to "above quote"

Status: VERIFIED FIXED

(MailNews Core :: Composition, defect)

Description

[Márton Balassa]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; hu; rv:1.9.1.7) Gecko/20091221 Firefox/3.5.7 (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.1; hu; rv:1.9.1.7) Gecko/20100111 Lightning/1.0b1 Thunderbird/3.0.1

When clicking on a mailto: link which has a body parameter, the signature text is placed above the message body instead of below.

Reproducible: Always

Steps to Reproduce:
1. Set your account's signature to something (e.g. SIGNATURE)
2. Create a html file with a mailto link, href="mailto:[e-mail]?body=MESSAGE TEXT"
3. Open the file in your browser, click on the link
Actual Results:  
Thunderbird compose window pops up, and the message looks like this:

SIGNATURE

MESSAGE TEXT

Expected Results:  
The message should look like:

MESSAGE TEXT

SIGNATURE

Comment 1

[Arnaud Ladrière]

I confirm, as I'm often using mailto links. This is pretty annoying.

Comment 2

[ccie25269]

I confirm this issue as well. I am running:

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2) Gecko/20100124 Firefox/3.6 (Swiftfox)
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1.9pre) Gecko/20100217 Lightning/1.0b1 Shredder/3.0.3pre

Comment 3

[griffon4]

I am seeing the same problem but coming from a different direction.  Using a command line to -Compose a message is giving me the same results.  This is a change from the V2 branch of TB which did not exhibit this behavior.

Also if I change the sub-option for "Automatically quote the original message when replying" "and place my signature" from {below my reply (above the quote)} to {below the quote (recommended)} the order comes out correctly with [Body] first, then [Sig].

Running Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1.8) Gecko/20100227 Thunderbird/3.0.3

Comment 4

[ccie25269]

Confirmed 'griffon4's workaround is successful for correcting the behavior after new mail composition is induced using an html mailto link ...

if I change the sub-option for "Automatically quote the original message
when replying" "and place my signature" from {below my reply (above the quote)}
to {below the quote (recommended)} the order comes out correctly with [Body]
first, then [Sig].

But now i have to copy/paste my signature manually for every email i reply to in my inbox, still very annoying. I'm not a seasoned programmer but it would be nice to confirm if there is a way to separate these two behaviors, and maybe develop a workaround for only new mail messages invoked via mailto links.

Comment 6

[Nathan Tuggy (:tuggyne)]

I can reproduce with Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1.9) Gecko/20100317 Lightning/1.0b1 Thunderbird/3.0.4 ID:20100317103207.
It's important to note that the signature position must be set to "above quote" (which is not recommended) for this bug to reproduce.

See also bug 57802 for a bit of (in-depth) background on how this was likely introduced.

Comment 8

[Justin Tocci]

I want to add that it seems to me the behavior of '-compose' from the command line should not be affected by checkboxes having to do with reply behavior. '-compose' would seem to be equivalent to clicking on 'write' and so should act that way.

It would add value for my users if '-compose' from the command line defaulted to the same behavior as clicking on 'Write'. Perhaps the solution is to add an option to '-compose' for specifying whether the behavior is supposed to be 'reply' behavior or 'write' behavior.

Comment 9

[Matt Doran]

I too can confirm this and behaviour for "mailto" links, and would like to see this fixed. :)

Comment 14

[Erick]

This is not limited to mailto links.  It happens with the "Send Link..." in Firefox, "Send Link" in Chrome, "Send To... Mail Recipient" in Windows.

This bug has been around for 16 months.  It changes behavior in a way which creates a negative UX.  Several bugs have been marked as dups, so people are bothered enough to report it.  I don't have the skills to fix it quickly, so who can I buy a case of beer to get this done for the next release?  Seriously.

Comment 15

[Erick]

This is a seemingly simple regression - anyone gonna fix it?  Do I need to buy someone a case of beer?

Comment 16

[Michael Belsky]

My company uses an IT case management software called Numara Track-It! that opens a case after you email the support email. The confirmation email contains a link that points to:
mailto:Support@******.com?subject=STATUS&body=CASE#

This allows you to click the link and a compose email window opens with the body containing the case number, such as 5430.

I have my signature set to place my signature "below my reply (above the quote)" and I can confirm that the same behavior occurs. (On a side note, why is having the sig appear below the quote the recommended option? Seems counter-intuitive to me.)

The resulting body in the created email is:

SIGNATURE
CASE#

instead of:
CASE#
SIGNATURE

This actually makes the automated tracking software not recognize the case number you are trying to get the status of, since it is looking at the first number in the body to find the case number.

Comment 17

[emeadows]

I, too, am finding this bug to be a huge annoyance.  One of my clients is complaining as well.

This seems like such a simple fix.  Can you please find the time to get this problem fixed?

Thank you very much!

Comment 18

[Erick]

I'm on 7.0.1 and this bug/regression still exists.  Is there any hope of a fix?  This is exceedingly annoying.

Comment 19

[mattosaur4]

Seems not I logged a variant of this related to messages coming into TBird from FileMaker in Sig, Message order, still hasn't been fixed, that was almost 2 years ago.

Comment 20

[Erick]

The version here is marked as 3.0, but it still exists in 8.0.  Is anyone going to fix it?  You break your software and then refuse to fix it for 2 years and ignore those who request it be fixed?  Give me a break - this is beyond ridiculous.  Oh, but it's free.  Oh, but I offered to buy someone a case of beer to fix it, but no takers (offer withdrawn).  Seems to me mozilla is too focused on kicking out new major release numbers and not on fixing actual bugs.

Comment 21

[Magnus Melin [:mkmelin]]

The signature handling is... complex. The fix should be somewhere around here: http://mxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgCompose.cpp#642

Comment 22

[Tom Chiverton]

If the fixer lives in England, I will drive a 12 pack of beer to your door (or other place of choosing).
If you are in the US, I have a Amazon gift certificate for you.
Elsewhere, we'll arrange something.

Comment 23

[Erick]

Wow!  http://allthingsd.com/20111222/google-will-pay-mozilla-almost-300m-per-year-in-search-deal-besting-microsoft-and-yahoo/

$300 million ought to pay for someone to fix this!  And it might even cover firing whomever thought version numbers skyrocketing was more important than fixing bugs!

(I add 1 year and 10 months after this idiotic regression was first reported - and after 6 major revisions of TB were released!)

Comment 24

[8472]

I've ran into this issue as well.

Interesting thing is, that I have noticed two different kinds of behaviour.
- At home, I'm using an older version of Ubuntu 10.10 , with TBird installed from additional repository, and also one copy downloaded from mozilla's web, placed in the /opt/ directory.
- At work, I'm using Arch Linux, where I have TBird installed from the main repository.

At home, I cannot reproduce this bug, but at work, there is it always present. :(
Thus, can't the problem be somewhere in the user's profile? If yes, I have no idea where, as I'm also not an programmer.

Comment 25

[marty]

It does seem that bug 819012 that I've just reported could be a dupe of this nearly three year/14 version old report that's yet to be resolved.

To this I'd respond that the "workaround" identified here is not only unacceptable (placing the signature below the reply quote - you could be replying to a long thread), but illogically links two different issues. One is an original message launched by another application or Mailto:// call, the other is a reply to a received message from the message window.

I'll add that until now I've been using a signature add-on, but then even simple emails are delivered with an Attachment flag. This a) is misleading, b)can cause the message to be rejected by some email and list servers.

Comment 26

[Thonixx]

It should be fixed I think. An addon is not a good workaround of this annoying bug.
Does someone of Thunderbird developers read this bug? I think no one is interested in fixing this behaviour.

Comment 27

[rsx11m]

This is also reproducible in recent SeaMonkey builds, hence a MailNews Core bug.

Comment 29

[Thonixx]

Ok here the discussion.
I have Linux (Ubuntu) and the latest version of Thunderbird installed.
And I have plain text signatures/emails.

So the problem lays in another section than HTML or operating system things.

Comment 30

[rsx11m]

Correct, HTML vs. plain-text composition or signature shouldn't be a factor.

Further analysis:

 - problem occurs only with "above the quote" /and/ "below my reply" settings
   in the Composition & Addressing tab (as stated before);

 - also reproducible when using "thunderbird.exe -compose body=test" from the
   command line, thus not using a "mailto:" URL (i.e., not a parsing issue);

 - unchecking "Include signature for reply" doesn't change the behavior;

 - the signature separator is placed as if there was no quote.

Thus,

 - context is clearly not "quote in reply" given the last two observations;

 - but, apparently the body text provided as command-line argument is treated
   as quoted text when it comes to the placement of that part, thus a "real"
   quote from a reply doesn't seem to be distinguished from a body passed as
   argument as it should;

 - maybe related to the forwarding behavior which introduced signature above
   the forwarded text following the reply settings, but then I would assume the
   "Include signature for forwards" setting being honored and the signature
   divider removed, neither of which is the case (thus likely not a candidate).

Per bug 516868 comment #12, this was working in TB 2.0; reported observed broken with or after TB 3.0 beta 4 (which leaves a wide range of possible causes...).

Comment 31

[rsx11m]

Regression range:

 - Works with Thunderbird 3.0a2pre 2008061005 (Linux i686)
 - Broken per Thunderbird 3.0a2pre 2008061103

Bonsai query:

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2008-06-10+05%3A00%3A00&maxdate=2008-06-11+03%3A59%3A59&cvsroot=%2Fcvsroot

Changes in nsMsgCompose.cpp during that time frame:

> Bug 437975 - Can't send emails to non-mork mailing lists.
> Bug 23394  - Quote just the selected portion of a message during Reply.
> Bug 273114 - When forwarding messages signature line is placed after
>     forwarded message (should obey reply signature position pref)

The last bug in this list looks like the best candidate for having caused the issue.

Comment 32

[rsx11m]

(In reply to rsx11m from comment #30)
> 
>  - but, apparently the body text provided as command-line argument is treated
>    as quoted text when it comes to the placement of that part, thus a "real"
>    quote from a reply doesn't seem to be distinguished from a body passed as
>    argument as it should;

Looks like my hunch was correct - bug 273114 introduced some special handling of forwarded messages in nsMsgCompose::BuildBodyMessageAndSignature(), but that context is lost later in nsMsgCompose::ConvertAndLoadComposeWindow() where only the information is retained whether or not the message body is a reply quote.

Thus, a possible solution is to also pass a boolean parameter to indicate that the message is built for nsIMsgCompType::New or nsIMsgCompType::MailToUrl and then disable "signature on top" for these cases to avoid this bug.

Comment 33

[rsx11m]

Attachment: Possible fix (untested)

This quick patch follows bug 273114 attachment 317497 [details] [diff] [review] and adds another parameter aNewBody to nsMsgCompose::ConvertAndLoadComposeWindow() to indicate that this is neither quoted nor forwarded but "new" content. It then adds it to the conditions for sigOnTop handling.

While this should fix it for SeaMonkey I'm not sure if it would be acceptable for Thunderbird as it modifies the nsIMsgCompose : nsIMsgSendListener interface, thus may render the patch unsuitable for the branches.

I'm not able to test the patch thoroughly at this time, but it should be a good starting point even if I screwed up the logic somewhere.

Comment 34

[rsx11m]

Attachment: Proposed fix (unbitrotted)

The draft posted before works fine, thus I'm requesting formal review for it. Tested in both plain-text and HTML composition and in all three signature modes (reply below, reply above with sig below the quote, reply above with sig below reply) the text passed with "-compose body=..." appeared above the sig as with new messages. No regressions in either reply or forward behaviors was observed.

Asking Neil for sr since this contains a mailnews/ interface change.

Comment 35

[rsx11m]

Attachment: Branch version (comm-esr17)

(In reply to rsx11m from comment #33)
> While this should fix it for SeaMonkey I'm not sure if it would be acceptable
> for Thunderbird as it modifies the nsIMsgCompose : nsIMsgSendListener interface,
> thus may render the patch unsuitable for the branches.

This is the branch version of attachment 691088 [details] [diff] [review], moving isNewBody into nsMsgCompose::ConvertAndLoadComposeWindow() itself rather than passing it as a parameter (that's ugly given that mType has to be evaluated within that  function now but does the job for 17.0.x).

Note that in both patches *NewBody isn't merged into sigOnTop on purpose - the former is a context flag (like aQuoted) whereas the latter represents the logical combination of two user preferences. Thus, it appears to be cleaner to keep them around as separate variables and to do the logic when evaluating aBody or aSignature in the conditional statements.

Comment 36

[[e-mail]]

(In reply to rsx11m from comment #35)
> Note that in both patches *NewBody isn't merged into sigOnTop on purpose -
> the former is a context flag (like aQuoted) whereas the latter represents
> the logical combination of two user preferences. Thus, it appears to be
> cleaner to keep them around as separate variables and to do the logic when
> evaluating aBody or aSignature in the conditional statements.

Ah, I did wonder briefly about that.

So, there are three cases:
1. Some sort of reply. In this case aQuote is true, we cite the quotation, then insert the signature at the appropriate position.
2. Forward inline. In this case we paste the body in but we want the signature above it if sigOnTop is true.
3. mailto: In this case we paste the body in but we want to ignore sigOnTop.

The control flow is quite hard to follow but I feel that it would be slightly better to special-case mType == nsIMsgCompType::ForwardInline in ConvertAndLoadComposeWindow, as there is already precedence for that case (i.e. you can define another local boolean and use it to replace the existing test).

Comment 37

[rsx11m]

Thanks Neil. So, your suggestion is to go with something similar to the branch patch in attachment 691543 [details] [diff] [review], replacing "isNewBody" with "isForwarded" though and forget about modifying the nsIMsgCompose interface?

Alternatively, "aNewBody" could be replaced with "aForwarded" in the interface, replacing the test for nsIMsgCompType::ForwardInline in line #715 as well? That would make it more consistent with respect to not using mType in this function.

Comment 38

[rsx11m]

Attachment: Proposed fix (v3, trunk)

This is the logical equivalent of attachment 691088 [details] [diff] [review] with comment #36 addressed, i.e., treating "Forward" rather than "New" as special case.

I still think that it's cleaner to pass two arguments (aQuoted, aForwarded) as they are of similar function; it feels odd to get one case from an argument but the other from the attribute (though aQuoted combines multiple cases, ok).

Neil, if you disagree and don't want to change the interface, please sr- this version, and the branch patch would apply to trunk as well in this case.

Comment 39

[rsx11m]

Attachment: Proposed fix (v3)

Logical equivalent of attachment 691543 [details] [diff] [review] with comment #36 addressed. No changes in interface definitions, thus suitable for release branches.

Comment 40

[[e-mail]]

Comment on attachment 692468 [details] [diff] [review]
Proposed fix (v3)

I like this version.

Comment 41

[[e-mail]]

Comment on attachment 692467 [details] [diff] [review]
Proposed fix (v3, trunk)

I'll reconsider this when (if?) it passes review.

Comment 42

[rsx11m]

Ok, let's wait for Mark's pick then. I'm certainly fine with taking the simpler patch on trunk as well, the other one just may be clearer and would add some consistency.

Since there don't seem to be any comprehensive signature-positioning tests yet (which may be beyond the scope of this bug), I've opened bug 822047 covering bug 57802, bug 167319, bug 273114, and this bug. I've only found an xpcshell unit test "test_mailtoURL.js" for basic URL parsing, and a Mozmill test for multiple signatures and separators, "test-signature-updating.js". A first attempt to modify the xpcshell test failed as it's too low level and doesn't actually use ConvertAndLoadComposeWindow() or nsIMsgCompose as such (posted in the new bug for reference), and adding the mailto: cases to the existing Mozmill tests unfortunately is over my head.

Comment 43

[Mark Banner (:standard8)]

Comment on attachment 692468 [details] [diff] [review]
Proposed fix (v3)

Review of attachment 692468 [details] [diff] [review]:
-----------------------------------------------------------------

Ok, I think this is fine. I think I also prefer it to changing the interface - convertAndLoadComposeWindow is pretty much an internal function anyway, and I think using the stored attribute is fine in this case (just as it already is for the first check).

Comment 44

[rsx11m]

Comment on attachment 692468 [details] [diff] [review]
Proposed fix (v3)

Thanks for the reviews, check-in for trunk please.

Comment 45

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/7f228f4d0672

Comment 46

[rsx11m]

This looks good on trunk and no regressions/issues have been reported over the last week. Since the release process for 17.0.2/18.0 has started already, I'll request branch approval after the upcoming merge for the 19.0 cycle.

Comment 47

[rsx11m]

Comment on attachment 692468 [details] [diff] [review]
Proposed fix (v3)

With today's merge, the fix is present in comm-aurora already, thus we'd only need it on comm-beta as well. Assuming that bug 815302 will be finished in time for 17.0.3, requesting comm-esr17 (default branch) approval only but not for comm-release (feel free to approve and push it there anyway if that's better).

[Approval Request Comment]
Regression caused by (bug #): bug 273114 
User impact if declined: Affected Thunderbird users will have to wait until 24.0 is released to benefit from this fix, thus would have to live with the unsatisfactory workarounds described in the comments leading up to the fix. As such, no security or stability impact.

Testing completed (on c-c, etc.): see comment #42; manually tested on trunk nightlies after pushing the patch to c-c.

Risk to taking this patch (and alternatives if risky): Minimal risk, just correcting logic of the initial patch causing the regression.

Comment 48

[Mark Banner (:standard8)]

Comment on attachment 692468 [details] [diff] [review]
Proposed fix (v3)

We'll land this on beta for now and get some testing, and land for release later.

Comment 49

[Mark Banner (:standard8)]

https://hg.mozilla.org/releases/comm-beta/rev/e94e3425c464

Comment 50

[rsx11m]

Both the Thunderbird 19.0 beta and SeaMonkey 2.16 beta 1 are now available for testing and contain the fix. It would be great if one or more of the original reporters can verify that it's working now with their specific application.

https://www.mozilla.org/en-US/thunderbird/channel

Please read http://kb.mozillazine.org/Testing_pre-release_versions before installing and running any pre-release version.

Comment 51

[Erick]

Confirmed fixed in TB 19b1.  Thank you!!!!!!!!!!!!

Comment 52

[rsx11m]

Thanks for testing. Marking verified pending any further reports.

Comment 53

[8472]

I can also confirm it's fixed in TB 19b1.
Furthermore, this fix is not compatible with add-on SmartTemplate4 , meaning, when this add-on is enabled and used, it won't have any effect, and the link is still misplaced on the bottom, below the signature. But I believe, this would have to be fixed by the add-on's developer afterwards.

Comment 54

[rsx11m]

Correct, making adjustments to add-ons after the main code has been changed in a way that affects them is the duty of the add-on developer. I'm not sure if it really impacts the add-on (specifically given that no API changes are employed), thus it rather appears that it may override the regular functions with its own, consequently any change here won't modify the behavior of the overriding add-on. This is speculation, of course, as I don't know the code of that add-on.

Comment 55

[Mark Banner (:standard8)]

https://hg.mozilla.org/releases/comm-esr17/rev/b72e2bda999c