Closed Bug 1792551 Opened 2 months ago Closed 2 months ago

MailExtension getComposeDetails() returns plainTextBody with no trailing spaces and redundant leading spaces since change b96d9df1eacc2092114ac04dcbed3a6cc4612043

Categories

(Thunderbird :: Add-Ons: Extensions API, defect)

Thunderbird 102
defect

Tracking

(thunderbird_esr102+ fixed, thunderbird106 affected)

RESOLVED FIXED
107 Branch
Tracking Status
thunderbird_esr102 + fixed
thunderbird106 --- affected

People

(Reporter: frederick888, Assigned: TbSync)

Details

Attachments

(1 file)

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:106.0) Gecko/20100101 Firefox/106.0

Steps to reproduce:

Calling messenger.compose.getComposeDetails(tab.id)

The compose window is in plain text mode, and the original text is

123456␣
␣Hello␣

(␣ denotes a regular space.)

Actual results:

plainTextBody from the returning value has no trailing spaces and redundant leading spaces:

123456
␣␣Hello

Expected results:

Prior to b96d9df1eacc2092114ac04dcbed3a6cc4612043, we used editor.outputToString("text/plain", Ci.nsIDocumentEncoder.OutputRaw) as plainTextBody.

(In Thunderbird 102.3.0 console:)

temp0.IsHTMLEditor()
false
editor = temp0.GetCurrentEditor()
editor.outputToString('text/plain', Ci.nsIDocumentEncoder.OutputRaw)
"123456␣
␣Hello␣
"

After this change we started using MsgUtils.convertToPlainText(htmlBody, true) instead:

htmlBody = trimContent(editor.outputToString('text/html', Ci.nsIDocumentEncoder.OutputRaw))
"<html><head><meta http-equiv="Content-Type" content="text/html; charset=UTF-8"><style></style></head><body style="font-family: -moz-fixed; white-space: pre-wrap;">123456 <br> Hello <br></body></html>"
parserUtils.convertToPlainText(htmlBody, Ci.nsIDocumentEncoder.OutputPersistNBSP | Ci.nsIDocumentEncoder.OutputFormatted | Ci.nsIDocumentEncoder.OutputDisallowLineBreaking | Ci.nsIDocumentEncoder.OutputFormatFlowed, 990)
"123456
␣␣Hello
"
parserUtils.convertToPlainText(htmlBody, Ci.nsIDocumentEncoder.OutputPersistNBSP | Ci.nsIDocumentEncoder.OutputFormatted | Ci.nsIDocumentEncoder.OutputDisallowLineBreaking, 72) // with formatFlowed = false, there are no more redundant leading spaces but trailing ones are still removed
"123456
␣Hello
"

IMHO when composing in plain text, plainTextBody should be exactly the same as what the user has in the compose window editor.

See Bug https://bugzilla.mozilla.org/show_bug.cgi?id=1692439#c3 and the following comments. We will not be able to revert, but I have a look at the strange double space. The trimmed trailing space can probably be reverted by removing the outer trim.

Thank you for the quick response, John!

A user reported https://github.com/Frederick888/external-editor-revived/issues/61 to me and I really appreciate the help :)

Ok, the request is to return the content of TEXT composers "as-is", but I think we should keep unifying the line ending. IIRC, we always return \n line ending regardless of the used OS. I would like to keep doing that.

The proposed way to fix this is:

let plainTextBody = composeWindow.IsHTMLEditor()
    ? trimContent(MsgUtils.convertToPlainText(body, true))
    : parserUtils.convertToPlainText(body, Ci.nsIDocumentEncoder.OutputLFLineBreak, 0);

So we are not changing the return value for HTML composers and keep doing what we are currently doing. For TEXT composer, we could directly use parserUtils.convertToPlainText() but without most flags (MsgUtils.convertToPlainText() is using this as well, but with a different set of flags).

Objections?

Assignee: nobody → john
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true

Here is a try run:
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=e639fce92fd81795b9c9a5fb919c75abbe014e4f

From the green Ba items you can download a built (Artifacts and Debug Tools tab -> target.zip / target.tar.bz2) to test the patch.

@John Thank you for the fix!

but I think we should keep unifying the line ending

That doesn't really concern me since I always convert the body to CRLF in my extension :)

Though one issue that I noticed is that there's now a new blank line added to plainTextBody due to an extra <br> in the HTML:

Original text:

123456␣
␣Hello␣

Tested in both 102.3.0 and the test build:
$ body = trimContent(editor.outputToString("text/html", Ci.nsIDocumentEncoder.OutputRaw))
"<!DOCTYPE html>
<html><head><meta http-equiv="Content-Type" content="text/html; charset=UTF-8"><style></style></head><body style="font-family: -moz-fixed; white-space: pre-wrap;">123456 <br> Hello <br></body></html>"
$ JSON.stringify(parserUtils.convertToPlainText(body, Ci.nsIDocumentEncoder.OutputLFLineBreak, 0))
""123456 \n Hello \n""
$ compared with the old implementation
""123456 \n Hello ""

Sorry, the 'old implementation' output was from

JSON.stringify(editor.outputToString('text/plain', Ci.nsIDocumentEncoder.OutputRaw))

It looks like we can simply remove the last line break, if there is one. If you intentionally added a line break at the end, there will be two. I update the tests to cover this.
https://hg.mozilla.org/try-comm-central/rev/6ad7ee025f9f6f6af5a73b180521a7a3c01fbd1c#l1.42

I think we are good for check-in:
https://treeherder.mozilla.org/jobs?repo=try-comm-central&collapsedPushes=1128357%2C1128350%2C1128348%2C1128024%2C1128395%2C1128394&revision=a3e5d92893271dd4c811a307e86d237cc05c6c79

Yup that LGTM. Thanks again :)

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/42872bef2ab8
Return plain text content as-is. r=mkmelin

Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → 107 Branch

Comment on attachment 9296467 [details]
Bug 1792551 - Return plain text content as-is. r=mkmelin

[Approval Request Comment]
Regression caused by (bug #):
User impact if declined:
Wrongly returned spaces and newlines.

Testing completed (on c-c, etc.):
On Beta for more than 30 days.

Risk to taking this patch (and alternatives if risky):
Low.

Attachment #9296467 - Flags: approval-comm-esr102?

Comment on attachment 9296467 [details]
Bug 1792551 - Return plain text content as-is. r=mkmelin

[Triage Comment]
Approved for esr102

Attachment #9296467 - Flags: approval-comm-esr102? → approval-comm-esr102+
You need to log in before you can comment on or make changes to this bug.