1353326 - Port bug 1297414 |Generate <p>/<div> for newlines, not <br> (defaultParagraphSeparator)| to Thunderbird - TEST-UNEXPECTED-FAIL | C:\slave\test\build\tests\mozmill\cloudfile\test-cloudfile-attachment-urls.js

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 #1297414 +++

I'm not sure what's required, but something will be required ;-)

We might end up calling
  document.execCommand("defaultparagraphseparator", false, "br/p")
depending on whether we're in paragraph or "body text" mode when initialising our compose window in MsgComposeCommands.js.

Note that having nsIHTMLEditor::GetDefaultParagraphSeparator() available might also be the key to fixing bug 1271835 by modifying HTMLEditRules::SplitMailCites().

Comment 1

[Jorg K (CEST = GMT+2)]

Bug 1297414 has landed and sure enough has caused test failures. I whole heap of tests fail in test-cloudfile-attachment-urls.js since that tests the DOM of the compose window, and if it doesn't find the <br>s it expects, the tests fail.

Comment 2

[Jorg K (CEST = GMT+2)]

Attachment: 1353326-defaultparagraphseparator.patch (v1).

With luck, this is all that's needed.

Comment 3

[Jorg K (CEST = GMT+2)]

Attachment: 1353326-defaultparagraphseparator.patch (v2). [landed in comment #9]

This works for me, let's see what the tests say:

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=52bcc81ee83b131f22d7112ca2cf68a20eb6a1b2
(same code but different comment and commit message).

Comment 4

[Jorg K (CEST = GMT+2)]

Attachment: 1353326-defaultparagraphseparator.patch (v3).

This is quite aggressive. When you're in paragraph mode, any <enter> will go and insert a new paragraph. That's nice if you split a block quote in a reply for example.

Let me know what you think. v2 just restores the previous behaviour, this patch takes advantage of the the new functionality in the M-C editor. Maybe I like this better.

Comment 5

[Jorg K (CEST = GMT+2)]

Oops, v3 should have this commit message:
Bug 1353326 - set defaultparagraphseparator based on pref mail.compose.default_to_paragraph. r=aceman

Comment 6

[:aceman]

I'm no user of HTML composing so I can't answer straight away. I need to play with this later.

Comment 7

[Jorg K (CEST = GMT+2)]

Try:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=b683630f50fbeb5e8b6be537f37034ca2477c1e3

I think the v3 is probably better. We've had some complaints that if the user has chosen paragraph mode, it gets lost all the time: Bug 1271835, bug 1304366.

Using M-C's new feature goes a long way towards fixing this.

Comment 8

[:aceman]

Comment on attachment 8854629 [details] [diff] [review]
1353326-defaultparagraphseparator.patch (v3).

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

RealRaven, do you have an opinion on this?

Comment 9

[Jorg K (CEST = GMT+2)]

Landed temporary fix to restore TB to working condition:
https://hg.mozilla.org/comm-central/rev/6e0ab7b34003c57db804bdb7c48ee3aad171a69e

Updated patch for the more aggressive behaviour coming.

Comment 10

[Jorg K (CEST = GMT+2)]

Attachment: 1353326-defaultparagraphseparator.patch (v3b) [triggers M-C bug, can't be used, comment #17]

OK, this is the more aggressive behaviour. If you hit enter in paragraph mode, you'll get a new paragraph, even if you're in body text.

If you split a blockquote, at first, body text is inserted, but with the next <enter>, we go back to paragraph mode. That also applies after adding, for example, a <hr>.

All hard to imagine, so a try run for Axel will be here in a few hours:
https://archive.mozilla.org/pub/thunderbird/try-builds/[e-mail]-d9f61759ab42ea2d58496b89297f1a99daaf4c6c/

Comment 11

[Jorg K (CEST = GMT+2)]

Whatever we decide to do here, this will need porting to SM.

Comment 12

[Axel Grude [:realRaven]]

Comment on attachment 8854629 [details] [diff] [review]
1353326-defaultparagraphseparator.patch (v3).

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

R+ Excellent, if it does what I think it should.

With the setting enabled it should:

 - insert a paragraph when splitting a quote.
 - add a paragraph when hitting enter in any <h*> tag
 - (when not in quote) split paragraphs into 2 while copying style / class attributes [I think this one is already existing behavior]

Shift Enter should force a <br>

Ideally this setting should be enabled as per default (just like the paragraph setting is). It might even use the same setting, as usually if we are interested in paragraph editing it's an "all or nothing" decision.

Comment 13

[Axel Grude [:realRaven]]

Comment on attachment 8854727 [details] [diff] [review]
1353326-defaultparagraphseparator.patch (v3b) [triggers M-C bug, can't be used, comment #17]

R+ If I understand correctly, with the setting enabled it should:

 - insert a paragraph when splitting a quote.
 - add a paragraph when hitting enter in any <h*> tag

Shift Enter should force a <br>

Comment 14

[Axel Grude [:realRaven]]

(In reply to Jorg K (GMT+2) from comment #10)
> Created attachment 8854727 [details] [diff] [review]
> 1353326-defaultparagraphseparator.patch (v3b).

> If you split a blockquote, at first, body text is inserted, but with the
> next <enter>, we go back to paragraph mode. That also applies after adding,
> for example, a <hr>.

why? having to hit Enter twice is not intuitive. I vote for inserting a <p> in right away on the first Enter; if you want to split a block quote let's say you want to insert a table or image, which is an exception, then use SHIFT+Enter.

Likewise if you want to split the quote to insert a heading you would enter => creates <p>
then select <heading> which will replace the <p>

Ideally you should never land in the "top level" of the body, text should always be in at least one block level container (div, p, h).

Comment 15

[Jorg K (CEST = GMT+2)]

(In reply to Axel Grude [:realRaven] from comment #12)
> R+ Excellent, if it does what I think it should.
Please try it out:
https://archive.mozilla.org/pub/thunderbird/try-builds/[e-mail]-d9f61759ab42ea2d58496b89297f1a99daaf4c6c/
The build is now complete.

> Ideally this setting should be enabled as per default (just like the
> paragraph setting is). It might even use the same setting, as usually if we
> are interested in paragraph editing it's an "all or nothing" decision.
That's the intention.

(In reply to Axel Grude [:realRaven] from comment #14)
> > If you split a blockquote, at first, body text is inserted, but with the
> > next <enter>, we go back to paragraph mode. That also applies after adding,
> > for example, a <hr>.
> why? having to hit Enter twice is not intuitive. I vote for inserting a <p>
> in right away on the first Enter; if you want to split a block quote let's
> say you want to insert a table or image, which is an exception, then use
> SHIFT+Enter.
Because bug 1271835 doesn't automatically get fixed here. For that, I'd have to make more changes in M-C's HTMLEditRules::SplitMailCites(). With the enabling technology from bug 1297414 now in place, this has just become easier.

As a fallout of bug 1297414, the proposal here will get you into a paragraph at some stage, but not straight away. I think that's certainly an improvement over what we have now.

Comment 16

[Axel Grude [:realRaven]]

Attachment: test of build from comment 10

Here is a test of the build you made (thanks for making the build!)

Method:
# set up a paragraph style (blue, big) to illlustrate.
# split quote with enter
# multiple Enter keystrokes don't do anything (note that this also applied in the HTML editor, more Enter strokes did not generate new lines)
# _after_ typing and hitting Enter the <p> is inserted (and changes the style to the paragraph's correct attributes

Comment 17

[Jorg K (CEST = GMT+2)]

Comment on attachment 8854769 [details]
test of build from comment 10

Thanks for checking it out. Nice video.

> Method:
> # set up a paragraph style (blue, big) to illlustrate.
> # split quote with enter
> # multiple Enter keystrokes don't do anything (note that this also applied
> in the HTML editor, more Enter strokes did not generate new lines)
> # _after_ typing and hitting Enter the <p> is inserted (and changes the
> style to the paragraph's correct attributes
Yes, I hadn't noticed that. Seems to be a bug in M-C. I'll file a bug.

So based on this, we can't use the patch.

Comment 18

[Jorg K (CEST = GMT+2)]

Comment on attachment 8854727 [details] [diff] [review]
1353326-defaultparagraphseparator.patch (v3b) [triggers M-C bug, can't be used, comment #17]

Based on comment #17, we can't use this patch.

Comment 19

[Jorg K (CEST = GMT+2)]

Let's close this bug, we re-established the previous behaviour and fixed the tests. All further action in a new bug or bug 1271835.

Only landing here:
https://hg.mozilla.org/comm-central/rev/6e0ab7b34003c57db804bdb7c48ee3aad171a69e

Comment 20

[Jorg K (CEST = GMT+2)]

Before we can use the more aggressive approach, M-C need to fix bug 1353695.
Also see bug 1297414 comment #77.
Axel, many thanks for the quick testing and finding a bug!

Just in general, if it weren't for that bug, would you like to go with the patch that forces a paragraph *after* you hit enter? Not the ideal solution and not a complete fix of bug 1271835, but a step into the right direction.

What do you think?

Comment 21

[Jorg K (CEST = GMT+2)]

(In reply to Jorg K (GMT+2) from comment #20)
> Before we can use the more aggressive approach, M-C need to fix bug 1353695.
> Also see bug 1297414 comment #77.
> Axel, many thanks for the quick testing and finding a bug!
Bug 1353695 has a patch that also fixes the problem when splitting the blockquotes that Axel discovered.

> Just in general, if it weren't for that bug, would you like to go with the
> patch that forces a paragraph *after* you hit enter? Not the ideal solution
> and not a complete fix of bug 1271835, but a step into the right direction.
Axel and Aceman: I need your opinions, do we want to land attachment 8854727 [details] [diff] [review] in another bug once bug 1353695 is fixed? I'm in favour.

Comment 22

[Axel Grude [:realRaven]]

(In reply to Jorg K (GMT+2) from comment #21)
> (In reply to Jorg K (GMT+2) from comment #20)
> > Before we can use the more aggressive approach, M-C need to fix bug 1353695.
> > Also see bug 1297414 comment #77.
> > Axel, many thanks for the quick testing and finding a bug!
> Bug 1353695 has a patch that also fixes the problem when splitting the
> blockquotes that Axel discovered.
> 
> > Just in general, if it weren't for that bug, would you like to go with the
> > patch that forces a paragraph *after* you hit enter? Not the ideal solution
> > and not a complete fix of bug 1271835, but a step into the right direction.
> Axel and Aceman: I need your opinions, do we want to land attachment 8854727 [details] [diff] [review]
> [details] [diff] [review] in another bug once bug 1353695 is fixed? I'm in
> favour.

Yes - because the actual bug needs to be fixed within the editor component anyway.

Comment 23

[Jorg K (CEST = GMT+2)]

Moved the more aggressive behaviour to bug 1354002.

Comment 24

[Frank-Rainer Grahl (:frg)]

Put a fix in my local suite tree and will test it later. Clearing NI for now.