1265534 - Make editor.CR_creates_new_p work for Mail & News composition and allow Paragraph mode as default

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: Composition, enhancement)

Description

[rsx11m]

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

This bug partially ports additions which Thunderbird made in  https://hg.mozilla.org/comm-central/diff/322cdc163b99/mail/components/compose/content/MsgComposeCommands.js to suite/mailnews/compose/MsgComposeCommands.js in order to have a consistent behavior between Composer and Mail & News when in a "Paragraph" context.

Note that I do *not* plan on also changing the composition default from "Body Text" to "Paragraph" in the process, the user can always pick this mode manually similar to any of the others.

The default of editor.CR_creates_new_p is subject of bug 1265246.

Comment 1

[rsx11m]

Attachment: Proposed patch

With "Return in a paragraph always creates a new paragraph" in Edit > Preferences > Composer checked, switch "Body Text" to "Paragraph" in a Mail & News composition window. The "Enter/Return" key should cause "<p></p><p></p" rather than "<p><br></p>"; independently, Shift+Enter should still insert a "<br>" instead.

No change in behavior if the box is not checked or if not in "Paragraph" context.

Comment 2

[rsx11m]

Follow-up from the discussion with Jorg K in the other bug:

> Note that I do *not* plan on also changing the composition default from
> "Body Text" to "Paragraph" in the process, the user can always pick this
> mode manually similar to any of the others.

