1266916 - New message invoked from mailto: URL starts as "Body Text" regardless of mail.compose.default_to_paragraph setting

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[rsx11m]

+++ This bug was initially created as a clone of Bug #1265920 +++

(Quoting rsx11m from bug 1265920 comment #10)
> Components.interfaces.nsIMsgCompType.ForwardAsAttachment: into the .New:
> case as it's essentially a new message with just an attachment (i.e.,
> forwarding as attachment did show up as empty "Body Text" regardless of the
> setting). It's a different story with .MailToUrl: where it depends whether
> or not a "?...&body=" parameter is given. If so, an empty <p> is generated
> with the intended body text placed underneath, so that isn't going to work
> if, e.g., a formatted reply is expected. Thus, I've left that one for a
> follow-up bug.

Currently, Components.interfaces.nsIMsgCompType.MailToUrl in the MsgComposeCommands.js stateListener doesn't check editor.CR_creates_new_p to switch to "Paragraph" mode. On the other hand, a "mailto:...?body=..." URL shouldn't add any formatting to the message passed as argument. Thus, if the body(+signature) is empty, the initial mode should be "Paragraph" if the pref is set, but stay unchanged as "Body Text" if a "[?&]body=" argument is present in the mailto: URL.

Comment 1

[Jorg K (CEST = GMT+2)]

The name of the preference which drives this has changed in bug 1273841, so I'm changing the summary.

Comment 2

[Jorg K (CEST = GMT+2)]

OK, I'm fixing this now.

Comment 3

[Jorg K (CEST = GMT+2)]

Attachment: Proposed solution (v1a).

This works for me. Try clicking on links like:
  <a href="mailto:[e-mail]">mailto:[e-mail]</a>
  <a href="mailto:[e-mail]?body=here%20comes%20trouble">
when paragraph mode is enabled.

Comment 4

[:aceman]

Comment on attachment 8794549 [details] [diff] [review]
Proposed solution (v1a).

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

Seems to work.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +344,5 @@
>        loadHTMLMsgPrefs();
>      AdjustFocus();
>    },
>  
> +  NotifyComposeBodyReadyNew: function(aMailToUrlProcessing) {

You can set a default here (= false) and please add the documentation header to the function.

@@ +345,5 @@
>      AdjustFocus();
>    },
>  
> +  NotifyComposeBodyReadyNew: function(aMailToUrlProcessing) {
> +

Why this empty line?

@@ +353,5 @@
> +    if (insertParagraph && aMailToUrlProcessing) {
> +      // Check for empty body before allowing paragraph to be inserted.
> +      // An "empty" body will have a <br> potentially followed by a
> +      // <pre class="moz-signature">.
> +      let mailDoc = document.getElementById("content-frame").contentDocument;

Try getBrowser() here.

Comment 5

[Jorg K (CEST = GMT+2)]

Attachment: Proposed solution (v1b).

Thanks, would you mind looking over it one more time.
I did a little clean-up using getBrowser() everywhere now to make it consistent.

Do I really need to add a documentation header to the function?? None of its brothers are documented and I'd rather leave it that way.

Comment 6

[Jorg K (CEST = GMT+2)]

Attachment: Proposed solution (v1c).

Fixed comment for getBrowser().

Comment 7

[Magnus Melin [:mkmelin]]

Comment on attachment 8794570 [details] [diff] [review]
Proposed solution (v1c).

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

::: mail/components/compose/content/MsgComposeCommands.js
@@ +344,5 @@
>        loadHTMLMsgPrefs();
>      AdjustFocus();
>    },
>  
> +  NotifyComposeBodyReadyNew: function(aMailToUrlProcessing = false) {

instead of adding another param, why now just look at gComposeType?

Comment 8

[Jorg K (CEST = GMT+2)]

Attachment: Proposed solution (v1d).

Incorporated Magnus' suggestion.

Comment 9

[:aceman]

Comment on attachment 8794604 [details] [diff] [review]
Proposed solution (v1d).

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

OK, thanks.

Sorry, I wanted to replace the opencoding of getElementByID(content-frame), but it seems we use GetCurrentEditorElement() for that in MsgComposeCommands.js (not getBrowser). Please change to that before pushing (and check it works). Thanks.

Comment 10

[Jorg K (CEST = GMT+2)]

Sorry, but it makes little sense to use GetCurrentEditorElement() from editorUtilities.js.

The getBrowser() does what we need. All I can offer is creating a function GetMailBody() as a shortcut for
document.getElementById("content-frame").contentDocument.querySelector("body");

Would you like that?

Comment 11

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/0958c59534c7

Landed as reviewed. Using GetCurrentEditorElement() makes no sense. I didn't create GetMailBody() but I checked that getBrowser() is used in zoom control.

Comment 12

[:aceman]

(In reply to Jorg K (GMT+2) from comment #10)
> Sorry, but it makes little sense to use GetCurrentEditorElement() from
> editorUtilities.js.
> 
> The getBrowser() does what we need.

GetCurrentEditorElement() does the same and is actually used in MsgComposeCommands.js, while getBrowser() is not (it is only needed due to external callers as you commented). So I wanted to have all code use the same function.

> All I can offer is creating a function
> GetMailBody() as a shortcut for
> document.getElementById("content-frame").contentDocument.
> querySelector("body");
> 
> Would you like that?

Not worth the hassle yet for 3 callers.

Comment 13

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #12)
> GetCurrentEditorElement() does the same ..
But please look ...
https://dxr.mozilla.org/comm-central/rev/26f04790ea9c46a3bc5a18ec18826b8bd790ffbe/editor/ui/composer/content/editorUtilities.js#193

That's a whole lot of overkill for one simple getElementById(). If anything, we should remove those calls and replace with getBrowser(), don't you think?

Comment 14

[:aceman]

Yes, the getElementById() is simple today, because we have a single <editor>. But it may be needed to use GetCurrentEditorElement() once it gets more fancy (with more editors).
So if you want to push the conversions of the callers to the future when it is needed, just leave it as it is now. (And hope somebody notices we wanted an editor when calling getBrowser). But do not convert anything more to getBrowser.

Actually how does your extension work with 2 editors (see https://dxr.mozilla.org/comm-central/rev/26f04790ea9c46a3bc5a18ec18826b8bd790ffbe/editor/ui/composer/content/editorUtilities.js#169, they expect HTML editing in the future)?

Comment 15

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #14)
> Actually how does your extension work with 2 editors ...
You should try it. There aren't two editors together. Each editor is in a separate tab.

Comment 16

[Jorg K (CEST = GMT+2)]

Attachment: 1266916-followup.patch

Sorry, I didn't test this with a HTML signature. Now I did and noticed that it doesn't work.

Comment 17

[:aceman]

Comment on attachment 8796870 [details] [diff] [review]
1266916-followup.patch

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

::: mail/components/compose/content/MsgComposeCommands.js
@@ +362,5 @@
>  
>          if (!nextSibling)
>            break;
>  
> +        if ((nextSibling.localName == "div" || nextSibling.localName == "pre") &&

Is the element name important? Isn't the moz-signature class name enough of a proof we are at the signature?

Comment 18

[Jorg K (CEST = GMT+2)]

You should have asked that for the first review ;-)

Well, a block element, either pre or div, with class moz-signature is the signature. I prefer to check.

Comment 19

[:aceman]

Comment on attachment 8796870 [details] [diff] [review]
1266916-followup.patch

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

ok :)

Comment 20

[Jorg K (CEST = GMT+2)]

Follow-up landed:
https://hg.mozilla.org/comm-central/rev/a07b70737d6f