Closed Bug 1613534 Opened 5 years ago Closed 5 years ago

Handle message body in browser.compose API functions

Categories

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

Product:
Thunderbird
Component:
Add-Ons: Extensions API
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 75.0

People

(Reporter: darktrojan, Assigned: darktrojan)

References

Details

Attachments

(2 files, 2 obsolete files)

1613534-message-body-3.diff
(deleted), patch
mkmelin
: review+
Details | Diff | Splinter Review
1613534-stray-paragraph-1.diff
(deleted), patch
mkmelin
: review+
Details | Diff | Splinter Review
No description provided.
Blocks: 1613535
Depends on: 1617022
Attached patch 1613534-message-body-1.diff (obsolete) (deleted) — — Splinter Review

This seems too easy. Is it really that easy?

While we're here it seems like a good time to work out what to do with plain-text/HTML choices. At the moment I'm just returning from getComposeDetails with a "bodyIsHTML" flag based on the compose window state, and ignoring the issue in all other places. Presumably the extension should be able to send HTML or plain text and have the composer respond. I don't know if that's feasible or a good idea. Thoughts?

Attachment #9128074 - Flags: feedback?(mkmelin+mozilla)
Comment on attachment 9128074 [details] [diff] [review] 1613534-message-body-1.diff Review of attachment 9128074 [details] [diff] [review]: ----------------------------------------------------------------- Compose format and send format are not the same thing - what we send is determined by other factors (pref, and content). We might not want to support an API for plain text compose.
Attachment #9128074 - Flags: feedback?(mkmelin+mozilla) → feedback+

Hi, A legacy add-on I maintain would be a user of this API on MX migration. I want to provide my add-on as a use case example if it is of some help.

The add-on code that modify the message body is here.

My add-on behaves like following:

  • User can choose either plain-text/HTML in pref of the add-on
  • Gets the mode of the editor via IsHTMLEditor()
  • If the editor is in plain-text mode, regardless of the user choice, the add-on modifies the message body as a plain-text.
  • If the editor is in HTML mode, and user chooses plain-text, the add-on modifies the message body as a plain-text.
  • If the editor is in HTML mode, and user chooses HTML, the add-on modifies the message body as a HTML.

That is, at least in my use case, the functionality to switching between plain-text/HTML is not needed.

My add-on expects that:

  • Getting whether the body is in plain-text or HTML (IsHTMLEditor() utility equivalent)
  • Getting the body as a whole plain-text is always available (outputToString("text/plain"))
  • Getting the body as a whole HTML is always available (outputToString("text/html"))
  • Setting the body as a whole plain-text is always available (insertText equivalent)
  • Setting the body as a whole HTML is available only if the editor is in HTML mode (equivalent to rebuildDocumentFromSource("") followed by insertHTML)
No longer blocks: 1613535
Attached patch 1613534-message-body-2.diff (obsolete) (deleted) — — Splinter Review
Attachment #9128074 - Attachment is obsolete: true
Attachment #9128729 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9128729 [details] [diff] [review] 1613534-message-body-2.diff Review of attachment 9128729 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/components/compose/content/MsgComposeCommands.js @@ +3978,5 @@ > * @param {string} [fields.replyTo] > * @param {string} [fields.newsgroups] > * @param {string} [fields.followupTo] > * @param {string} [fields.subject] > */ Please add the new things to the doc. But looks like https://searchfox.org/comm-central/rev/7ea3d1555e95f8c8f68f82da3772818f508c88cd/mail/components/compose/content/MsgComposeCommands.js#3973-3982 is slightly wrong: newValues vs fields ::: mail/components/extensions/parent/ext-compose.js @@ +104,5 @@ > + if (composeParams.isPlainText) { > + throw new ExtensionError( > + "Cannot specify body when isPlainText is true. Use plainTextBody instead." > + ); > + } maybe it still would be preferable to just have body + isPlainText Or was the idea that both parts should be accessible? I guess for that case it still shouldn't be possible to pass in more than one body. ::: mail/components/extensions/test/browser/browser_ext_compose_begin.js @@ +214,5 @@ > + arguments: { body: "<p>I'm an HTML message!</p>" }, > + expected: { > + isHTML: true, > + html: "<p>I'm an HTML message!</p>", > + plainText: "\nI'm an HTML message!", I guess we should trim the output? I wouldn't expect to get a text starting with a newline from that. ::: mail/components/extensions/test/browser/browser_ext_compose_details.js @@ +370,5 @@ > + browser.test.fail( > + "calling setComposeDetails with these arguments should throw" > + ); > + } catch (ex) { > + browser.test.succeed("expected exception thrown"); maybe add ex.message into there for these. would make it easier to find problems if some other exception is ever thrown
Attachment #9128729 - Flags: review?(mkmelin+mozilla)

