1273841 - Separate Enter key behavior and starting a new message using paragraph format

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, enhancement)

Description

[rsx11m]

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

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

(In reply to Jorg K (PTO during summer, NI me) from bug 1265534 comment #13)
> 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 1

[rsx11m]

A couple of design questions:

- Since you propose a checkbox, I take it that you didn't like the initial proposal in
  bug 1265534 attachment 8744075 [details] [diff] [review] either to use a menulist mirroring
  the [Body Text] / [Paragraph] choices in the composition window's formatting toolbar?

  Another way to be more specific about the unchecked state of that box might be:

  [ ] When using Paragraph format, the Enter key creates a new paragraph
  Start a new message using: (o) Paragraph format ( ) Body Text format

  I'm fine with either solution, and the checkbox certainly is most concise. Or, maybe:

  [ ] When using Paragraph format, the Enter key creates a new paragraph
  [ ] Start a new message using Paragraph format instead of Body Text


> 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.

- This is asking for some migration code to avoid that users who switched editor.CR_creates_new_p
  off for the purpose of starting as [Body Text] get [Paragraph] as default again.

  So, if there exists a user-set editor.CR_creates_new_p value I'd copy that to the new
  mail.compose.start_as_paragraph pref. Should editor.CR_creates_new_p remain as was set
  by the user or reset to the default, i.e., Enter = new paragraph?

Comment 2

[Jorg K (CEST = GMT+2)]

(In reply to rsx11m from comment #1)
> A couple of design questions:
> - Since you propose a checkbox, I take it that you didn't like the initial
>   proposal in bug 1265534 attachment 8744075 [details] [diff] [review] either to use a
>   menulist mirroring the [Body Text] / [Paragraph] choices in the composition
>   window's formatting toolbar?
Frankly, since the other bug was for SM, I didn't follow it all that closely.
I don't mind which ever UI it is, but perhaps a dropdown menu (menulist) is not necessary.
 
>   [ ] When using Paragraph format, the Enter key creates a new paragraph
>   [ ] Start a new message using Paragraph format instead of Body Text
I think the two options are largely independent. So I like to separate check-boxes better.

> - This is asking for some migration code to avoid that users who switched
>   editor.CR_creates_new_p off ...
Yes, to make it perfect, there should be some migration.

> So, if there exists a user-set editor.CR_creates_new_p value I'd copy that
> to the new mail.compose.start_as_paragraph pref.
Yes.

> Should editor.CR_creates_new_p remain as was set
> by the user or reset to the default, i.e., Enter = new paragraph?
I'd leave it untouched, so there are no surprises when switching to the new version.

So the only migration would be:
mail.compose.start_as_paragraph = editor.CR_creates_new_p.

Thanks for the detailed analysis. Much appreciated. I wish we had done this before switching to paragraph mode ;-(

I'll let Richard take a look at it.

Comment 3

[Richard Marti (:Paenglab)]

+1 for doing this.

(In reply to Jorg K (PTO during summer, NI me) from comment #2)
> (In reply to rsx11m from comment #1)
> >   [ ] When using Paragraph format, the Enter key creates a new paragraph
> >   [ ] Start a new message using Paragraph format instead of Body Text
> I think the two options are largely independent. So I like to separate
> check-boxes better.

How do you want to separate them, with a spacer?

> So the only migration would be:
> mail.compose.start_as_paragraph = editor.CR_creates_new_p.

That should do it.

Comment 4

[Jorg K (CEST = GMT+2)]

(In reply to Richard Marti (:Paenglab) from comment #3)
> +1 for doing this.
Thanks ;-)

> > I think the two options are largely independent. So I like to separate
> > check-boxes better.
> How do you want to separate them, with a spacer?
No idea. Not at all? What's a spacer ;-) Not a separator, I assume.

> > So the only migration would be:
> > mail.compose.start_as_paragraph = editor.CR_creates_new_p.
> 
> That should do it.
Thanks for confirming.

Comment 5

[Richard Marti (:Paenglab)]

(In reply to Jorg K (PTO during summer, NI me) from comment #4)
> (In reply to Richard Marti (:Paenglab) from comment #3)
> > +1 for doing this.
> Thanks ;-)
> 
> > > I think the two options are largely independent. So I like to separate
> > > check-boxes better.
> > How do you want to separate them, with a spacer?
> No idea. Not at all? What's a spacer ;-) Not a separator, I assume.

A spacer is the horizontal separator. So now, with a separator?

Comment 6

[Magnus Melin [:mkmelin]]

Technically the options are different, but without automatically going into <p> that's an unused preference for pretty much everybody. The percentage of people *ever* manually switching from <body> to <p> mode must be waaaaay below 1%.

I don't think it's reasonable to require users to check two boxes to get sane behaviour.

Comment 7

[Jorg K (CEST = GMT+2)]

(In reply to Richard Marti (:Paenglab) from comment #5)
> A spacer is the horizontal separator. So now, with a separator?
Well, I'm still confused. Take a look at the composition options:
[ ] Auto save every ...
[ ] Confirm when using a keyboard shortcut ...
Just like that. No visible separator.

(In reply to Magnus Melin from comment #6)
> Technically the options are different, but without automatically going into
> <p> that's an unused preference for pretty much everybody. The percentage of
> people *ever* manually switching from <body> to <p> mode must be waaaaay
> below 1%.
How do you know? Are you saying that paragraph mode is almost not used at all because people don't switch to it? Even 0.5% would be 100.000 people, more users than most add-ons have.

> I don't think it's reasonable to require users to check two boxes to get
> sane behaviour.
What's wrong with clear orthogonal design and not the conflation we have now. If you want one option, then it should be the one in which format to start. When in paragraph mode, <enter> should always enter a new paragraph. That's what FF does in its contenteditable. But sadly, that hasn't been TB's standard behaviour.

I think it's sane to have:
[x] When using Paragraph format, the Enter key creates a new paragraph
[ ] Start a new message using Paragraph format instead of Body Text
If you occasionally use paragraph mode, then is works as it should.

[ ] When using Paragraph format, the Enter key creates a new paragraph
[x] Start a new message using Paragraph format instead of Body Text
Is not really sane, I agree. Both ticked is sane and both unticked is also sane. So only 25% insane ;-)

Basically you're saying this is a WONTFIX?

Comment 8

[Magnus Melin [:mkmelin]]

(In reply to Jorg K (PTO during summer, NI me) from comment #7)
> How do you know? Are you saying that paragraph mode is almost not used at
> all because people don't switch to it? Even 0.5% would be 100.000 people,
> more users than most add-ons have.

Absolute numbers doesn't really matter. What kind of UI would you end up if we implemented every fringe feature people could think of?

> start. When in paragraph mode, <enter> should always enter a new paragraph.
> That's what FF does in its contenteditable. But sadly, that hasn't been TB's
> standard behaviour.

Yes of course, it was simply a bug. 

> Basically you're saying this is a WONTFIX?

What I'm saying is the "enter in <p> means new <p>" does not really need an UI. Even a hidden pref is quite questionable.

Comment 9

[Jorg K (CEST = GMT+2)]

Well, what I'm hearing here is:
Have two preferences:
editor.CR_creates_new_p: That's always true and doesn't appear in the UI.
mail.compose.start_as_paragraph: That the user can switch.

So we replace
[ ] When using Paragraph format, the Enter key creates a new paragraph
with
[ ] Start a new message using Paragraph format instead of Body Text.

I could live with that.

Comment 10

[rsx11m]

(In reply to Magnus Melin from comment #8)
> What I'm saying is the "enter in <p> means new <p>" does not really need an UI.

Yeah, I'm with Magnus on that one. The more user-relevant choice is whether to use Paragraph or Body Text for the message by default. Since Body Text doesn't obey editor.CR_creates_new_p, there isn't any change from the current behavior unless the user switches manually into Paragraph mode. Only then we would insert </p><p> rather than <br>, and switching that off can be done with the hidden preference for those who care.

Users I encountered had issues anyway to understand the "Enter key creates a new paragraph" and cared more about Paragraph as default mode. Thus, from a superficial perspective, just the label is "clarified" to be more intuitive, even though we have now two separate prefs under the hood.

Not exposing both prefs in the UI also allows me to retain the 'P' accesskey for that checkbox. ;-)

Comment 11

[rsx11m]

Attachment: Proposed patch

This looks bigger than it actually is. The UI layout is identical, it just swaps editor.CR_creates_new_p against mail.compose.start_as_paragraph, adjusting ids and labels respectively.

In the backend, mail.compose.start_as_paragraph replaces editor.CR_creates_new_p in all occurrences but where it matters when initializing the editor (which is still based on editor.CR_creates_new_p and not implied by mail.compose.start_as_paragraph).

Note that the mail.compose.start_as_paragraph default in mailnews.js preempts bug 1265534 and is overridden in all-thunderbird.js, thus we don't have to move it again with SeaMonkey having a different default selection.

I'm asking Paenglab for ui-review, in case that the other checkbox is supposed to be retained anyway. Adding it back to the patch would be trivial.

Comment 12

[Kent James (:rkent)]

Will this solve my problem? I'm about ready to turn paragraph mode off, because the behavior is so inconsistent between normal replies and cases where I have edited the reply. Whenever I am involved with an in-line reply (which is a lot), the Enter key only does one line spacing. Otherwise, it is two lines. Which is it, one or two? I don't really much care whether it is "paragraphs" or not, only the number of lines. I really cannot live with the current case where the behavior is unpredictable.

Does "When using Paragraph format, the Enter key creates a new paragraph" mean that I will ALWAYS get two lines when I enter ENTER? If so, that is what I want.

But dumbing myself down, I don't really think in terms of paragraphs, only number of line spaces. I think the wording needs to be more meaningful to my dumbed-down self who really knows little about <p>.

Comment 13

[Jorg K (CEST = GMT+2)]

(In reply to Kent James (:rkent) from comment #12)
> Whenever I am involved with an in-line reply
> (which is a lot), the Enter key only does one line spacing.
Bug 1271835.

> Otherwise, it is two lines. Which is it, one or two?
Actually, there is always one item inserted, either a line break or a paragraph break which looks wider.

> I really cannot live with the
> current case where the behavior is unpredictable.
The indicator tells you.

> Does "When using Paragraph format, the Enter key creates a new paragraph"
> mean that I will ALWAYS get two lines when I enter ENTER? If so, that is
> what I want.
No, that preference does what it says: When you are in a paragraph, and currently, you are not in a paragraph when splitting a block quote, the enter key inserts a new paragraph, which looks like two lines.

> But dumbing myself down, I don't really think in terms of paragraphs, only
> number of line spaces. I think the wording needs to be more meaningful to my
> dumbed-down self who really knows little about <p>.
All the proposed solution does is split the existing preference in two and change the label in the UI. So in the future, if you want to switch off starting in paragraph mode, you switch off
  [ ] Start a new message using Paragraph format instead of Body Text.

Comment 14

[Jorg K (CEST = GMT+2)]

(In reply to rsx11m from comment #11)
> I'm asking Paenglab for ui-review, in case that the other checkbox is
> supposed to be retained anyway. Adding it back to the patch would be trivial.
The patch looks excellent. I will try it shortly. I'm undecided on whether to retain the existing preference in the UI. I don't like that users have to use the preference editor, and given that the preference was already exposed (sadly with a double-meaning), I don't think the is much point to withdraw it now.

If we add another preference to the UI, users will say: Oh, the improved the existing feature. If we just change the label (since users won't understand the two preferences) they will say: What's happened here?

Comment 15

[Magnus Melin [:mkmelin]]

Hm, actually now that I read bug 1271835, separating the prefs at all may be a mistake. The pref is editor.CR_creates_new_p and it's a slippery slope with what you would implement if you start disentangling it from the startup behavior. 

Our current pref UI label is a bit misleading, but if we interpret the pref as it's named, enter inside a blockquote *would* create a new paragraph which is certainly what we want.

Comment 16

[Jorg K (CEST = GMT+2)]

Here's another argument against withdrawing
[ ] When using Paragraph format, the Enter key creates a new paragraph

A user who had the only currently existing option switched off and in the future decides he wants paragraph mode, will get mail.compose.start_as_paragraph set, but not editor.CR_creates_new_p. This user will get the faulty paragraph behaviour we try to avoid (like I just did testing it).

So either,
1) setting mail.compose.start_as_paragraph will switch editor.CR_creates_new_p back on or
2) we should go with two check-boxes or
3) the migration should switch it on as suggested in comment #1.
I'm against hidden behaviour in 1).

(In reply to Magnus Melin from comment #15)
I would *NOT* drive the blockquote splitting with editor.CR_creates_new_p. That preference is about what happens when you're already in paragraph mode. We could consider the preference permanently switched on.

If we change the blockquote splitting, we should control it with the second preference, which should perhaps read:
[ ] Use Paragraph format instead of Body Text by default.

rsx11m, I tested the patch, I'd give it r+, but we're still discussing the UI.

Comment 17

[Axel Grude [:realRaven]]

As Jörg mentioned, [Bug 1271835] is related. If we have a separate option to select <p> as the starting point (or I would call it the default element) this should also work for splitting block quotes.

Comment 18

[rsx11m]

(In reply to Magnus Melin from comment #15)
> Hm, actually now that I read bug 1271835, separating the prefs at all may be
> a mistake. The pref is editor.CR_creates_new_p and it's a slippery slope
> with what you would implement if you start disentangling it from the startup
> behavior. 

If you don't disentangle them, a user who doesn't want to use Paragraph as default won't have a chance to enable the "Enter creates <p>" behavior as default for the occasional cases where he or she manually switches into that mode for whatever reason. That's the whole point of this bug to start with.

(In reply to Jorg K (PTO during summer, NI me) from comment #16)
> A user who had the only currently existing option switched off and in the
> future decides he wants paragraph mode, will get
> mail.compose.start_as_paragraph set, but not editor.CR_creates_new_p. This
> user will get the faulty paragraph behaviour we try to avoid (like I just
> did testing it).

Good point.

> 1) setting mail.compose.start_as_paragraph will switch editor.CR_creates_new_p back on
> ... I'm against hidden behaviour in 1).

Me too, that's kind-of emulating what we currently have and thus wouldn't help.

> 2) we should go with two check-boxes or
> 3) the migration should switch it on as suggested in comment #1.

I'm ok with either of those.

> If we change the blockquote splitting, we should control it with the second
> preference, which should perhaps read:
> [ ] Use Paragraph format instead of Body Text by default.

That's for bug 1271835 to decide, but it would make sense (thus leaving the editor pref strictly scoped to within-paragraph behavior). We can generalize the label here or if and when that happens in the other bug (where simply "Use ... by default" would be sufficient here already as well, thus being just less specific when Paragraph is switched on and not just restricting it to the start scenario).

Comment 19

[Jorg K (CEST = GMT+2)]

I'm still in favour of two visible options, one to control the paragraph behaviour, the other one to control the "default tag" with a generic label so it can control other cases, too.

Comment 20

[rsx11m]

In that case, mail.compose.start_as_paragraph probably should be mail.compose.default_to_paragraph instead to reflect its broader scope.

Comment 21

[Jorg K (CEST = GMT+2)]

+1.

Maybe Richard can tell us whether he prefers two options or one option in the UI.
He's already marked for ui-r? but I'll NI him. No rush, Richard ;-)

Comment 22

[Richard Marti (:Paenglab)]

Comment on attachment 8754149 [details] [diff] [review]
Proposed patch

Not tested it but from ui code ui-r=me.

I think the editor.CR_creates_new_p doesn't need to be exposed in UI as this should be the default in paragraph mode like it is in word processors.

And for Jörg, two options may be too complicated for the user to select the correct combination.

Comment 23

[Jorg K (CEST = GMT+2)]

OK, I'll review it when the the migration issue is fixed and when we have a more generic label. Both easy adjustments ;-)

Comment 24

[rsx11m]

Attachment: Updated patch (v2)

(In reply to Jorg K (PTO during summer, NI me) from comment #16)
> 3) the migration should switch it on as suggested in comment #1.

Done.

> [ ] Use Paragraph format instead of Body Text by default.

Done.

(In reply to rsx11m from comment #20)
> In that case, mail.compose.start_as_paragraph probably should be
> mail.compose.default_to_paragraph instead to reflect its broader scope.

Done.

Comment 25

[Jorg K (CEST = GMT+2)]

Comment on attachment 8754365 [details] [diff] [review]
Updated patch (v2)

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

Very nice. Just one more question.

::: mail/base/modules/mailMigrator.js
@@ +319,5 @@
> +          Services.prefs.setBoolPref("mail.compose.default_to_paragraph",
> +            Services.prefs.getBoolPref("editor.CR_creates_new_p"));
> +          Services.prefs.clearUserPref("editor.CR_creates_new_p");
> +        }
> +      }

I wonder whether it would be safer to do this instead (pseudo-code):
if (CR_creates_new_p)
  default_to_paragraph = true;
else
  default_to_paragraph = false;
  // reinstate useful enter key behaviour paragraph.
  CR_creates_new_p = true;

Comment 26

[rsx11m]

I don't think it is useful to set a user-defined value for a pref if it's just the default.

The proposed code is:

if (user changed anything with CR_creates_new_p)
  copy CR_creates_new_p to default_to_paragraph
  reset CR_creates_new_p to default
else
  // do nothing as both defaults are identical

which I think is intuitively what it's supposed to do. In which scenario would this be unsafe?
Where would you see the advantage of that (slightly) more complex code?

Comment 27

[rsx11m]

Or, more specific: If there *is* a user-defined value for a preference, then *that's* what the user wants, otherwise he or she is happy with the default(s) and we just keep it.

Comment 28

[rsx11m]

It should only make a difference if the defaults are changed some time in the future. Let's assume that default_to_paragraph is changed to "false" for whatever reason while CR_creates_new_p remains "true" by default. Then, someone updates from 45.x to that new version.

In my code, unless the user has explicitly set CR_creates_new_p to "true" as a user-defined pref, he or she will be migrated to the new default for default_to_paragraph which is "false" then. In your code, the user would be migrated from the default to the non-default value, thus the change in default wouldn't be picked up. I'd say that my version meets the expected migration behavior in this scenario.

Comment 29

[Jorg K (CEST = GMT+2)]

(In reply to rsx11m from comment #26)
> In which scenario would this be unsafe?
I've just seen default values being switched around too many times, that prefHasUserValue is pretty dangerous, IMHO.

How about unconditionally?
copy CR_creates_new_p to default_to_paragraph [1]
reset CR_creates_new_p to default             [2]
so
+          Services.prefs.setBoolPref("mail.compose.default_to_paragraph",
+            Services.prefs.getBoolPref("editor.CR_creates_new_p"));
+          Services.prefs.clearUserPref("editor.CR_creates_new_p");
without the if (Services.prefs.prefHasUserValue("editor.CR_creates_new_p")).

We agree that default_to_paragraph should take over the function of CR_creates_new_p, right? We basically want to maintain the user choice. So that's what the first line does.
In the second line we're saying that we want to return CR_creates_new_p to its default, since it was previously misused for another function.

This way the code works without knowing what the actual defaults are and the person reading the code doesn't have to think about that either.

Comments?

Comment 30

[rsx11m]

Attachment: Unconditional migration (v3)

(In reply to Jorg K (PTO during summer, NI me) from comment #29)
> We agree that default_to_paragraph should take over the function of
> CR_creates_new_p, right? We basically want to maintain the user choice. So
> that's what the first line does.

Well, that's what .prefHasUserValue() is *supposed* to figure out. But then, given that the user_pref is simply cleared when the user changes it to the default value, it's impossible to tell whether it's still reflecting the default (which is subject to be changed in a future version) or the explicit user's choice.

So, given that, existing profiles would be migrated to the previously set behavior with disregard of any possible change in default value at that time while new installations will pick up the current default instead. Sounds acceptable, and it's done elsewhere as well (e.g., Account Settings).

Comment 31

[Jorg K (CEST = GMT+2)]

Comment on attachment 8754453 [details] [diff] [review]
Unconditional migration (v3)

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

Very nice. Thanks a lot. Sorry about the discussion about the details, I'm glad we found something we could both agree on. I wish you had been around when I started about one year ago, we could have done things better to start with.

Comment 32

[rsx11m]

Thanks, pushed as http://hg.mozilla.org/comm-central/rev/45c77ad60f28

No problem, thinking things through a bit longer is usually a good idea even if it implies a few hoops more to go through. Now I'll have to update the patch for bug 1265534 which I've just bitrotted.  ;-)