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

RESOLVED FIXED in Thunderbird 46.0

Status

Thunderbird
General
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: Jorg K (GMT+2), Assigned: Jorg K (GMT+2))

Tracking

Trunk
Thunderbird 46.0

Thunderbird Tracking Flags

(thunderbird45 fixed, thunderbird46 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

a year ago
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.
(Assignee)

Comment 1

a year ago
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?
Flags: needinfo?(mkmelin+mozilla)
(Assignee)

Comment 2

a year ago
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.
(Assignee)

Comment 3

a year ago
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?
(Assignee)

Comment 4

a year ago
Created attachment 8698074 [details] [diff] [review]
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.
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
Flags: needinfo?(mkmelin+mozilla)
Attachment #8698074 - Flags: feedback?(ehsan)
(Assignee)

Comment 5

a year ago
Created attachment 8698135 [details] [diff] [review]
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
Attachment #8698074 - Attachment is obsolete: true
Attachment #8698074 - Flags: feedback?(ehsan)
Attachment #8698135 - Flags: review?(mkmelin+mozilla)

Comment 6

a year ago
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??)
(Assignee)

Comment 7

a year ago
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

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

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

Yes let's do this. r=mkmelin
Attachment #8698135 - Flags: review?(mkmelin+mozilla) → review+
(Assignee)

Comment 9

a year ago
Thanks. (I don't want to repeat that I think that this is the right decision ;-).)
Keywords: checkin-needed
(Assignee)

Comment 10

a year ago
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.
Attachment #8698135 - Flags: approval-comm-aurora?
(Assignee)

Updated

a year ago
Attachment #8698135 - Flags: approval-comm-aurora? → approval-comm-aurora+

Comment 11

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

Updated

a year ago
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 46.0
(Assignee)

Comment 12

a year ago
Aurora:
https://hg.mozilla.org/releases/comm-aurora/rev/ea0c3e39de3e
status-thunderbird45: --- → fixed
status-thunderbird46: --- → fixed
You need to log in before you can comment on or make changes to this bug.