Closed Bug 1658132 Opened 4 years ago Closed 3 years ago

compose API doesn't create HTML email for plain text identity

Categories

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

Product:
Thunderbird
Component:
Add-Ons: Extensions API
Type:
defect

Tracking

(thunderbird_esr78+ fixed, thunderbird84 affected)

Status:
RESOLVED FIXED
Milestone:
85 Branch
Tracking Flags:
Tracking Status
thunderbird_esr78 + fixed
thunderbird84 --- affected

People

(Reporter: janek, Assigned: TbSync)

References

Details

Attachments

(1 file, 5 obsolete files)

bug1658132_properly_handle_isPlainText_in_compoe_API_v3.patch
14.07 KB, patch
darktrojan
: review+
wsmwk
: approval-comm-esr78+
Details | Diff | Splinter Review

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:79.0) Gecko/20100101 Firefox/79.0

Steps to reproduce:

When an identity is configured to not use html (pref: mail.identity.id1.compose_html is set to false) a call to

    browser.compose.beginNew({
      "identityId": identityId,
      "isPlainText" : false,
      "body": `<html>

<head>
<meta http-equiv="content-type" content="text/html; charset=UTF-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<p>Test<br>
</p>
</body>
</html>`
});

wont open an html composer.

I'm porting my addon Identity Chooser[1] and need to replicate the behaviour of the default compose button which in the described case would open a html composer when shift-clicking on the compose button.

[1] https://addons.thunderbird.net/de/thunderbird/addon/identity-chooser/

Actual results:

A compose window is opened with the html text included verbatim as plain text

Expected results:

A compose window with html enabled rendering the html text.

This could be fixed by setting the format explicitely in ext-compose.js, starting line 146 to:

if (details.body !== null) {
  composeFields.body = details.body;
  params.format = Ci.nsIMsgCompFormat.HTML;
}
Summary: compose API doesn't create HTML email for plain text account → compose API doesn't create HTML email for plain text identity
Assignee: nobody → john
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true

fix a spelling error

Attachment #9189617 - Attachment is obsolete: true
Attachment #9189620 - Flags: review?(geoff)

Please extend browser_ext_compose_begin.js to test this. It's already got a second identity you can change to plain text, then add a few more cases to testBody.

Comment on attachment 9189620 [details] [diff] [review]
bug1658132_override_default_message_format_if_isPlainText_is_set.patch

Found a few issues while working on the test. This needs some more tweaking.

Attachment #9189620 - Flags: review?(geoff)

I am running into issues related to bug 1513824, of course. I currently test for the following scenarios:

      let funcTests = [
        /* - */{ name: "beginNew" },
        /* 1 */{ name: "beginNew", mId: messages[0].id },
        /* - */{ name: "beginReply", mId: messages[0].id },
        /* - */{ name: "beginReply", mId: messages[1].id, type: "replyToSender" },
        /* - */{ name: "beginReply", mId: messages[2].id, type: "replyToList" },
        /* - */{ name: "beginReply", mId: messages[3].id, type: "replyToAll" },
        /* 2 */{ name: "beginForward", mId: messages[0].id },
        /* 2 */{ name: "beginForward", mId: messages[1].id, type: "forwardInline" },
        /* - */{ name: "beginForward", mId: messages[2].id, type: "forwardAsAttachment" },
      ];

The tests marked with 1 and 2 take "the other" code path (this one) and they have the following issues:

  • they ignore any plaintext/html setting and always use plaintext (even the default one is ignored)
  • they crash(!) if also a body/plainTextBody is set
[JavaScript Error: "[Exception... "Unexpected error"  nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)"  location: "JS frame :: chrome://messenger/content/messengercompose/MsgComposeCommands.js :: SetComposeDetails :: line 4383"  data: no]"]
SetComposeDetails@chrome://messenger/content/messengercompose/MsgComposeCommands.js:4383:24
setComposeDetails@chrome://messenger/content/parent/ext-compose.js:294:17
InitEditor@chrome://messenger/content/messengercompose/MsgComposeCommands.js:8629:10
observe@chrome://messenger/content/messengercompose/MsgComposeCommands.js:3778:9

Moving them into the other code path will result in empty emails, as you probably know. So that is currently not an option.

I will try to understand what is going on, as Thunderbird itself is doing at least the plaintext/html setting correctly.

For the body crash: One could forbid setting the body in template mode to get around that.

