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

RESOLVED FIXED in Thunderbird 52.0

Status

Thunderbird
Message Compose Window
RESOLVED FIXED
a year ago
9 months ago

People

(Reporter: rsx11m, Assigned: Jorg K (GMT+2))

Tracking

Trunk
Thunderbird 52.0
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

a year ago
+++ 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.
(Assignee)

Comment 1

9 months ago
The name of the preference which drives this has changed in bug 1273841, so I'm changing the summary.
Summary: New message invoked from mailto: URL starts as "Body Text" regardless of editor.CR_creates_new_p setting → New message invoked from mailto: URL starts as "Body Text" regardless of mail.compose.default_to_paragraph setting
(Assignee)

Comment 2

9 months ago
OK, I'm fixing this now.
Assignee: nobody → jorgk
(Assignee)

Comment 3

9 months ago
Created attachment 8794549 [details] [diff] [review]
Proposed solution (v1a).

This works for me. Try clicking on links like:
  <a href="mailto:jorgk@jorgk.com">mailto:jorgk@jorgk.com</a>
  <a href="mailto:jorgk@jorgk.com?body=here%20comes%20trouble">
when paragraph mode is enabled.
Attachment #8794549 - Flags: review?(acelists)

Comment 4

9 months ago
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.
Attachment #8794549 - Flags: review?(acelists) → review+
(Assignee)

Comment 5

9 months ago
Created attachment 8794552 [details] [diff] [review]
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.
Attachment #8794549 - Attachment is obsolete: true
Attachment #8794552 - Flags: review?(acelists)
(Assignee)

Comment 6

9 months ago
Created attachment 8794570 [details] [diff] [review]
Proposed solution (v1c).

Fixed comment for getBrowser().
Attachment #8794552 - Attachment is obsolete: true
Attachment #8794552 - Flags: review?(acelists)
Attachment #8794570 - Flags: review?(acelists)

Comment 7

9 months ago
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?
(Assignee)

Comment 8

9 months ago
Created attachment 8794604 [details] [diff] [review]
Proposed solution (v1d).

Incorporated Magnus' suggestion.
Attachment #8794570 - Attachment is obsolete: true
Attachment #8794570 - Flags: review?(acelists)
Attachment #8794604 - Flags: review?(acelists)

Comment 9

9 months ago
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.
Attachment #8794604 - Flags: review?(acelists) → review+
(Assignee)

Comment 10

9 months ago
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?
(Assignee)

Comment 11

9 months ago
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.
Status: NEW → RESOLVED
Last Resolved: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 52.0

Comment 12

9 months ago
(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.
(Assignee)

Comment 13

9 months ago
(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

9 months ago
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)?
(Assignee)

Comment 15

9 months ago
(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.
(Assignee)

Comment 16

9 months ago
Created attachment 8796870 [details] [diff] [review]
1266916-followup.patch

Sorry, I didn't test this with a HTML signature. Now I did and noticed that it doesn't work.
Attachment #8796870 - Flags: review?(acelists)
(Assignee)

Updated

9 months ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 17

9 months ago
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?
(Assignee)

Comment 18

9 months ago
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

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

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

ok :)
Attachment #8796870 - Flags: review?(acelists) → review+
(Assignee)

Comment 20

9 months ago
Follow-up landed:
https://hg.mozilla.org/comm-central/rev/a07b70737d6f
Status: REOPENED → RESOLVED
Last Resolved: 9 months ago9 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.