(Quoting rsx11m from bug 1265246 comment #28)
> [using "Paragraph" as the default] If so, I wouldn't tie it to Composer's
> editor.CR_creates_new_p pref but use a separate mail{news}.compose* pref to
> be presented in the Mail & News > Composition prefpane, otherwise it would
> be completely counter-intuitive (and as bug 1266211 shows, the connection
> isn't obvious for TB users either, even though the checkbox is at the right
> place). For that reason, I'd think that keeping editor.CR_creates_new_p true
> now by default would even work if we'd implement the <p> default composition
> mode as well (with a separate pref).

If this is wanted, it should be straight-forward to add given that we have a similar stateListener implementation as Thunderbird. However, such a new preference IMO should default to "false" in this case and be exposed in the "Default for HTML Messages" groupbox as a checkbox or menulist.

Comment 3

[rsx11m]

Attachment: More comprehensive patch

While I'm on it, here the alternative patch that mirrors TB bug 330891, except:
 - creating a new pref mail.compose.start_as_paragraph to select the default
 - retaining the default of composing as "Body Text" with optional "Paragraph"
 - basing Return/Enter behavior on both editor and mail preferences
 - adding a more explicit UI into the Composition prefpane

This also contains changes/fixes made/proposed for Thunderbird in bug 1265920 comment #10.

The initialization of editor.returnInParagraphCreatesNewParagraph in InitEditor() is based on both preferences with the rationale:
 - if the user chooses editor.CR_creates_new_p=true for Composer, it's used for
   Mail & News regardless of "Body Text" or "Paragraph" as default;
 - if the user wants "Paragraph" as default mode, it is assumed that he/she also
   wants the Enter/Return key to open a new paragraph.
The latter choice makes this independent of the Composer default picked in bug 1265246 and mirrors more closely what Thunderbird did (just based on a mail-specific pref instead).

As for the implementation, the new pref is wrapped into #ifdef MOZ_SUITE which TB can remove in case they decide to use it in some way as well. The hunks in MsgComposeCommands.js are mostly coming from bug 330891/bug 1265920. For the prefpane, I'm reusing the editor entities for the menu, similar to the default size selections.

I'll keep attachment 8742525 [details] [diff] [review] open for review in case this full implementation isn't wanted for SeaMonkey.

Comment 4

[Philip Chee]

Comment on attachment 8744075 [details] [diff] [review]
More comprehensive patch

>        </grid>
I think a separator class="thin" might be in order to open up some vertical space here.

> +      <hbox align="center">
> +        <label id="selectBodyType"
> +               value="&selectBodyType.label;"
> +               accesskey="&selectBodyType.accesskey;"
> +               control="defaultBodyType"/>
> +        <menulist id="defaultBodyType"
> +                  preference="mail.compose.start_as_paragraph">
> +          <menupopup>
> +            <menuitem value="false"
> +                      label="&bodyTextCmd.label;"/>
> +            <menuitem value="true"
> +                      label="&paragraphParagraphCmd.label;"/>

1. Since this is a boolean I think a checkbox or a radio group is more appropriate.

2. "Format message as:"
This is totally opaque. I have no clue what "Format message as Body Text" means. Yes I know the help page explains this,

The Thunderbird equivalent at least is clearer:
"When using paragraph format, the enter key creates a new paragraph"

I suggest something like:

Start a new message with:
(*) Paragraph format. The Return/Enter key creates a new paragraph.
( ) Body format. The Return/Enter key always inserts a new line.

3.  "Body Text" "Paragraph"
You're stealing strings from editor (but I see we are already doing that)

> +    // Look all the possible compose types (nsIMsgComposeParams.idl):
Where did nsIMsgCompType.NewsPost go?

> +    case Components.interfaces.nsIMsgCompType.Redirect:
> +      break;
> +
> +    default:
> +      dump("Unexpected nsIMsgCompType in NotifyComposeBodyReady (" +
> +           gComposeType + ")\n");
> +      break;
I don't think we need that dump. It just generates noise if you're looking for something else.

> +  NotifyComposeBodyReadyNew: function() {
> +    // Control insertion of line breaks.
> +    let useParagraph = Services.prefs.getBoolPref("mail.compose.start_as_paragraph");
> +    if (gMsgCompose.composeHTML && useParagraph) {

> +  NotifyComposeBodyReadyReply: function() {
> +    // Control insertion of line breaks.
> +    let useParagraph = Services.prefs.getBoolPref("mail.compose.start_as_paragraph");
> +    if (gMsgCompose.composeHTML && useParagraph) {

> +  NotifyComposeBodyReadyForwardInline: function() {
> .........
> +    // Control insertion of line breaks.
> +    selection.collapse(mailBody, 0);
> +    let useParagraph = Services.prefs.getBoolPref("mail.compose.start_as_paragraph");
> +    if (gMsgCompose.composeHTML && useParagraph) {

I see lots of duplicated code. Perhaps something like:


   NotifyComposeBodyReady: function() {
     this.useParagraph = gMsgCompose.composeHTML &&
                         Services.prefs.getBoolPref("mail.compose.start_as_paragraph");
     this.editor = GetCurrentEditor();
     this.paragraphState = document.getElementById("cmd_paragraphState");
....
  NotifyComposeBodyReadyNew: function() {
    if (this.useParagraph) {
....

> +    switch (gComposeType) {
> +
> +    case Components.interfaces.nsIMsgCompType.New:
case/default should be indented 2 more spaces.

> +      // Delete a <br> if we see one.
> +      let currentNode = mailBody.childNodes[start];
> +      if (currentNode.nodeName == "BR") {

> +        mailBody.removeChild(currentNode);
You can use currentNode.remove();

> +      // insertLineBreak() has been observed to insert two <br> elements
> +      // instead of one before a <div>, so we'll do it ourselves here.
Did anyone file a bug? Has this been fixed?

> +  // Control insertion of line breaks.
> +  editor.returnInParagraphCreatesNewParagraph =
> +    Services.prefs.getBoolPref("mail.compose.start_as_paragraph") ||
> +      Services.prefs.getBoolPref("editor.CR_creates_new_p");
The two Services lines should align vertically.

Comment 5

[rsx11m]

Attachment: Proposed patch (v3)

(In reply to Philip Chee from comment #4)
> I think a separator class="thin" might be in order to open up some vertical
> space here.

done.

> 1. Since this is a boolean I think a checkbox or a radio group is more
> appropriate.

I've made it a menu in analogy to the menu which you see in the formatting bar of the composition window, as a visual clue. I don't like the checkbox, thus went with the radiogroup as it allows for a better explanation of the two options. I left "Body Text" as the first option to reflect the order in the editor menu.

> The Thunderbird equivalent at least is clearer:
> "When using paragraph format, the enter key creates a new paragraph"

Except that it doesn't tell you that checking this will actually make "Paragraph" the default. ;-)

I went with your suggested labels, slightly altered though to make them fit (they now align at the right with the "Automatically save" label, which is a tight fit on Linux already).

> 3.  "Body Text" "Paragraph"
> You're stealing strings from editor (but I see we are already doing that)

The strings are different now, thus I'm properly defining them in pref-composing_messages.dtd instead.

> > +    // Look all the possible compose types (nsIMsgComposeParams.idl):
> Where did nsIMsgCompType.NewsPost go?

Oops, that's the code I've copy-pasted from Jorg's implementation for Thunderbird. News posts should be treated as a new message, thus I've added it there and filed bug 1267469 to fix this in Thunderbird.

I've also added the fix from attachment 8745117 [details] [diff] [review] to properly disable undo for the manipulations when replying.

> I don't think we need that dump. It just generates noise if you're looking
> for something else.

Removed along with the empty (and now useless) cases just resulting in a break without any action.

> I see lots of duplicated code. Perhaps something like:

Followed your example, works fine. I only took the existing code without trying to optimize it.

> case/default should be indented 2 more spaces.

done, with default: now gone.

> You can use currentNode.remove();

done.

> > +      // insertLineBreak() has been observed to insert two <br> elements
> > +      // instead of one before a <div>, so we'll do it ourselves here.
> Did anyone file a bug? Has this been fixed?

I don't know, Jorg added this comment in the original implementation, thus fairly recent. I can check if anything is filed yet.

> The two Services lines should align vertically.

done.

Comment 6

[rsx11m]

(In reply to rsx11m from comment #5)
> > > +      // insertLineBreak() has been observed to insert two <br> elements
> > > +      // instead of one before a <div>, so we'll do it ourselves here.
> > Did anyone file a bug? Has this been fixed?
> 
> I don't know, Jorg added this comment in the original implementation, thus
> fairly recent. I can check if anything is filed yet.

I can't find anything filed on this. Jorg, since you've made the observation in bug 736584 and know the steps to reproduce, can you file such an editor bug unless it's hiding somewhere already? Thanks. (assuming that it actually /is/ a bug and not a feature...)

Comment 7

[Jorg K (CEST = GMT+2)]

Sorry, I don't intend doing anything here. If you want to investigate, put the "insertLineBreak()" back locally and see what happens. I haven't filed a bug on it, I don't know whether it's a feature or not but we have more important bugs to fix in the M-C editor.

Let me take this opportunity to thank you for all the diligent work. You've started to become active again recently, is that right? Great to have you back and it's been great working with you.

Comment 8

[Philip Chee]

>>> +    // Look all the possible compose types (nsIMsgComposeParams.idl):
>> Where did nsIMsgCompType.NewsPost go?
....
>> I don't think we need that dump. It just generates noise if you're looking
>> for something else.
See? That dump should have alerted you to the missing nsIMsgCompType.NewsPost. Since it didn't I was right that it is useless.

Comment 9

[rsx11m]

(In reply to Philip Chee from comment #8)
> See? That dump should have alerted you to the missing nsIMsgCompType.NewsPost.
> Since it didn't I was right that it is useless.

:-) I don't have any newsgroups set up and rarely use those (and if then mostly through associated mailing lists), thus the dump was never reached.

(In reply to Jorg K (GMT+2) from comment #7)
> Sorry, I don't intend doing anything here. If you want to investigate, put
> the "insertLineBreak()" back locally and see what happens.

I'm not really sure what to file there either, it may be a feature to emphasize the <div>.

> Let me take this opportunity to thank you for all the diligent work. You've
> started to become active again recently, is that right? Great to have you
> back and it's been great working with you.

Thanks. It's going up and down, depending on real-life workload. Lately I've focused more on SeaMonkey than Thunderbird, that's certainly right.

Comment 10

[Philip Chee]

Comment on attachment 8745139 [details] [diff] [review]
Proposed patch (v3)

すばらし! ui-r=me

> +<!ENTITY defaultCompose.label "Start a new message in:">
?Start a new message with
?Start a new message using
I don't know. Ask IanN for some better wording.

Comment 11

[rsx11m]

I'm not sure either. For some reason, the "with" felt wrong to me; if you don't like the "in" either, the "Start a new message using: ... format" is probably the best. I'll use that one unless IanN comes up with a better phrase.

Comment 12

[rsx11m]

Attachment: Proposed patch (v4)

Updated patch using "using" with changes to the Help text to avoid "using" + "used" in the same line.

Comment 13

[Jorg K (CEST = GMT+2)]

rxs11m, I've thought about this for a while. I like your proposal of having two options, one for determining the function of the enter key and one for determining in which mode to start.

I believe that having the enter key in paragraph mode is really the way to go, but that shouldn't imply that we always also start in paragraph mode. We've seen that many conservative users don't like it, but by switching the option off, the also forgo the option of the more meaningful enter key processing.

In brief: Would you prepared to port your suggestion here to TB in a new bug? I'll promise you a very quick review ;-)

I'd just put the two options together
[ ] When using paragraph format, the enter key creates a new paragraph [existing]
[ ] Start a new message using paragraph format [new]

Both would be enabled by default in order not to change the behaviour from TB 45.

What do you think, Magnus?

Comment 14

[rsx11m]

(In reply to Jorg K (PTO during summer, NI me) from comment #13)
> I'd just put the two options together
> [ ] When using paragraph format, the enter key creates a new paragraph
> [existing]
> [ ] Start a new message using paragraph format [new]

Sure, that should be straight-forward. I'd leave the patch here untouched though and depend the Enter key on either pref, given that editor.CR_creates_new_p is primarily a Composer preference, thus shouldn't be replicated in SeaMonkey's Mail & News prefpane.

Comment 15

[rsx11m]

I've filed bug 1273841 for Thunderbird, let's continue there.

Comment 16

[rsx11m]

(In reply to Philip Chee from comment #10)
> > +<!ENTITY defaultCompose.label "Start a new message in:">
> ?Start a new message with
> ?Start a new message using

In the Thunderbird version of this bug, the potential future use for the new pref in other scopes like splitting blockquotes is discussed. A more general label is thus proposed, which would translate here to something like

> +<!ENTITY defaultCompose.label "Default composition format:">

which coincidentally the entity name already suggests. ;-)

Let's see what's happening over there, I may be revising the patch again (especially if the name of the underlying pref changes).

Comment 17

[rsx11m]

Attachment: Unbitrotted patch (v5)

Adjusted for comm-central changeset 45c77ad60f28:

 - change of preference name to mail.compose.default_to_paragraph;
 - removed mailnews/mailnews.js part which was defined by bug 1273841 already;
 - made UI labels more generic per comment #16 for things to come;
 - adjust Help content respectively.

I'm re-requesting ui-r? for the label changes. Since "format" is now stated in the radiogroup's description, I've removed it from the individual radios. Using "Default composition style:" instead was another label crossing my mind, in which case I'd retain "Paragraph format" and "Body Text format" for the radios.

Comment 18

[Philip Chee]

Comment on attachment 8754488 [details] [diff] [review]
Unbitrotted patch (v5)

Looks good. ui-r=me

Comment 19

[Ian Neal]

Comment on attachment 8754488 [details] [diff] [review]
Unbitrotted patch (v5)

r/a=me looks good

Comment 20

[rsx11m]

Thanks, pushed as http://hg.mozilla.org/comm-central/rev/82b23d30dcca