Current work, so it does not get lost.

Attachment #9189620 - Attachment is obsolete: true
See Also: → 1617377

This patch fixes the reported bug and also adds support for details.isPlainText for EditAsNew and ForwardInline, which use a different code path where it was ignored completely so far.

I moved a bit of code around to reduce redundancy. I also modified the test:

  • I added tests for the reported case, for the EditAsNew cases and for one ForwardInline case.
  • I replaced createAccount() by MailServices.accounts.createAccount() which allows to set a default account. The default account is needed to properly test compose format for EditAsNew and ForwardInline, if the default compose format of the default identity is used (the used mimeConverter falls back to text if it cannot find a default account in that case). I also adjusted the expected result of two already existing texts, which were set to text because of this, and now properly get html.
  • The new MailServices.accounts.createAccount() function creates a "Local Folder" account, tests for the number of accounts have been adjusted.
Attachment #9189824 - Attachment is obsolete: true
Attachment #9190166 - Flags: review?(geoff)
Comment on attachment 9190166 [details] [diff] [review]
bug1658132_properly_handle_isPlainText_in_compoe_API.patch

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

Things to work on. Nice work figuring all of this out. One day we'll fix some of the underlying problems that make it so complicated.

::: mail/components/extensions/parent/ext-compose.js
@@ +78,5 @@
>        Services.obs.addObserver(observer, "chrome-document-loaded");
>      });
>    }
>  
> +  function getDefaultComposeFormat(aIdentity) {

Probably doesn't need to be a function. You only use it once.

@@ +144,5 @@
> +      // This is a bit of madness. OpenComposeWindow() for the types in this
> +      // code path only supports to toggle the formt from default, but does not
> +      // allow to set HTML or PlainText directly. Check if OppositeOfDefault is
> +      // needed or not.
> +      // https://searchfox.org/comm-central/rev/5007c6f56b3408f49c1efc203afc2d8ca8552610/mailnews/compose/src/nsMsgComposeService.cpp#395

I reworded your comment:

For the types in this code path, OpenComposeWindow only uses nsIMsgCompFormat.Default or OppositeOfDefault. Check which is needed. See
https://hg.mozilla.org/comm-central/file/592fb5c396ebbb75d4acd1f1287a26f56f4164b3/mailnews/compose/src/nsMsgComposeService.cpp#l395

Use hg.m.o when linking like this as it's the canonical source of the code. It's fine to use searchfox links day to day, but the code is permanent.

Also, please move this comment and the calculation outside of the function call, this is too messy.

::: mail/components/extensions/test/browser/browser_ext_compose_begin.js
@@ +10,5 @@
> +account.incomingServer = acctMgr.createIncomingServer(
> +  "nobody",
> +  "compose testing",
> +  "pop3"
> +);

Take this and replace what's in head.js with it. I've always intended the two to be the same but it's one of those things I've never gotten around to doing. Then you just need createAccount("pop3").

https://searchfox.org/comm-central/rev/bfc7644683950de390d29602ccf43c7fc358491e/mail/components/extensions/test/xpcshell/head.js#30-37,45-56

@@ +20,5 @@
>  let folder = rootFolder.getChildNamed("test");
>  createMessages(folder, 4);
> +MailServices.accounts.defaultAccount = account;
> +defaultIdentity.composeHtml = true;
> +nonDefaultIdentity.composeHtml = false;

Let's put these lines further up next to where each variables is created.

@@ +27,5 @@
>    let files = {
>      "background.js": async () => {
> +      let accounts = await browser.accounts.list();
> +      let [defaultIdentity, nonDefaultIdentity] = accounts[0].identities;
> +      let folder = accounts[0].folders.find(f => f.name == "test");

This does the same thing! (Okay, so you're not reusing the name `account`, but this code doesn't run in the same scope as the code outside it anyway.)

@@ +260,5 @@
>    let files = {
>      "background.js": async () => {
>        let accounts = await browser.accounts.list();
> +      browser.test.assertEq(2, accounts.length, "number of accounts");
> +      let identities = accounts[0].identities;

To make this easier to debug I'd call them htmlIdentity and plainTextIdentity, or something like that.
Attachment #9190166 - Flags: review?(geoff)
Depends on: 1679901

This is assuming bug 1679901 has landed.

I did all the requested changes, except one where I use the full accounts array, to check for its length. I reverted to the original code, where that is not needed.

Attachment #9190166 - Attachment is obsolete: true
Attachment #9190468 - Flags: review?(geoff)

Altered a test to detect another edge case.

This is assuming bug 1679901 has landed.

I did all the requested changes, except one where I use the full accounts array, to check for its length. I reverted to the original code, where that is not needed.

Attachment #9190468 - Attachment is obsolete: true
Attachment #9190468 - Flags: review?(geoff)
Attachment #9190751 - Flags: review?(geoff)
Attachment #9190751 - Flags: review?(geoff) → review+
status-thunderbird85: affected → ---
tracking-thunderbird84: ? → ---
tracking-thunderbird85: ? → ---
tracking-thunderbird_esr78: ? → +
Target Milestone: --- → 85 Branch

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/0b8d4165349f
properly handle isPlainText in compose API begin* functions. r=darktrojan

Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Keywords: checkin-needed-tb
Resolution: --- → FIXED

We typically don't set the tracking flag for current milestone. Clearing 84 tracking since I think there are no more betas planned for 84.

For whatever reason, this seem to have causing a bunch of bc2 failures. Doesn't fail locally for individual runs, but does fail for ./mach test --headless comm/mail/components/extensions/test/browser/

Locally backing this out I don't get the same failures, though one remained:
comm/mail/components/extensions/test/browser/browser_ext_windows.js FAIL undefined - Expected: -1, Actual: 3 - Stack trace: chrome://mochikit/content/browser-test.js:test_ok:1304 chrome://mochikit/content/tests/SimpleTest/ExtensionTestUtils.js:testHandler:68 chrome://mochikit/content/tests/SimpleTest/ExtensionTestUtils.js:testResult:82 resource://specialpowers/SpecialPowersChild.jsm:listener:1975 resource://specialpowers/SpecialPowersChild.jsm:loadExtension/<:1913 resource://specialpowers/SpecialPowersChild.jsm:receiveMessage:267 FAIL Test timed out - FAIL no tasks awaiting on messages - Got "[\"closeWindow\"]", expected "[]" Stack trace: chrome://mochikit/content/browser-test.js:test_is:1332 chrome://mochikit/content/tests/SimpleTest/ExtensionTestUtils.js:ExtensionTestUtils.loadExtension/<:36 chrome://mochikit/content/browser-test.js:nextTest:555 FAIL Extension left running at test shutdown - Stack trace: chrome://mochikit/content/browser-test.js:test_ok:1304 chrome://mochikit/content/tests/SimpleTest/ExtensionTestUtils.js:ExtensionTestUtils.loadExtension/<:117 chrome://mochikit/content/browser-test.js:nextTest:555 FAIL Found a mail window after previous test timed out -

I'll back it out for now.

Backout: https://hg.mozilla.org/comm-central/rev/d007e2c72f0ed6e7e6f6adfa56177353c144aefd

Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Depends on: 1680653

I found the cause for this and Geoff fixed it in Bug 1680653. As soon as that is checked in, this can be checked in again as well. Here is a try run with both patches applied to c-c:
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=16aab9cbb19656944a7bfa05b0bcdd02b0bd254d

Keywords: checkin-needed-tb

Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/1575ce118ad1
properly handle isPlainText in compose API begin* functions. r=darktrojan

Status: REOPENED → RESOLVED
Closed: 3 years ago → 3 years ago
Keywords: checkin-needed-tb
Resolution: --- → FIXED

Comment on attachment 9190751 [details] [diff] [review]
bug1658132_properly_handle_isPlainText_in_compoe_API_v3.patch

[Approval Request Comment]
User impact if declined:
Add-on developers can not use the latest fixes and improvements of our WebExtension API.

Testing completed (on c-c, etc.):
https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=3cdff293d5d1bd77777f5da371caaa02c4b05de3

Risk to taking this patch (and alternatives if risky):
I hope none

Remark:
The provided try run includes all bugs I want to uplift for TB 78.7 and shows a working patch order.
The first one, which does not have a bug number in the commit messages is Bug 1680653.
This bug is the 7th one.

Attachment #9190751 - Flags: approval-comm-esr78?

Comment on attachment 9190751 [details] [diff] [review]
bug1658132_properly_handle_isPlainText_in_compoe_API_v3.patch

[Triage Comment]
Approved for esr78

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

Attachment

General

Creator:
Created:
Updated:
Size: