Closed
Bug 1266916
Opened 8 years ago
Closed 8 years ago
New message invoked from mailto: URL starts as "Body Text" regardless of mail.compose.default_to_paragraph setting
Categories
(Thunderbird :: Message Compose Window, defect)
Thunderbird
Message Compose Window
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 52.0
People
(Reporter: rsx11m.pub, Assigned: jorgk-bmo)
References
Details
Attachments
(2 files, 3 obsolete files)
6.79 KB,
patch
|
aceman
:
review+
|
Details | Diff | Splinter Review |
1.63 KB,
patch
|
aceman
:
review+
|
Details | Diff | Splinter Review |
+++ 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•8 years 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 3•8 years ago
|
||
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 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•8 years ago
|
||
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•8 years ago
|
||
Fixed comment for getBrowser().
Attachment #8794552 -
Attachment is obsolete: true
Attachment #8794552 -
Flags: review?(acelists)
Attachment #8794570 -
Flags: review?(acelists)
Comment 7•8 years 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•8 years ago
|
||
Incorporated Magnus' suggestion.
Attachment #8794570 -
Attachment is obsolete: true
Attachment #8794570 -
Flags: review?(acelists)
Attachment #8794604 -
Flags: review?(acelists)
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•8 years 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•8 years 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
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 52.0
Comment 12•8 years 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•8 years 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•8 years 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•8 years 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•8 years ago
|
||
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•8 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 17•8 years 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•8 years 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•8 years 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•8 years ago
|
||
Follow-up landed: https://hg.mozilla.org/comm-central/rev/a07b70737d6f
Status: REOPENED → RESOLVED
Closed: 8 years ago → 8 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•