1330796 - Mail editor: Enter key should insert an invisible <br> at the end of a <span style="display: block;"> - Fix HTMLEditRules::SplitMailCites()

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect, P3)

Description

[Jorg K (CEST = GMT+2)]

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

This bug was reported for Thunderbird, STR:

Answer a short BMO mail in plaintext mode. Click in the middle of the mail, select to the end, delete selection. Hit <enter> once to get to the next line after the quoted text. Type something. Note that the quote is blue, the new text is black, so the text didn't go into the span. Send. Result: Typed line joined to quote although the typed line was displayed in the line after the quote.

The problem is the DOM produced by hitting <enter> while in the span. There is no <br> inserted in the <span>, instead, the selection/insertion point is moved just outside the span. Since the span is "display: block", the caret ends up on the next line.

But when serialising this, the "block-ness" of the span is ignored and the quote (inside span) and the following text (displayed on the next line) are joined.

The solution is to insert an invisible break inside the "block" span.

The processing in HTMLEditRules::SplitMailCites() needs to changed slightly.

It's impossible to demonstrate the problem in a plain HTML document, since SplitMailCites() only runs for mail editors, see here:
http://searchfox.org/mozilla-central/rev/3f614bdf91a2379a3e2c822da84e524f5e742121/editor/libeditor/HTMLEditRules.cpp#1489

This is no regression, since this behaviour was always wrong, only quotes/cites never used to be blocks. Since bug 1288911 they are, so we're finding all sorts of problems.

Comment 1

[Jorg K (CEST = GMT+2)]

Attachment: Proposed solution (v1).

OK, this works for me.

Masayuki-san, this is difficult territory. I know you use TB, but I don't know whether you compile it. Of course, HTMLEditRules::SplitMailCites() is only run in a mail editor, see
http://searchfox.org/mozilla-central/rev/3f614bdf91a2379a3e2c822da84e524f5e742121/editor/libeditor/HTMLEditRules.cpp#1489
and IsMailEditor() tests |mFlags & nsIPlaintextEditor::eEditorMailMask|.

Do you want a test? I could write a Mochitest, I think, that reaches into those flags and sets that flag so the editor will behave like in a mail message. But then, none of this code has tests as far as I know. I could do a test that exercises a few cases, but here I'm really interested in what happens at the end of the left span.

Please let me know.

Comment 2

[Jorg K (CEST = GMT+2)]

I noticed that clicking in the middle of a quote and hitting "enter" without this patch inserts two visible empty lines since two breaks are inserted, the second one by this code here:
        if (wsType == WSType::normalWS || wsType == WSType::text ||
            wsType == WSType::special) {
          NS_ENSURE_STATE(mHTMLEditor);
          brNode = mHTMLEditor->CreateBR(selNode, newOffset);
          NS_ENSURE_STATE(brNode);
        }

The patch (accidentally) fixes this, too, you only get one line:
...left... <br></span><br><span ... right ...>
            (1)        (2)
(1) is inserted by the new code, (2) is inserted by the existing code and it is required, since without it, newly entered text would go into the right span.

So it's all good.

Comment 3

[Jorg K (CEST = GMT+2)]

Attachment: 1330796-mailcite-test.patch (v1).

Here's a mochitest for six cases:
Click at the start, middle and end of a quote and hit enter.
Do this for quotes displayed at blocks and inline.
It turns out that the block case works with the new code, but the existing inline case doesn't. That can even be reproduced with a current version of TB, say, TB 45.6, which has nothing of the display:block business: Delete a quote to the very end, make sure there is no <br> at the end, hit enter and type something. Result: no <br> before what was typed. The new text is glued to the previous line. So the last test fails here.

Comment 4

[Jorg K (CEST = GMT+2)]

Comment on attachment 8826749 [details] [diff] [review]
Proposed solution (v1).

Cancelling feedback for now since I need to fix another bug I detected with the test and which can also be reproduced manually in versions of TB which don't use the display:block yet.

