1354002 - Set execCommand("defaultparagraphseparator", false, "br/p") depending on mail.compose.default_to_paragraph

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, enhancement, P3)

Description

[Jorg K (CEST = GMT+2)]

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

Users who have chosen to compose in paragraph format will get paragraphs when hitting enter. This applies to HTML composition only.

This goes 50% of the way towards fixing bug 1271835, but we still need to fix HTMLEditRules::SplitMailCites() for the remaining 50%.

Comment 1

[Jorg K (CEST = GMT+2)]

Attachment: 1354002-defaultparagraphseparator.patch (v1).

Comment 2

[Jorg K (CEST = GMT+2)]

Comment on attachment 8855184 [details] [diff] [review]
1354002-defaultparagraphseparator.patch (v1).

Magnus likes to compose in paragraph mode, too, so let's get his feedback.

This is the "odd" effect introduced here:
1. Reply to a message.
2. Click in a blockquote.
3. Hit enter, block quote is split, you get "Body Text" between the two resulting blockquotes.
4. Type something, then hit enter. The line and the previous line get turned into paragraphs.

Note: Until bug 1353695 is fixed, it only works when you type something and then hit enter, don't hit enter straight away in 4).

Not perfect, but better than being stuck with "Body Text".

Comment 3

[:aceman]

So do we need to wait for bug 1353695 to be fixed?

Comment 4

[Jorg K (CEST = GMT+2)]

Yes. You can review it now, you can also apply the fix from that bug to M-C for testing. I did ;-)

Comment 5

[:aceman]

Comment on attachment 8855184 [details] [diff] [review]
1354002-defaultparagraphseparator.patch (v1).

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

OK, I tested it with the m-c patch and it seems to work.
It is strange that the typed line is only converted to <p> in step 4) after typing Enter at the end of it, but probably better than nothing.

Comment 6

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #5)
> It is strange that the typed line is only converted to <p> in step 4) after
> typing Enter at the end of it, but probably better than nothing.
Indeed, better than nothing. More to come in bug 1271835.

Thanks for the review, I won't land this until bug 1353695 has landed.

Comment 7

[Magnus Melin [:mkmelin] (away, back June 30)]

Comment on attachment 8855184 [details] [diff] [review]
1354002-defaultparagraphseparator.patch (v1).

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

Yes, I like it!

Comment 8

[Jorg K (CEST = GMT+2)]

Me too. I'll land it in while since bug 1353695 has landed.

Comment 9

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/ae3cb512e8d286c2161d44a8ac15ffffa96b9c29

Comment 10

[Jorg K (CEST = GMT+2)]

I wish it worked better, since I've just discovered bug 1358228.

Comment 11

[Jorg K (CEST = GMT+2)]

Backout:
https://hg.mozilla.org/comm-central/rev/831a455bfe2f76768f0b5b281bfb9e4430058dce

This change causes the following unwanted side effect:

In a reply three paragraphs are inserted instead of one. When replying with a signature, three paragraphs are inserted before the signature. When replying without a signature, three paragraphs are inserted before the blockquote, which is visible, or after the blockquote, which is not noticeable at first.

Comment 12

[Jorg K (CEST = GMT+2)]

The side effect described in comment #11 is not the only one. Maybe it's related to another undesired effect I documented in bug 1361008.

Comment 13

[Jorg K (CEST = GMT+2)]

Bug 1361008 has landed but with the patch here applied, I'm still seeing three paragraphs inserted as per comment #11. Looks like I have to file more editor bugs :-(

Comment 14

[Jorg K (CEST = GMT+2)]

(In reply to Jorg K (GMT+2) from comment #13)
> Looks like I have to file more editor bugs :-(
Nope, that was wrong. What happens is that InsertLineBreak() inserts <p> instead for <br> and the clean-up that previously removed unwanted <br>s now doesn't work any more.

So there needs to be a better control of those InsertLineBreak() calls in nsMsgCompose::ConvertAndLoadComposeWindow().

I'm onto it.

Comment 15

[Jorg K (CEST = GMT+2)]

Attachment: 1354002-defaultparagraphseparator.patch (v2).

This should not add extra paragraphs when replying.

InsertLineBreak() inserts paragraphs instead of line breaks when defaultparagraphseparator is set to "p". So we avoid calling this in paragraph mode in a HTML composition.

m_editor->BeginningOfDocument() moves to the beginning of the document but into the first container, MoveToBeginningOfDocument() moves to the beginning but before the first container which is where we want to insert a paragraph.

I've tested these cases:
HTML and plaintext reply above the quote with signature above or below the quote (four cases). The same for forward.

HTML and plaintext reply below the quote with signature below the quote (two cases). Same for forward.

I've run
mozmake SOLO_TEST=composition/test-signature-updating.js mozmill-one
mozmake SOLO_TEST=cloudfile/test-cloudfile-attachment-urls.js mozmill-one
and they passed.

Full run here:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=8ae5c5086ef8ee83d59912ea0577006fa862412e

P.S.: This stuff is a real pain :-(

Comment 16

[Jorg K (CEST = GMT+2)]

Comment on attachment 8878882 [details] [diff] [review]
1354002-defaultparagraphseparator.patch (v2).

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

::: mail/components/compose/content/MsgComposeCommands.js
@@ -424,5 @@
>        }
>  
>        editor.enableUndo(false);
>  
> -      // Delete a <br> if we see one.

Now since no <br>s are inserted any more, this code can go.

Comment 17

[:aceman]

Comment on attachment 8878882 [details] [diff] [review]
1354002-defaultparagraphseparator.patch (v2).

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

Seems to work for me. It seems you move more logic into the cpp backend. Will it work in Seamonkey (I'm not sure it has the paragraph format) ?

::: mail/components/compose/content/MsgComposeCommands.js
@@ +5351,5 @@
>      Services.prefs.getBoolPref("editor.CR_creates_new_p");
> +  editor.document.execCommand("defaultparagraphseparator", false,
> +    gMsgCompose.composeHTML &&
> +    Services.prefs.getBoolPref("mail.compose.default_to_paragraph") ?
> +                               "p" : "br");

This looks ugly. Any way to rewrap this or use a temp variable?
Also why remove all the comments? All the other calls here have descriptions.

Comment 18

[Jorg K (CEST = GMT+2)]

Hmm, that hunk already had r+ in comment #5 and it got landed. Ian ported it 1:1 here:
https://hg.mozilla.org/comm-central/rev/d67289948650#l1.15
I agree it's a little on the ugly side. I tried rewrapping it but didn't come up with anything pretty.

I removed the comment explaining why we hard-coded "br", now I could only write a comment describing what we do, since it seems pretty obvious to set the default paragraph separator to "p" if in paragraph mode.

As for SM, they have bug 1357920, so it I reland this as well, they will be fine.

Comment 19

[Jorg K (CEST = GMT+2)]

Oh, one more thing: I'm not moving more logic into the cpp backend. All I'm doing is to suppress calling InsertLineBreak() since with the paragraph setting it doesn't do what it traditionally did.

Comment 20

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/dd97865d7a80a00490e1cf1226b05ca6d2fe18bc

Let's see whether it sticks this time ;-)