1674830 - compose.setComposeDetails does not detect citations in plaintext

Status: RESOLVED FIXED

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

Description

[jan.kiszka]

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

Steps to reproduce:

In an addon call something like

let details = await messenger.compose.getComposeDetails(tab.id);
messenger.compose.setComposeDetails(tab.id, {
plainTextBody: details.plainTextBody
});

Actual results:

When in plaintext composition mode, citations in the original message are not detected as such and wrap around on long lines. Color highlighting is also removed.

Expected results:

The message in the composer window should appear as it was, permitting programmatic modifications to it without destroying the formatting otherwise.

The reason seems to be that setting plainTextBody is treated like a manual select-all + cut + paste which has the same effect. Instead, citation marks should be detected, and corresponding lines should be treated like on the composer window setup when replying.

See also https://github.com/4ch1m/mozext/issues/15

Comment 1

[John Bieling (:TbSync)]

Plaintext is somewhat special, as it does not contain formatting. So I am not totally surprised that getting the content as plaintext and then re-setting the content via plainTextBody will remove formats.

To understand this better:

In your test case, you worked on an HTML email?
What is the default compose mode of the used identity?

Comment 2

[jan.kiszka]

See the issue I cited. Editing mode is plaintext only.

By now, the extension tries to workaround this API limitation by modifying the DOM tree of the composer window directly (https://github.com/4ch1m/mozext/pull/18). That avoids this issue, but it is fragile as well because it is not in sync with the internal state of TB.

Comment 3

[John Bieling (:TbSync)]

Magnus, there seems to be only one method which is responsible for the quotation color, which is InsertTextWithQuotations(aBuf) in m-c:
https://searchfox.org/comm-central/search?q=insertTextWithQuotations&path=&case=false&regexp=false

It should be used here:
https://searchfox.org/comm-central/rev/36c713a3d0bf0d0ca2208fdf74f8a133d4ed7edd/mail/components/compose/content/MsgComposeCommands.js#5343

But it is not exposed to JS (Error: editor.insertTextWithQuotations is not a function).

Can I add a wrapper to nsMsgCompose.cpp or should I ask m-c to add that function to the idl and expose it to JS?

Comment 4

[Magnus Melin [:mkmelin]]

It would probably be best to expose it in nsIEditorMailSupport
Masayuki, what do you think?

Comment 5

[John Bieling (:TbSync)]

Attachment: Bug 1674830 - Use insertTextWithQuotations() for SetComposeDetails() to improve citation handling. r=mkmelin

Comment 6

[John Bieling (:TbSync)]

I created a patch for m-c in bug 1768057, but did not know who to pick as reviewer. I selected Magnus, hoping he knows who to pick.

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Yes, changing it into a new interface method is fine if it's not called from another method in editor (because editor classes are really complicated so that we need to restrict and clarify the entrances), but as I reviewed, please add new automated tests for every interface method adding newly. Such automated tests prevent regressions at least they test. Otherwise, any changes in mozilla-central could case break such features which are used only in comm-central. I'll add the new one into the table.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/a5da4c3f4345
Use insertTextWithQuotations() for SetComposeDetails() to improve citation handling. r=mkmelin

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/5170990b3aaf
follow-up - Fix linting error. rs=linting

Comment 11

[John Bieling (:TbSync)]

Hi Masayuki,

we encountered an unexpected regression after switching to InsertTextWithQuotations(): If the editor has the Ci.nsIEditor.eEditorReadonlyMask flag set, the function updates the text, but removes the quotation.

We run into this issue, when a user has clicked on the "send" button to send a message. The editor is not closed directly, but is locked and set to read-only. There are a few checks done in the background and if the send process fails, we unlock the editor and the user can continue to edit it. One of these checks is to ask installed add-ons if they want to manipulate the message and if so, update the content using InsertTextWithQuotations() (while the editor is still locked and set to read-only).

I guess we can fix this on our side by lifting the flag before we update the editor. However, I think the behaviour is somewhat inconsistent. Should we file an m-c bug about this?

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Sorry, I missed this needinfo request.

(In reply to John Bieling (:TbSync) from comment #11)

we encountered an unexpected regression after switching to InsertTextWithQuotations(): If the editor has the Ci.nsIEditor.eEditorReadonlyMask flag set, the function updates the text, but removes the quotation.

I guess we can fix this on our side by lifting the flag before we update the editor. However, I think the behaviour is somewhat inconsistent. Should we file an m-c bug about this?

Well, obviously, it's a bug missing to check the readonly state. I'll file a bug by myself and I'll fix it. Thank you for notifying me!

Comment 13

[John Bieling (:TbSync)]

Hi Masayuki, can you link the new bug here, so we know when it has landed? I would like to remove our work-around if it is no longer needed.