1265246 - Move Thunderbird-specific change of editor.CR_creates_new_p default from composer.js to all-thunderbird.js override

Status: RESOLVED FIXED

(MailNews Core :: Composition, defect, P1)

Description

[rsx11m]

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

editor/ui/composer.js is composer code shared by SeaMonkey Composer, SeaMonkey Mail & News, and Thunderbird. In comm-central changeset 322cdc163b99 the global editor.CR_creates_new_p default was changed to "true" and thus affecting also SeaMonkey Composer and Mail & News.

Technically, this module is owned by SeaMonkey.

Since SeaMonkey didn't adopt any of the related front-end changes (and I'm not sure if it would accept that change as such), the default change should move into Thunderbird's overrides. Just setting the pref on Windows doesn't seem to do anything on Windows, but I've seen reports that it produces some issues on Linux, thus I'd like to see this move done before SM 2.42+ is released.

Comment 1

[rsx11m]

Attachment: Easy fix

IanN, ewong - we may want to take this for comm-release (or a SM-specific branch on the other repositories ahead of any reviews), to be sure that we don't see any side effects from bug 330891 in that release.

Comment 2

[Jorg K (CEST = GMT+2)]

Comment on attachment 8742125 [details] [diff] [review]
Easy fix

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

Stealing this review, Magnus has enough of them to do.
I wish you could set this somewhere in suite/, but you only have browser preferences.
So I'm OK with this.

Note: Bug 330891 only touched the preference in shared TB and SM code, the rest was in mail/.

Comment 3

[rsx11m]

Thanks, pushed as https://hg.mozilla.org/comm-central/rev/dab127daefb4

