Closed Bug 614919 Opened 12 years ago Closed 12 years ago

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

Categories

(MailNews Core :: Composition, defect)

defect
Not set
major

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: Aureliano, Assigned: neil)

References

()

Details

(Keywords: regression, testcase, Whiteboard: 20101114 (work) --- 20101115 (broken))

Attachments

(2 files)

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
Whiteboard: 20101112 (work) --- 20101115 (broken)
Attached file signature testcase
attached signature testcase
Keywords: testcase
Summary: Reply to HTML message with account that use HTML signature file, quote my signature → Reply to HTML message with account that use HTML signature file, quote my signature (builds broken starts 20101115)
Whiteboard: 20101112 (work) --- 20101115 (broken) → 20101114 (work) --- 20101115 (broken)
Component: Message Compose Window → Composition
Product: Thunderbird → MailNews Core
QA Contact: message-compose → composition
It would be great if someone can find the hg range for this regression, and try to bisect that range.
Setting platform to "All" as bug 615377 was filed for Mac OSX.
OS: Windows 7 → All
Hardware: x86 → All
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.
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.
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.
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.
(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?
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?
(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/bugzilla@standard8.plus.com-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.
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.
Blocks: 611182
... 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.
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)...
A Sig is not a Sig if it appears in the quote.
Bumping severity, as this is a loss of a major feature.
Severity: normal → major
blocking-thunderbird5.0: --- → ?
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.
Attached patch Possible patchSplinter Review
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.
Assignee: nobody → neil
Status: NEW → ASSIGNED
Attachment #497507 - Flags: review?(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)
Attachment #497507 - Flags: review?(bienvenu) → review+
Pushed changeset 0d0dd81c5eab to comm-central.

Sadly I botched the comment and wrote bug 614949 by mistake. Oops.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
verified with Mozilla/5.0 (Windows NT 5.0; rv:2.0b9pre) Gecko/20101226 Thunderbird/3.3a2pre ID:20101226030013
Thanks for the Christmas present.
Status: RESOLVED → VERIFIED
blocking-thunderbird5.0: ? → ---
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
You need to log in before you can comment on or make changes to this bug.