Comment 5

[Jorg K (CEST = GMT+2)]

Attachment: 1330796-mailcite.patch, code + test (v1b).

Here we have code and test in one patch. A little tweak also fixed the inline case (last test case).

Comment 6

[Jorg K (CEST = GMT+2)]

Attachment: 1330796-mailcite.patch, code + test (v1c).

I added two more tests, sorry about the noise. This should be good for review.
Try run here:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6a7786d1c4dca475dbe800cc921569e71df7376b

Comment 7

[Jorg K (CEST = GMT+2)]

The try looks OK, I'd like to say "green", but the tier-2 is all orange :-(
A quick review would be much appreciated since I need this uplifted to mozilla52 (like bug 1288911 and bug 1328093) for TB 52 ESR and the branch date (Jan 23rd) is fast approaching.

Comment 8

[Alfred Peters [:infofrommozilla]]

Attachment: Quotedtext.PNG - Quote with attachment 8826906

On the left you see the Editor and on the right the Result.

(In reply to Jorg K (GMT+1) from comment #6)
> Created attachment 8826906 [details] [diff] [review]
> 1330796-mailcite.patch, code + test (v1c).

At part 'A' I entered [ENTER] + "Text A"
At part 'B' I entered [ENTER] + "Text B" plus an '>' at the beginning of the first quoted Line.

In both cases there is an additional line break at the Beginning of the quote.

Comment 9

[Alfred Peters [:infofrommozilla]]

(In reply to Alfred Peters from comment #8)

Cancel this.
Sorry, it was my fault.

Comment 10

[Jorg K (CEST = GMT+2)]

Attachment: Two screenshots comparing compose window with sent message

(In reply to Alfred Peters from comment #9)
> Cancel this. Sorry, it was my fault.
Thanks for testing. Looks like you were comparing the compose window with the sent message. I'm not sure how you managed to produce the additional space between the inserted text and the rest of the quote. You must have applied some CSS trick, since the quoted text is blue (and smaller) in the sent message, which is non-standard.

I did the same and it looks absolutely fine for me.

Comment 11

[Alfred Peters [:infofrommozilla]]

(In reply to Jorg K (GMT+1) from comment #10)

> the sent message. I'm not sure how you managed to produce the additional
> space between the inserted text and the rest of the quote. You must have
> applied some CSS trick,

Exactly, I should have checked the RAW view first.

> I did the same and it looks absolutely fine for me.

Just like here. Windows 10 - TB Trunk and SM Trunk.

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8826906 [details] [diff] [review]
1330796-mailcite.patch, code + test (v1c).

># HG changeset patch
># User Jorg K <[e-mail]>
># Parent  2963cf6be7f830c0d2155e2968cfc53585868a76
>Bug 1330796 - Add invisible break to span of display:block in HTMLEditRules::SplitMailCites. r=masayuki
>
>diff --git a/editor/libeditor/HTMLEditRules.cpp b/editor/libeditor/HTMLEditRules.cpp
>--- a/editor/libeditor/HTMLEditRules.cpp
>+++ b/editor/libeditor/HTMLEditRules.cpp
>@@ -23,16 +23,17 @@
> #include "mozilla/mozalloc.h"
> #include "nsAutoPtr.h"
> #include "nsAString.h"
> #include "nsAlgorithm.h"
> #include "nsCRT.h"
> #include "nsCRTGlue.h"
> #include "nsComponentManagerUtils.h"
> #include "nsContentUtils.h"
>+#include "nsIFrame.h"

Please keep a-z order of header file name. It's helpful to check if necessary header file is already included.

> #include "nsDebug.h"
> #include "nsError.h"
> #include "nsGkAtoms.h"
> #include "nsIAtom.h"
> #include "nsIContent.h"
> #include "nsIContentIterator.h"
> #include "nsID.h"
> #include "nsIDOMCharacterData.h"
>@@ -1705,24 +1706,41 @@ HTMLEditRules::SplitMailCites(Selection*
>     }
> 
>     NS_ENSURE_STATE(mHTMLEditor);
>     NS_ENSURE_STATE(selNode->IsContent());
>     int32_t newOffset = mHTMLEditor->SplitNodeDeep(*citeNode,
>         *selNode->AsContent(), selOffset, HTMLEditor::EmptyContainers::no,
>         getter_AddRefs(leftCite), getter_AddRefs(rightCite));
>     NS_ENSURE_STATE(newOffset != -1);
>+
>+    // Add an invisible <br> to the end of the left part if it was a <span> of
>+    // style="display: block". This is important, since when serialising the
>+    // cite to plain text, the span which caused the visual break is discarded.
>+    // So the added <br> will guarantee that the serialiser will insert a
>+    // break where the user saw one.
>+    if (leftCite &&
>+        leftCite->IsHTMLElement(nsGkAtoms::span) &&
>+        leftCite->GetPrimaryFrame()->IsFrameOfType(nsIFrame::eBlockFrame)) {
>+       nsCOMPtr<nsINode> lastChild = leftCite->GetLastChild();
>+       if (lastChild && !lastChild->IsHTMLElement(nsGkAtoms::br)) {
>+         nsCOMPtr<Element> invisBR = mHTMLEditor->CreateBR(leftCite, leftCite->Length());

nit: too long line, please break after |=|.

I wonder, doesn't cause this unused variable warning? If so, please add |Unused << invisBR;| to next line.

>@@ -1732,39 +1750,43 @@ HTMLEditRules::SplitMailCites(Selection*
>                              &visOffset, &wsType);
>       if (wsType == WSType::normalWS || wsType == WSType::text ||
>           wsType == WSType::special) {
>         NS_ENSURE_STATE(mHTMLEditor);
>         WSRunObject wsObjAfterBR(mHTMLEditor, selNode, newOffset+1);
>         wsObjAfterBR.NextVisibleNode(selNode, newOffset + 1,
>                                      address_of(visNode), &visOffset, &wsType);
>         if (wsType == WSType::normalWS || wsType == WSType::text ||
>-            wsType == WSType::special) {
>+            wsType == WSType::special ||
>+            // In case we're at the very end.
>+            wsType == WSType::thisBlock) {

I wonder, switch-case looks better for here, but perhaps, we should do that in another bug.

>diff --git a/editor/libeditor/tests/test_bug1330796.html b/editor/libeditor/tests/test_bug1330796.html

Thank you very much for adding automated test!

>new file mode 100644
>--- /dev/null
>+++ b/editor/libeditor/tests/test_bug1330796.html
>@@ -0,0 +1,89 @@
>+<!DOCTYPE HTML>
>+<html>
>+<!--
>+https://bugzilla.mozilla.org/show_bug.cgi?id=1330796
>+-->
>+<head>
>+  <meta charset="utf-8">
>+  <title>Test for Bug 772796</title>
>+  <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
>+  <script type="application/javascript" src="/tests/SimpleTest/EventUtils.js"></script>
>+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
>+  <style> .pre { white-space: pre } </style>
>+</head>
>+<body>
>+<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=772796">Mozilla Bug 1330796</a>
>+<p id="display"></p>
>+<div id="content" style="display: none">
>+</div>
>+
>+<div id="editable" contenteditable></div>
>+
>+<pre id="test">
>+
>+<script type="application/javascript">
>+var tests = [

Could you add comment here for explaining what each item means?

>+  // With style="display: block;".
>+  [ "<span _moz_quote=true style=\"display: block;\">&gt; mailcite<br></span>", 0,
>+    "x<br><span style=\"display: block;\">&gt; mailcite<br></span>" ],
>+  [ "<span _moz_quote=true style=\"display: block;\">&gt; mailcite<br></span>", 5,
>+    "<span style=\"display: block;\">&gt; mai<br></span>x<br><span style=\"display: block;\">lcite<br></span>"],
>+  [ "<span _moz_quote=true style=\"display: block;\">&gt; mailcite<br></span>", 10,
>+    "<span style=\"display: block;\">&gt; mailcite<br></span>x<br>" ],
>+  // No <br> at the end to simulate prior deletion to the end of the quote.
>+  [ "<span _moz_quote=true style=\"display: block;\">&gt; mailcite</span>", 10,
>+    "<span style=\"display: block;\">&gt; mailcite<br></span>x<br>" ],
>+
>+  // Without style="display: block;".
>+  [ "<span _moz_quote=true>&gt; mailcite<br></span>", 0,
>+    "x<br><span>&gt; mailcite<br></span>" ],
>+  [ "<span _moz_quote=true>&gt; mailcite<br></span>", 5,
>+    "<span>&gt; mai</span><br>x<br><span>lcite<br></span>" ],
>+  [ "<span _moz_quote=true>&gt; mailcite<br></span>", 10,
>+    "<span>&gt; mailcite<br></span>x<br>" ],
>+  // No <br> at the end to simulate prior deletion to the end of the quote.
>+  [ "<span _moz_quote=true>&gt; mailcite</span>", 10,
>+    "<span>&gt; mailcite</span><br>x<br>" ]
>+];

Although, I'm surprised at removing _moz_quote attribute...

>+
>+/** Test for Bug 1330796 **/
>+
>+SimpleTest.waitForExplicitFinish();
>+
>+SimpleTest.waitForFocus(function() {
>+
>+  var sel = window.getSelection();
>+  var theEdit = document.getElementById("editable");
>+  makeMailEditor();
>+
>+  for (i = 0; i < tests.length; i++) {
>+    theEdit.innerHTML = tests[i][0];
>+    theEdit.focus();
>+    var theText = theEdit.firstChild.firstChild;
>+    // Position set at the beginning , middle and end of the text.
>+    sel.collapse(theText, tests[i][1]);
>+
>+    synthesizeKey("VK_RETURN", {});

nit: Could you use "KEY_Enter" in new test?

>+    synthesizeKey("x", {});
>+    is(theEdit.innerHTML, tests[i][2], "unexpected HTML for test " + i.toString());

Please add which test's result at head of the message. When one of the tests fails during writing a patch, it's helpful if developer can check which test fails easier.

Comment 13

[Jorg K (CEST = GMT+2)]

Attachment: 1330796-mailcite.patch, code + test (v1d).

Thank you so much for the quick review. Much appreciated.

Carrying forward Masayuki's r+ with the issues/nits addressed.

(In reply to Masayuki Nakano [:masayuki] from comment #12)
> Please keep a-z order of header file name. It's helpful to check if
> necessary header file is already included.
Done.

> >+         nsCOMPtr<Element> invisBR = mHTMLEditor->CreateBR(leftCite, leftCite->Length());
> nit: too long line, please break after |=|.
Done.

> I wonder, doesn't cause this unused variable warning?
Not on Windows, and the try on Linux worked, too. I'll send of a Mac build just to be sure.

> Thank you very much for adding automated test!
You wouldn't have passed the patch without it, so I had no choice ;-)
Besides, the test found another bug.

> Could you add comment here for explaining what each item means?
I'll try ;-)

> Although, I'm surprised at removing _moz_quote attribute...
That's because it's internal, right?

> nit: Could you use "KEY_Enter" in new test?
Done, I didn't know that existed.

> >+    is(theEdit.innerHTML, tests[i][2], "unexpected HTML for test " + i.toString());
> Please add which test's result at head of the message. When one of the tests
> fails during writing a patch, it's helpful if developer can check which test
> fails easier.
I don't think that's necessary. Artificially adding "huhu" to the end of one expected result gives this:
10 INFO TEST-UNEXPECTED-FAIL | editor/libeditor/tests/test_bug1330796.html | unexpected HTML for test 7 - got "<span>&gt; mailcite</span><br>x<br>", expected "<span>&gt; mailcite</span><br>x<br>huhu"

Here's the Mac build:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=daba26ffd85afadc6583c9b823ff8b19e01654ec

Masayuki-san, if you're still around, you could check the changes in the interdiff.

Comment 14

[Jorg K (CEST = GMT+2)]

Attachment: 1330796-mailcite.patch, code + test (v1d-correct)

Wrong file uploaded, not sure how that happened. So interdiff between v1c and v1d-correct.

Comment 15

[Jorg K (CEST = GMT+2)]

https://bugzilla.mozilla.org/attachment.cgi?oldid=8826906&action=interdiff&newid=8827093&headers=1

Comment 16

[Jorg K (CEST = GMT+2)]

Attachment: 1330796-mailcite.patch, code + test (v1e).

Sigh, another iteration, since due to some grammar errors in the comment.
So compare v1c to v1e.

Comment 17

[Jorg K (CEST = GMT+2)]

Mac compile also green and no warnings in the log.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3110a34c9897
Add invisible break to span of display:block in HTMLEditRules::SplitMailCites. r=masayuki

Comment 19

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/3110a34c9897

Comment 20

[Jorg K (CEST = GMT+2)]

Comment on attachment 8827094 [details] [diff] [review]
1330796-mailcite.patch, code + test (v1e).

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1288911 which uncovered (not caused) this.
[User impact if declined]: In Thunderbird, editing plain text replies (with cites/quotes) is broken in some cases. Very nasty actually, since the sent message doesn't look like what the user saw when writing it.
[Is this code covered by automated tests?]: Yes! This code does not execute in Firefox. However, I added automated tests to make FF behave like TB during the test.
[Has the fix been verified in Nightly?]: Yes, on a Thunderbird Daily (and independently tested by another contributor, see comment #11).
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: Not for Firefox since the code is not used in Firefox. I'm the Thunderbird release manager and I request this to be uplifted to mozilla52 since bug 1288911 (and bug 1328093) kindly got uplifted as well upon my request.
[Why is the change risky/not risky?]: Not risky for Firefox, since code is not run in Firefox.
[String changes made/needed]: None.

I'm really sorry about this long list of uplift requests. First there was bug 387687, then people weren't happy, and I did bug 1288911, and that caused further problems fixed in bug 1328093 and here. I'm hoping that this is the end of it.

Comment 21

[Julien Cristau [:jcristau] (back April 22)]

Comment on attachment 8827094 [details] [diff] [review]
1330796-mailcite.patch, code + test (v1e).

fix some line-break issue for thunderbird, aurora52+

Comment 22

[Jorg K (CEST = GMT+2)]

Attachment: 1330796-follow-up.patch - add key code

Masayuki-san, I've just realised the new KEY_Enter needs a key code. Do you think we should fix this?

Comment 23

[Jorg K (CEST = GMT+2)]

Attachment: 1330796-follow-up.patch - add key code

Oops, added the code to the wrong line. But apparently the "x" should have a code, too.

Comment 24

[Jorg K (CEST = GMT+2)]

Dear Sheriff, perhaps you can wait with the uplift until the follow-up patch (just a minor tweak) has been reviewed and then uplift both patches together.

Comment 25

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8827567 [details] [diff] [review]
1330796-follow-up.patch - add key code

Really better if this defines .code value because it emulates actual keyboard event of the real world (but I guess without the .code value isn't problem. IIRC, nobody hasn't check code value except Spacebar yet).

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c4acba4df2b2
Follow-up: Add key code to KEY_Enter. r=masayuki

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/dcb1213212c1
https://hg.mozilla.org/releases/mozilla-aurora/rev/fd66a1e49411

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c4acba4df2b2