(In reply to Jorg K (GMT+2) from comment #2)
> Note: Bug 330891 only touched the preference in shared TB and SM code, the rest was in mail/.

Yes, that's what I've noticed. We have a UI for it in the Composer preferences already, which should be effective for composer (an inherited by Mail & News), but for some reason apparently not.

Comment 4

[rsx11m]

Comment on attachment 8742125 [details] [diff] [review]
Easy fix

[Approval Request Comment]
Regression caused by (bug #): bug 330891
User impact if declined: no user-facing change for Thunderbird, uncertain effect of new default on SeaMonkey
Testing completed (on c-c, etc.): just moving a pref unchanged between files
Risk to taking this patch (and alternatives if risky): very low

Comment 5

[Jorg K (CEST = GMT+2)]

(In reply to rsx11m from comment #3)
> Yes, that's what I've noticed. We have a UI for it in the Composer
> preferences already, which should be effective for composer (an inherited by
> Mail & News), but for some reason apparently not.

I'm not sure I'm understanding your comment 100%. Yes, you have the composer preference.
It will only apply in mail composition if you initialise the mail editor that way. We do it here:
https://dxr.mozilla.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#4866

This only seems to do it for the composer which is not used in TB:
https://dxr.mozilla.org/comm-central/source/editor/ui/composer/content/editor.js#166

So the SM mail editor doesn't get this set and hence it doesn't work in e-mail.

I don't care either way, but you could back this out and fix SM instead.

Comment 6

[Jorg K (CEST = GMT+2)]

Why would we need this in comm-esr45?

Comment 7

[rsx11m]

(In reply to Jorg K (GMT+2) from comment #5)
> It will only apply in mail composition if you initialise the mail editor
> that way. We do it here:
> https://dxr.mozilla.org/comm-central/source/mail/components/compose/content/
> MsgComposeCommands.js#4866

Ah, I see. So that would explain why Mail & News didn't pick it up.
Should be an easy addition if we want that.

> So the SM mail editor doesn't get this set and hence it doesn't work in e-mail.

It does, but I didn't see an effect with Composer when switched on...

> I don't care either way, but you could back this out and fix SM instead.

This may be considered in the future, sure. For now, I wanted a "safe" solution for 2.42.

(In reply to Jorg K (GMT+2) from comment #6)
> Why would we need this in comm-esr45?

SeaMonkey /may/ release from 45 ESR in the immediate future in order to be less impacted by changes on the rapid-release train for their releases. That's still pending a decision, there are pros and cons going either way, but we'd like to be ready to go if and when that happens (hence suite/ checkins for comm-release are currently mirrored to comm-esr45, as you may have noticed).

Comment 8

[Jorg K (CEST = GMT+2)]

(In reply to rsx11m from comment #7)
> It does, but I didn't see an effect with Composer when switched on...
Do I know your software better than you do? ;-)
I have SM 2.38 here. I open a new composer window. I switch to paragraph format. I type x<enter>y. Voilà!

Comment 9

[Jorg K (CEST = GMT+2)]

Comment on attachment 8742125 [details] [diff] [review]
Easy fix

No problem taking this in Aurora (although I'm still not convinced that this is the right thing to do, but I don't want to argue with the SM folks).

In any case, it's good for TB to have control over this preference with its own way of setting it.

Comment 10

[Jorg K (CEST = GMT+2)]

Why was this not marked resolved? It has landed and there is nothing else to do here.

Comment 11

[Magnus Melin [:mkmelin]]

Hmm, so all in all this change doesn't really change anything than for the seamonkey editor (since sm window is unaffected). And changing it for the worse really. editor.CR_creates_new_p default false for a web page editor?? Please at least file a bug to change the editor default to true!

Comment 12

[Jorg K (CEST = GMT+2)]

Still not to late to back this out. I approved since it was a SM request and we had changed their behaviour. However, the basis of this decision is rather thin, quoting from comment #0:
  I've seen reports that it produces some issues on Linux.

Comment 13

[rsx11m]

(In reply to Jorg K (GMT+2) from comment #8)
> Do I know your software better than you do? ;-)
> I have SM 2.38 here. I open a new composer window. I switch to paragraph
> format. I type x<enter>y. Voilà!

Apparently so. :-) I missed the "switch to paragraph format" part, then it works for me as well.

So, SM Composer starts off with "Body Text" regardless of the preference setting, which makes this less of an impact. Where did you set TB's composition window to start in "Paragraph" instead? That doesn't seem to be just implied by the editor.returnInParagraphCreatesNewParagraph flag.

(In reply to Jorg K (GMT+2) from comment #12)
> Still not to late to back this out. I approved since it was a SM request and
> we had changed their behaviour. However, the basis of this decision is
> rather thin, quoting from comment #0:
>   I've seen reports that it produces some issues on Linux.

...and to make it even thinner, I can't find that post any more to provide a link to it. I'm wondering how this would cause any problems, given that all that pref does is setting the editor flag (which is not set in Mail & News, thus /should/ be a don't care here). I'll get a Linux fresh build today and see if I find anything to support that.

(In reply to Magnus Melin from comment #11)
> Please at least file a bug to change the editor default to true!

SeaMonkey is rather conservative when it comes to defaults; e.g., messages are still forwarded as attachments by default, replies started below the quote, and the dialog recommending to send an HTML message as plain text still shown by default (and yes, a couple of those I'd personally change).

I would suggest to separate the TB settings as proposed here to retain the status quo, but will open a bug to make the pref work in SM's Mail & News and to investigate whether or not to flip the pref default in composer.js (and if so, that patch would contain reversal of the changes made here).

Comment 14

[Jorg K (CEST = GMT+2)]

(In reply to rsx11m from comment #13)
> Where did you set TB's
> composition window to start in "Paragraph" instead? That doesn't seem to be
> just implied by the editor.returnInParagraphCreatesNewParagraph flag.
Well, you could just look at the patch for bug 330891. But I can tell you, it's here:
https://dxr.mozilla.org/comm-central/source/mail/components/compose/content/cloudAttachmentLinkManager.js#66
and below.

Let me know what you want to do. Backout is still an option, but you only have a week before the next branch date since it would have to land on Aurora before that date.

Comment 15

[rsx11m]

(In reply to Jorg K (GMT+2) from comment #14)
> Well, you could just look at the patch for bug 330891. But I can tell you,

Indeed, that's what I did before, but got distracted by the name gCloudAttachmentLinkManager and thus didn't bother trying to further understand what it is doing (nor did I want to dig through the 126 comments in 330891), assuming it's only related to the filelink stuff. Maybe rename cloudAttachmentLinkManager.js to something that's more general, given that it does things beyond cloud attachments?

Ok, so you are modifying the editor content after it has been built by nsMsgCompose to inject the <p> elements here for the different cases, that's a bit obscure.

(In reply to rsx11m from comment #13)
> I'll get a Linux fresh build today and see if I find anything to support that.

I've been playing with a current SM trunk and a comm-release 2.42 build on Linux, didn't see any problems composing mails (new or as replies or forwarded inline) and in the composer with the pref set. Without any specifics given by that user, though, hard to know what to look for.

I'll raise a needinfo request in a moment to get more opinions what to do with the default.

Comment 16

[rsx11m]

(In reply to Jorg K (GMT+2) from comment #14)
> Let me know what you want to do. Backout is still an option, but you only
> have a week before the next branch date since it would have to land on
> Aurora before that date.

IanN, Ratty: Where do we want to go here? It appears that the changed editor.CR_creates_new_p default doesn't have any negative user-facing impact, also given that it requires the user to go into "Paragraph" mode first (we default to "Body Text" where it's not effective).

So, do you support backing this out again, thus accepting the change in default to "true" made by bug 330891? Either way, I think we should obey that pref in Mail & News as well to have a consistent behavior (while retaining the "Body Text" default), thus will file a SM bug with a patch on that.

Comment 17

[Jorg K (CEST = GMT+2)]

(In reply to rsx11m from comment #15)
> Indeed, that's what I did before, but got distracted by the name
> gCloudAttachmentLinkManager and thus didn't bother trying to further
> understand what it is doing assuming it's only related to the filelink stuff.
> Maybe rename
> cloudAttachmentLinkManager.js to something that's more general, given that
> it does things beyond cloud attachments?
Yes, this is a little unfortunate. It was even more unfortunate before bug 736584 ripped out a lot of processing there on inline forwarded messages.

> Ok, so you are modifying the editor content after it has been built by
> nsMsgCompose to inject the <p> elements here for the different cases, that's
> a bit obscure.
Yes, since just setting paragraph format doesn't stick unless you're really in a paragraph. But you don't need to switch users into paragraph mode. That's an additional thing we do.

Comment 18

[rsx11m]

(In reply to Jorg K (GMT+2) from comment #17)
> Yes, this is a little unfortunate. It was even more unfortunate before bug 736584
> ripped out a lot of processing there on inline forwarded messages.

nsMsgCompose.cpp sure is a giant, not easy to work with either...

> But you don't need to switch users into paragraph mode.

Don't plan to (comment #16), just the editor flag from the pref.

Comment 19

[Magnus Melin [:mkmelin]]

(In reply to rsx11m from comment #15)
> in 330891), assuming it's only related to the filelink stuff. Maybe rename
> cloudAttachmentLinkManager.js to something that's more general, given that
> it does things beyond cloud attachments?

The paragraph handling should probably move to be handled in MsgComposeCommands.js stateListener instead.

Comment 20

[rsx11m]

(In reply to Magnus Melin from comment #19)
> MsgComposeCommands.js stateListener instead.

That's approaching 5000 lines as well but appears to be intuitively the better spot.

With bug 736584 having removed the <div> stuff from NotifyComposeBodyReadyForwardInline, it appears that the entire set of NotifyComposeBodyReadyForward* functions can just be moved to the stateListener.

I'll have a look.

Comment 21

[Jorg K (CEST = GMT+2)]

I must admit that one aim of bug 736584 was to move this handling to a better spot. I never go to it, but I reduced it greatly ;-) Personally, I'd put it into a new source file.

Comment 22

[rsx11m]

Moved this discussion to bug 1265920.

Comment 23

[Kent James (:rkent)]

As far as I can tell, this patch has still not landed in comm-aurora, and I am reluctant to push it to comm-esr45 until that, and any remaining controversy is resolved. AFACIT this also only affects SM, so no reason to rush for Thunderbird 45.1

Comment 24

[rsx11m]

Yeah, sorry for the confusion. Either we land this on all branches (at least the ones SM is using) or on neither, given that changing the default in one release and then changing it back with a following release would be irritating for the user.

After the discussion here and getting a better understanding what this pref is doing and how it works, I'm no longer convinced that it's the right thing to do myself either and tend to retract my patch. Thus, unless either IanN or Ratty says otherwise by the end of the week, I'd say let's back it out again (Sunday at the latest) and only revisit if there are any complaints from the SM user community.

Comment 25

[Jorg K (CEST = GMT+2)]

Hmm, but you're saying something else in bug 1265534 comment #0 (quote):
I do *not* plan on also changing the composition default from "Body Text" to "Paragraph" in the process.

This says to me that you decided to leave the preference at "false" for SM as landed here. What am I missing?

Comment 26

[rsx11m]

As you pointed out in comment #17, the behavior of the Enter key in a "Paragraph" context and actually enforcing that context as default are two different things. We can accept one without getting the other. Bug 1265534 attachment 8742525 [details] [diff] [review] only enables the pref for Mail & News but doesn't add the message-body manipulations which were added in bug 330891 into SeaMonkey's MsgComposeCommands.js. Thus, the actual impact of that change is much less, given that the user has to explicitly switch from "Body Text" to "Paragraph" mode, but then (given "true" default of the pref) get the benefit of expected Enter and Shift+Enter behavior once in that mode.

If we were to also port the MsgComposeCommands.js changes, I'd still vote for that pref to be "false" given that the default is enforcing "Paragraph" mode in this case.

Comment 27

[Jorg K (CEST = GMT+2)]

(In reply to rsx11m from comment #26)
> If we were to also port the MsgComposeCommands.js changes, I'd still vote
> for that pref to be "false" given that the default is enforcing "Paragraph"
> mode in this case.
IOW: Keep what was landed and land it on further repositories, right?

Comment 28

[rsx11m]

Yes, but only *if* SeaMonkey would copy your bug 330891 solution entirely, which is not the case.

Are you trying to convince me to port the code enforcing "Paragraph" as the default to SeaMonkey's MsgComposeCommands.js as well, which is arguably the more disputed change? 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).

So, for "not using the same pref" the answer to comment #27 would be "No" instead.

Comment 29

[Jorg K (CEST = GMT+2)]

I'm utterly confused now. I don't want to convince you of anything. These are the facts:

- editor.CR_creates_new_p is just a preference, you can use it as you deem fit.
  In SM there are two editors, the mail editor and the HTML editor. It's
  already used for the HTML editor.
- The TB implementation has two independent parts:
  1) communicate the preference to the editor at start up
  2) create a paragraph for new e-mail compositions.
  I don't know which part you want to adopt in SM, apparently just 1).
As Magnus pointed out, letting the HTML editor produce paragraphs makes a lot of sense. I think consistent behaviour in e-mail makes the same sense, you'd just have to explain the preference better.
Since your user base is rather conservative, maybe changing the default is not a good idea.

I don't really care what we do, that's why I approved the change in the first place. However, is appears good to me that TB and editor/ become independent, and I already said so in comment #9.

If it were my decision I'd go ahead with this change. That way SM becomes independent of TB and that's a good thing.

If you still want to back it out, let me know or you can do it yourself.

If it's not backed out by Saturday night, I will land it on Aurora on Sunday morning.

Comment 30

[rsx11m]

Yeah, it appears that we keep confusing each other...  :-\

> I don't know which part you want to adopt in SM, apparently just 1).

Correct, where I'd also think that adopting 2) as an *option* would make sense if users prefer to utilize that mode by default. I'd just not like to *make* it the default mode for all messages.

> Since your user base is rather conservative, maybe changing the default is not a good idea.
> [...] If it were my decision I'd go ahead with this change. That way SM becomes independent of TB
> and that's a good thing.

From that perspective, yes. So, let's wait until the weekend if anybody else chimes in, then go ahead with this for comm-aurora at least before the merge. If it doesn't land on comm-beta that's probably ok as we have control over comm-release for 2.42 and 2.43 (that's for IanN and ewong to decide then).

In parallel, I'd file another spin-off bug for SeaMonkey (or handle in bug 1265534)
  - whether or not to support a "Paragraph by default" mode in SeaMonkey Mail & News
  - whether or not to base it on a separate mail-specific preference
  - and what the default(s) for the pref(s) involved shall be

Comment 31

[Philip Chee]

So this patch flips the default in editor back to what it was (as far as SeaMonkey is concerned). That's good. We can discuss what we want to do in bug 1265534 or wherever.

Comment 32

[Jorg K (CEST = GMT+2)]

OK, we keep this then and I'll uplift on Sunday/tomorrow.

Comment 33

[rsx11m]

Thanks Phil, I can land this on comm-aurora a bit later today. We yet have to figure out what to do for 2.42/2.43 to avoid a gap, but either will be on comm-release next week anyway.

Comment 34

[Jorg K (CEST = GMT+2)]

Let me land it with some other changes on Sunday, please.

Comment 35

[rsx11m]

Ok, no problem.

Comment 36

[Jorg K (CEST = GMT+2)]

Aurora (TB 47):
https://hg.mozilla.org/releases/comm-aurora/rev/1549ca7806d2

Comment 37

[rsx11m]

Thanks!

Comment 38

[Kent James (:rkent)]

Comment on attachment 8742125 [details] [diff] [review]
Easy fix

http://hg.mozilla.org/releases/comm-esr45/rev/e1a8c2a21f41

Comment 39

[Jorg K (CEST = GMT+2)]

Comment on attachment 8742125 [details] [diff] [review]
Easy fix

Looks like we're not landing this on beta (TB 46).

Comment 40

[rsx11m]

Fixed for SeaMonkey 2.43 on the default branch:
http://hg.mozilla.org/releases/comm-release/rev/57fddbdfd40d

Comment 41

[rsx11m]

Comment on attachment 8742125 [details] [diff] [review]
Easy fix

Guess I can cancel the request for comm-release given that it landed on the default branch for 2.43, and a 2.42 release would either come from THUNDERBIRD451b1_2016042214_RELBRANCH or comm-esr45, where this patch was pushed already.