(In reply to Magnus Melin [:mkmelin] from comment #5)

maybe it still would be preferable to just have body + isPlainText
Or was the idea that both parts should be accessible? I guess for that case
it still shouldn't be possible to pass in more than one body.

We already have the plainTextBody in the schema due to using the same object type for other things, so I decided to use it here. My thinking is that by forcing the extension writer to do something different (use plainTextBody instead of body) they're less likely to accidentally put HTML where it shouldn't be. They've already had to set isPlainText to get here, so choosing which field to put the body in shouldn't be too unexpected.

I guess we should trim the output? I wouldn't expect to get a text starting
with a newline from that.

Yes, I think there's a defect in the existing code here. I thought this might depend on the current identity's replyOnTop setting, but that isn't the case. I'll file a new bug about it and handle it separately.

Attached patch 1613534-message-body-3.diff (deleted) — — Splinter Review

This mostly the same. I've renamed the html and plainText properties in the test to htmlIncludes and plainTextIs to make it clearer what's being checked. I've figured out where the stray paragraph comes from and will fix that in another patch.

Attachment #9128729 - Attachment is obsolete: true
Attachment #9130926 - Flags: review?(mkmelin+mozilla)
Attached patch 1613534-stray-paragraph-1.diff (deleted) — — Splinter Review
Attachment #9130927 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9130927 [details] [diff] [review] 1613534-stray-paragraph-1.diff Review of attachment 9130927 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, r=mkmelin
Attachment #9130927 - Flags: review?(mkmelin+mozilla) → review+
Keywords: checkin-needed-tb
Comment on attachment 9130926 [details] [diff] [review] 1613534-message-body-3.diff Review of attachment 9130926 [details] [diff] [review]: ----------------------------------------------------------------- ::: mail/components/extensions/test/browser/browser_ext_compose_begin.js @@ +250,5 @@ > + isPlainText: true, > + }, > + expected: { > + isHTML: false, > + htmlIncludes: ">I'm a plain text message!<", shouldn't it have the <p> and </p> ::: mail/components/extensions/test/browser/browser_ext_compose_details.js @@ +317,5 @@ > + htmlDetails = await browser.compose.getComposeDetails(htmlTabId); > + browser.test.log(JSON.stringify(htmlDetails)); > + browser.test.assertTrue(!htmlDetails.isPlainText); > + browser.test.assertTrue( > + htmlDetails.body.includes(">This is some <code>HTML</code> text.<") here too, what's up with the > <
Attachment #9130926 - Flags: review?(mkmelin+mozilla) → review+

Oops, got in a bit early with the checkin-needed. I added the > and < to say I don't really care what the surrounding tag is as long as there's not white-space. I guess in all these cases it probably is <p> with no attributes now.

Keywords: checkin-needed-tb

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/381b9496d886
Handle message body in browser.compose API functions. r=mkmelin
https://hg.mozilla.org/comm-central/rev/42930fd343fe
If a message body is specified, do not insert a paragraph at the start. r=mkmelin

Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED

Actually in most cases the outer tag was <body>, and in plain text messages it has a style attribute. I've included those in the version just landed.

Target Milestone: --- → Thunderbird 75.0
Pushed by geoff@darktrojan.net: https://hg.mozilla.org/comm-central/rev/15b43b265eb4 Strip Windows line endings in compose API tests. rs=bustage-fix

Bah! My follow-up needs a follow-up because it doesn't work on beta. Apparently .replaceAll isn't a thing there yet.

Landed the fix on beta, I'll push it to central when I next push things:

https://hg.mozilla.org/releases/comm-beta/rev/485543f1353223fea59fbc2e781bc0efcbf8d526

Pushed by geoff@darktrojan.net: https://hg.mozilla.org/comm-central/rev/e52ae9ddfabc Stop using String.prototype.replaceAll in tests because it's not available on beta. rs=bustage-fix
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: