1231917 - Opening a CJK e-mail from the desktop or command line and replying to it inserts HTML tags into the composition

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Jorg K (CEST = GMT+2)]

Opening a CJK e-mail from the desktop or command line and replying to it inserts HTML tags into the composition.

Place attachment 8696494 [details] onto the desktop and double click it (or open it from the command line, Magnus knows how).

Reply in HTML mode. At the end of the quote this text is inserted:
<br></body>
</html>
</html>

See attachment 8697405 [details] for a screenshot.

Comment 1

[Jorg K (CEST = GMT+2)]

As I said elsewhere the strange effect doesn't happen when dragging/importing the message into a folder first before answering. Magnus, can you confirm this?

I opened the imported message in a single window and compared that to the message opened directly in the DOM inspector:

mailbox:///D:/MAIL-THUNDERBIRD/jorgk.default/Mail/jorgk.jorgk.com/Sent.sbd/Test%20messages?number=14145535
and 
mailbox:///D:/Desktop/longline-ja-JP.eml?type=application/x-message-display&number=0

Both look the same. Yet, replying to the one from the folder works, replying to the message opened directly creates the ugly tags. Quite puzzling. Do you have any hints?

Comment 2

[Jorg K (CEST = GMT+2)]

One more thing: Try this in TB 38.4:
File > Open Saved Message...
Then (HTML) reply. Same ugly tags get inserted.
Note that delsp=yes support for reading messages was already implemented in bug 231701. That's why you can open and reply to the message even in TB 38.4.

In summary: This is an old bug, nothing that went broken with recent CJK fixes.

Comment 3

[Jorg K (CEST = GMT+2)]

Some detective work:
Replying to the message after importing/dragging it into a folder runs InsertAsCitedQuotation:
Caller:
https://dxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgCompose.cpp#683
Called function:
https://dxr.mozilla.org/comm-central/source/mozilla/editor/libeditor/nsHTMLDataTransfer.cpp#1845

Opening a message from a saved file and replying to it runs InsertAsQuotation:
Caller:
https://dxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgCompose.cpp#688
Called function:
https://dxr.mozilla.org/comm-central/source/mozilla/editor/libeditor/nsHTMLDataTransfer.cpp#

Note that the called functions are part of the M-C editor, so here we're looking at an editor bug.

Internally InsertAsQuotation calls InsertAsCitedQuotation with parameter aInsertHTML set to 'false'. Then inside InsertAsCitedQuotation this parameter determines whether LoadHTML or InsertText is called:
https://dxr.mozilla.org/comm-central/source/mozilla/editor/libeditor/nsHTMLDataTransfer.cpp#1891
https://dxr.mozilla.org/comm-central/source/mozilla/editor/libeditor/nsHTMLDataTransfer.cpp#1893
We note the comment "// XXX ignore charset" on the call to InsertText.

InsertText lives in the plaintext editor:
https://dxr.mozilla.org/comm-central/source/mozilla/editor/libeditor/nsPlaintextEditor.cpp#709

This call seems just plain wrong. Why use the plaintext editor in a HTML context?

Comment 4

[Jorg K (CEST = GMT+2)]

Attachment: Patch for discussion (v1).

This patch fixes the problem observed in Thunderbird. When replying to an e-mail under certain circumstances, nsHTMLEditor::InsertAsQuotation is called, which calls nsHTMLEditor::InsertAsCitedQuotation. Based on the parameter aInsertHTML, this funtion later calls LoadHTML or InsertText. The call to InsertText appears questionable, IMHO LoadHTML should always be called.

At least in the Thunderbird test case, this seems to be the right thing to do.

The problem we're seeing is that additionally to the text we want to insert, we get this text added:
<br></body>
</html>
</html>

Please take a look at the screenshot in attachment 8697405 [details] (attached to another bug), so see how funny that looks.

Ehsan, do you know why there is this distinction? Can we always call LoadHTML? If so, I'll run a try run to see whether this breaks anything, but it looks like nsHTMLEditor::InsertAsCitedQuotation is only used by C-C.

An alternative fix would be to not call InsertAsQuotation but call InsertAsCitedQuotation as is already done here:
https://dxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgCompose.cpp#3009
The author must have known that nsHTMLEditor::InsertAsPlaintextQuotation doesn't work.

So two ways to fix the bug: Fix the faulty M-C function or not call it any more from C-C code and call the one that works instead.

Comment 5

[Jorg K (CEST = GMT+2)]

Attachment: Proposed solution (v1).

Maybe we better fix this in C-C and make it look like the code here:
https://dxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgCompose.cpp#3008

Comment 6

[Magnus Melin [:mkmelin]]

Comment on attachment 8698135 [details] [diff] [review]
Proposed solution (v1).

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

If the problem is the citereference is empty, why is that?
Seems it's set here: http://mxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgCompose.cpp#2206

(maybe something like the patch in bug 529735 would be needed if so??)

Comment 7

[Jorg K (CEST = GMT+2)]

The citereference may be a secondary problem.

The real problem is that the code is just plain wrong. It should read like this:
https://dxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgCompose.cpp#3008

 if (aHTMLEditor)
        mailEditor->InsertAsCitedQuotation(mMsgBody, EmptyString(), true,
                                           getter_AddRefs(nodeInserted));
      else
        mailEditor->InsertAsQuotation(mMsgBody, getter_AddRefs(nodeInserted));

For an HTML editor *always* call InsertAsCitedQuotation, it can cope with empty cites.

Here is what happens:
If you call InsertAsCitedQuotation() you get here:
https://dxr.mozilla.org/comm-central/source/mozilla/editor/libeditor/nsHTMLDataTransfer.cpp#1845

If you erroneously call InsertAsQuotation() with a HTML editor, as currently happens if the cite is empty, you get here:
https://dxr.mozilla.org/comm-central/source/mozilla/editor/libeditor/nsHTMLDataTransfer.cpp#1744
This also calls InsertAsCitedQuotation() but passes aInsertHTML==false, and that's the problem.

The HTML your inserting is inserted as plaintext and that's when the tags start showing up as text. This happens here:
https://dxr.mozilla.org/comm-central/source/mozilla/editor/libeditor/nsHTMLDataTransfer.cpp#1890.

So my fix of removing part of the condition is right.

If you want to chase the citereference, feel free to do so elsewhere. I'm concerned with the underlying editor problem. And inserting HTML as text ain't going down well ;-)

Comment 8

[Magnus Melin [:mkmelin]]

Comment on attachment 8698135 [details] [diff] [review]
Proposed solution (v1).

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

Yes let's do this. r=mkmelin

Comment 9

[Jorg K (CEST = GMT+2)]

Thanks. (I don't want to repeat that I think that this is the right decision ;-).)

Comment 10

[Jorg K (CEST = GMT+2)]

Comment on attachment 8698135 [details] [diff] [review]
Proposed solution (v1).

[Approval Request Comment]
Regression caused by (bug #): No regression, was always wrong.
User impact if declined: Spurious HTML tags appear in body text.
Testing completed (on c-c, etc.): Manual testing.
Risk to taking this patch (and alternatives if risky):
No risky, only change an incorrect call to the M-C editor.

Comment 11

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/5ceefe6044d48dcac981e05a53fe9440c726dfa6
Bug 1231917 - Correct condition controling call to InsertAsCitedQuotation. r=mkmelin

Comment 12

[Jorg K (CEST = GMT+2)]

Aurora:
https://hg.mozilla.org/releases/comm-aurora/rev/ea0c3e39de3e