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

VERIFIED FIXED in Thunderbird 20.0

Status

MailNews Core
Composition
VERIFIED FIXED
8 years ago
4 years ago

People

(Reporter: Márton Balassa, Assigned: rsx11m)

Tracking

(Blocks: 1 bug, {regression})

Trunk
Thunderbird 20.0
regression
Dependency tree / graph

Thunderbird Tracking Flags

(thunderbird18 wontfix, thunderbird19 verified, thunderbird20 fixed, thunderbird-esr1719+ fixed, seamonkey2.15 wontfix, seamonkey2.16 fixed, seamonkey2.17 fixed)

Details

(Whiteboard: [gs][tb-papercut], URL)

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

8 years ago
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:address@host.com?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
(Reporter)

Updated

8 years ago
Severity: normal → minor
Version: unspecified → 3.0

Comment 1

7 years ago
I confirm, as I'm often using mailto links. This is pretty annoying.
Component: OS Integration → Mail Window Front End
QA Contact: os-integration → front-end

Comment 2

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

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

7 years ago
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.
Duplicate of this bug: 557334
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.
Status: UNCONFIRMED → NEW
Depends on: 57802
Ever confirmed: true
Hardware: x86_64 → All
Summary: Signature is misplaced when the compose window was invoked by a mailto: link with body parameter → Signature is misplaced when the compose window was invoked by a mailto: link with body parameter and signature position set to "above quote"

Updated

7 years ago
Duplicate of this bug: 560529

Comment 8

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

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

Updated

6 years ago
Duplicate of this bug: 596119

Updated

6 years ago
Duplicate of this bug: 538691

Updated

6 years ago
Duplicate of this bug: 516868

Updated

6 years ago
Duplicate of this bug: 667631

Comment 14

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

Updated

6 years ago
Component: Mail Window Front End → Message Compose Window
OS: Windows 7 → All
QA Contact: front-end → message-compose

Comment 15

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

Comment 16

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

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

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

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

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

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

Updated

6 years ago
Version: 3.0 → Trunk

Comment 22

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

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

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

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

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

Comment 27

5 years ago
This is also reproducible in recent SeaMonkey builds, hence a MailNews Core bug.
Severity: minor → normal
Component: Message Compose Window → Composition
Product: Thunderbird → MailNews Core
(Assignee)

Updated

5 years ago
Duplicate of this bug: 819012

Comment 29

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

Comment 30

5 years ago
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...).
Keywords: regression, regressionwindow-wanted
(Assignee)

Comment 31

5 years ago
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.
Keywords: regressionwindow-wanted
Whiteboard: [regression range: comment #31]
(Assignee)

Comment 32

5 years ago
(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.
Blocks: 273114
Whiteboard: [regression range: comment #31]
(Assignee)

Comment 33

5 years ago
Created attachment 690097 [details] [diff] [review]
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.
Attachment #690097 - Flags: feedback?(mbanner)
(Assignee)

Comment 34

5 years ago
Created attachment 691088 [details] [diff] [review]
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.
Attachment #690097 - Attachment is obsolete: true
Attachment #690097 - Flags: feedback?(mbanner)
Attachment #691088 - Flags: superreview?(neil)
Attachment #691088 - Flags: review?(mbanner)
(Assignee)

Comment 35

5 years ago
Created attachment 691543 [details] [diff] [review]
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.
Assignee: nobody → rsx11m.pub
Status: NEW → ASSIGNED
Attachment #691543 - Flags: review?(mbanner)
(Assignee)

Updated

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

Updated

5 years ago
Attachment #691088 - Flags: superreview?(neil) → feedback+
(Assignee)

Comment 37

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

Comment 38

5 years ago
Created attachment 692467 [details] [diff] [review]
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.
Attachment #691088 - Attachment is obsolete: true
Attachment #691088 - Flags: review?(mbanner)
Attachment #692467 - Flags: superreview?(neil)
Attachment #692467 - Flags: review?(mbanner)
(Assignee)

Comment 39

5 years ago
Created attachment 692468 [details] [diff] [review]
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.
Attachment #691543 - Attachment is obsolete: true
Attachment #691543 - Flags: review?(mbanner)
Attachment #692468 - Flags: review?(mbanner)
Comment on attachment 692468 [details] [diff] [review]
Proposed fix (v3)

I like this version.
Attachment #692468 - Flags: feedback+
Comment on attachment 692467 [details] [diff] [review]
Proposed fix (v3, trunk)

I'll reconsider this when (if?) it passes review.
Attachment #692467 - Flags: superreview?(neil)
(Assignee)

Comment 42

5 years ago
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.
Blocks: 822047
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).
Attachment #692468 - Flags: review?(mbanner) → review+
Attachment #692467 - Flags: review?(mbanner) → review-
(Assignee)

Updated

5 years ago
Attachment #692467 - Attachment is obsolete: true
(Assignee)

Comment 44

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

Thanks for the reviews, check-in for trunk please.
Attachment #692468 - Attachment description: Proposed fix (v3, branches) → Proposed fix (v3)
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
Whiteboard: [gs] → [gs][c-n: comm-central]
https://hg.mozilla.org/comm-central/rev/7f228f4d0672
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [gs][c-n: comm-central] → [gs]
Target Milestone: --- → Thunderbird 20.0
(Assignee)

Comment 46

5 years ago
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.
status-seamonkey2.15: --- → wontfix
status-seamonkey2.16: --- → affected
status-seamonkey2.17: --- → fixed
status-thunderbird18: --- → wontfix
status-thunderbird19: --- → affected
status-thunderbird20: --- → fixed
status-thunderbird-esr17: --- → affected
(Assignee)

Comment 47

5 years ago
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.
Attachment #692468 - Flags: approval-comm-esr17?
Attachment #692468 - Flags: approval-comm-beta?
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.
Attachment #692468 - Flags: approval-comm-beta? → approval-comm-beta+
https://hg.mozilla.org/releases/comm-beta/rev/e94e3425c464
status-seamonkey2.16: affected → fixed
status-thunderbird19: affected → fixed
tracking-thunderbird-esr17: --- → 19+
(Assignee)

Comment 50

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

5 years ago
Confirmed fixed in TB 19b1.  Thank you!!!!!!!!!!!!
(Assignee)

Comment 52

5 years ago
Thanks for testing. Marking verified pending any further reports.
Status: RESOLVED → VERIFIED
status-thunderbird19: fixed → verified
Whiteboard: [gs] → [gs][tb-papercut]

Comment 53

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

Comment 54

5 years ago
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.
Attachment #692468 - Flags: approval-comm-esr17? → approval-comm-esr17+
https://hg.mozilla.org/releases/comm-esr17/rev/b72e2bda999c
status-thunderbird-esr17: affected → fixed
You need to log in before you can comment on or make changes to this bug.