Closed Bug 1231917 Opened 5 years ago Closed 5 years ago

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


(Thunderbird :: General, defect)

Not set


(thunderbird45 fixed, thunderbird46 fixed)

Thunderbird 46.0
Tracking Status
thunderbird45 --- fixed
thunderbird46 --- fixed


(Reporter: jorgk-bmo, Assigned: jorgk-bmo)



(1 file, 1 obsolete file)

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:

See attachment 8697405 [details] for a screenshot.
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:


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)
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.
Some detective work:
Replying to the message after importing/dragging it into a folder runs InsertAsCitedQuotation:
Called function:

Opening a message from a saved file and replying to it runs InsertAsQuotation:
Called function:

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:
We note the comment "// XXX ignore charset" on the call to InsertText.

InsertText lives in the plaintext editor:

This call seems just plain wrong. Why use the plaintext editor in a HTML context?
Attached patch Patch for discussion (v1). (obsolete) — Splinter Review
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:

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:
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
Flags: needinfo?(mkmelin+mozilla)
Attachment #8698074 - Flags: feedback?(ehsan)
Maybe we better fix this in C-C and make it look like the code here:
Attachment #8698074 - Attachment is obsolete: true
Attachment #8698074 - Flags: feedback?(ehsan)
Attachment #8698135 - Flags: review?(mkmelin+mozilla)
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:

(maybe something like the patch in bug 529735 would be needed if so??)
The citereference may be a secondary problem.

The real problem is that the code is just plain wrong. It should read like this:

 if (aHTMLEditor)
        mailEditor->InsertAsCitedQuotation(mMsgBody, EmptyString(), true,
        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:

If you erroneously call InsertAsQuotation() with a HTML editor, as currently happens if the cite is empty, you get here:
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:

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 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+
Thanks. (I don't want to repeat that I think that this is the right decision ;-).)
Keywords: checkin-needed
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?
Attachment #8698135 - Flags: approval-comm-aurora? → approval-comm-aurora+
Bug 1231917 - Correct condition controling call to InsertAsCitedQuotation. r=mkmelin
Closed: 5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 46.0
You need to log in before you can comment on or make changes to this bug.