614919 - Reply to HTML message with account that use HTML signature file, quote my signature (builds broken starts 20101115)

Status: VERIFIED FIXED

(MailNews Core :: Composition, defect)

Description

[[:Aureliano Buendía]]

STR:

1. configure an account [A] to attach an *.html file as your signature;
2. reply with this account to message [M]
3. the new created message as reply of [M] quote my signature!

It's a regression

TB 3.3a2pre 20101125

Comment 1

[[:Aureliano Buendía]]

Attachment: signature testcase

attached signature testcase

Comment 2

[(no longer active)]

It would be great if someone can find the hg range for this regression, and try to bisect that range.

Comment 4

[rsx11m]

Setting platform to "All" as bug 615377 was filed for Mac OSX.

Comment 5

[rsx11m]

Additional observations (testing 20101128 trunk build on Windows):

- This also happens when defining a signature in the textarea of the Account
  Settings, with or without "Use HTML" checked -> not restricted to HTML sig
  or reading the signature from a file;

- highlighting the message and checking in Insert > HTML shows the signature
  clearly being located within the <blockquote> construct;

- not a problem with "start my reply above the quote" and "place my signature
  below my reply (above the quote)"

- when composing in plain-text format, the signature is correctly placed
  outside the quoted text.

I've added the mozilla-central pushlog for the regression range as URL, there is nothing suspicious I can find in the comm-central logs.

Comment 6

[rsx11m]

http://mxr.mozilla.org/comm-central/ident?i=ConvertAndLoadComposeWindow in nsMsgCompose.cpp#528 may be a good starting point, which receives the original message and the formatted signature as two parameters, from there invoking nsHTMLEditor::InsertAsCitedQuotation() at #588 in the HTML-composition case.

Comment 7

[Joe Sabash [:JoeS1]]

Bug 611182 is dealing with EndOFDocument which might explain why "start my
reply above the quote" and "place my signature below my reply (above the
quote)" is not affected.

Comment 8

[rsx11m]

That was pushed Nov 14 13:28:20, thus would match the regression range.
http://hg.mozilla.org/mozilla-central/rev/a9f682f84cab

Maybe the <blockquote> is left open after InsertAsCitedQuotation() somehow
and only closed after the signature block was added to the end? Or, whatever nsMsgCompose is doing was wrong to start with and now has been uncovered.

Comment 9

[(no longer active)]

(In reply to comment #8)
> That was pushed Nov 14 13:28:20, thus would match the regression range.
> http://hg.mozilla.org/mozilla-central/rev/a9f682f84cab
> 
> Maybe the <blockquote> is left open after InsertAsCitedQuotation() somehow
> and only closed after the signature block was added to the end? Or, whatever
> nsMsgCompose is doing was wrong to start with and now has been uncovered.

If you back out this changeset locally, does the problem occur for those two cases as well?

Comment 10

[rsx11m]

My trunk build is too old and needs clobbering/rebuild from scratch, I don't quite have the time for that now. Anybody else in a better position to try?

Comment 11

[Mark Banner (:standard8)]

(In reply to comment #10)
> My trunk build is too old and needs clobbering/rebuild from scratch, I don't
> quite have the time for that now. Anybody else in a better position to try?

I've just started try server on doing a build with that patch backed out, it should appear here:

http://ftp.mozilla.org/pub/mozilla.org/thunderbird/tryserver-builds/[e-mail]-479ff107ae97

If you want the Mac build, it may take about 6 hours for that to cycle through as the builder is currently busy and we've only got one of them.

Comment 12

[Mark Banner (:standard8)]

Sorry, I messed the link up, it will be:

http://ftp.mozilla.org/pub/mozilla.org/thunderbird/tryserver-builds/[e-mail]-a6991ff52c64

Comment 13

[rsx11m]

Thanks Mark, I've just tried the Linux build which was ready first:

Output from today's (respun) nightly build -

RSX11M wrote:
<blockquote cite="mid:..." type="cite">
  <meta http-equiv="content-type" content="text/html;
    charset=ISO-8859-1">
  Test message<br>
  <br>
  <div class="moz-signature">-- <br>
    Test signature</div>
</blockquote>
<br>

Output from the try-server build with a9f682f84cab backed out -

RSX11M wrote:
<blockquote cite="mid:..." type="cite">
  <meta http-equiv="content-type" content="text/html;
    charset=ISO-8859-1">
  Test message<br>
</blockquote>
<br>
<br>
<div class="moz-signature">-- <br>
  Test signature</div>

Thus, it's safe to say that this part of bug 611182 indeed caused the regression in mail/news composition reported here.

Comment 14

[rsx11m]

... or at least triggered this bug, to phrase the conclusion more carefully.

Interesting to see that the signature and the closing </blockquote> are
squeezed between the two <br> line breaks, maybe this provides a hint where
to look for the underlying issue.

Comment 15

[(no longer active)]

Mark, can you debug this?  I'm interested in calls to nsEditor::EndOfDocument, what the call stack looks like, and where it puts the selection on, and also what the last child of the root element is (which is what used to be returned by that method)...

Comment 16

[Joe Sabash [:JoeS1]]

A Sig is not a Sig if it appears in the quote.
Bumping severity, as this is a loss of a major feature.

Comment 17

[[e-mail]]

InsertAsCitedQuotation actually leaves the caret in "virtual space" between the </blockquote> and the </body>. Presumably the EndOfDocument() change moves it "back" to the "correct" location inside the </blockquote>, which causes the signature to insert inside the quotation.

Comment 18

[[e-mail]]

Attachment: Possible patch

This is a bit fragile because it assumes that InsertAsCitedQuotation leaves the caret in the correct place for our purposes. I guess as an alternative I could copy the code from later on in the method that forces the caret to the correct place for us to insert the signature outside of the quotation.

The other two deletions were really to make things easier to debug, but since undo is off anyway there's no point creating a transaction.

Comment 19

[David :Bienvenu]

Comment on attachment 497507 [details] [diff] [review]
Possible patch

thx, I tried this and it does fix the bug - can you either add a comment in the code to the effect of "it assumes that InsertAsCitedQuotation leaves the
caret in the correct place for our purposes" or try your other suggestion (though I think jumping the cursor around more than required might be a bit slow)

Comment 20

[[e-mail]]

Pushed changeset 0d0dd81c5eab to comm-central.

Sadly I botched the comment and wrote bug 614949 by mistake. Oops.

Comment 21

[Joe Sabash [:JoeS1]]

verified with Mozilla/5.0 (Windows NT 5.0; rv:2.0b9pre) Gecko/20101226 Thunderbird/3.3a2pre ID:20101226030013
Thanks for the Christmas present.

Comment 24

[Lewis Rosenthal]

Could I possibly solicit one or more of you kind folks to have a look at Bug 703086 and tell me if what I reported there is really a dupe of this, and whether we can reopen this one?

TIA