This was raised by Thunderbird users in bug 1248971. See attached sample page for a reproducible case.

Works OK in Chrome.

This works for the simple text case. Let's see what it breaks: https://treeherder.mozilla.org/#/jobs?repo=try&revision=42709485736e

Wrong try syntax, again: https://treeherder.mozilla.org/#/jobs?repo=try&revision=53213d4f93dc

This works and doesn't break anything, so let's ask for review. Robert (:roc) kindly did the last review, but not his nickname says "Exited". His blog says that he left Mozilla to work on "rr". So who's left to review this? It can't be that unpaid volunteers work on this and then there's not even anyone to review. Sorry about the r? and NI SPAM.

(Oops, accidentally changed an unrelated file, sorry)

Comment on attachment 8732654 [details] [diff] [review] Possible solution (v1b) with test. I don't review editor code any more.

Although, I'm not an official reviewer around editor and I don't have enough knowledge around HTMLEditRules yet, but if there is no reviewer, I'll try to review it with reading around the patch. Is this okay to you, Ehsan?

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #7) > Although, I'm not an official reviewer around editor and I don't have enough > knowledge around HTMLEditRules yet, but if there is no reviewer, I'll try to > review it with reading around the patch. Is this okay to you, Ehsan? I exchanged e-mails with Ehsan on this. That should be OK. Thanks, Masayuki!

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #7) > I'll try to review it with reading around the patch. Let me explain how I came up with the patch. I put a breakpoint on Collapse(). This way I could see that there was only one call to Collapse() under the circumstances of the testcase. This Collapse() was unconditionally called like this: // Adjust selection to be right after it. res = aSelection->Collapse(blockParent, offset + 1); In other words: Deleting an empty block via the delete key and via the backspace key were treated exactly the same. Obviously this is wrong since for backspace you want to move the caret up to the preceding text node, if any. So this is what the patch does. It works and I have a green try run for it.

I also have a try run (which I did for another bug) that contains the new test: https://treeherder.mozilla.org/#/jobs?repo=try&revision=45b55ca763d3

Sorry for the delay to review. I'd like to review it tomorrow or Friday. But I'm going to be in a business trip until Sunday, so, I cannot say exactly when I can review it.

OK, I've duped bug 968733 here, so the summary needs to change. Fix is coming in a minute, review cancelled for now since I need to add a test for the delete case.

Here is the fix for the delete case. Will add a test as well, then request review again.

OK, here are the complete fix and tests for both cases. Will add try run in the morning.

(oops, cut/paste error in comment)

New try with with latest patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=484fe22f2c34

Comment on attachment 8734159 [details] [diff] [review] Possible solution (v2) for backspace and delete case, tests for both cases I'm very sorry for the long delay. First, the if condition of the |else| block is checking if the empty block is a list item. If it's list item (i.e., changing <p></p> in your test to <li></li>), I can reproduce same bug but probably your patch won't fix this case. But I think that this is different bug if we take your approach. >@@ -5011,19 +5012,44 @@ nsHTMLEditRules::CheckForEmptyBlock(nsIN > // Adjust selection to be right before it > res = aSelection->Collapse(listParent, listOffset); > NS_ENSURE_SUCCESS(res, res); > } > // Else just let selection percolate up. We'll adjust it in > // AfterEdit() > } > } else { >- // Adjust selection to be right after it >- res = aSelection->Collapse(blockParent, offset + 1); >- NS_ENSURE_SUCCESS(res, res); >+ if (aAction == nsIEditor::eNext) { >+ // Adjust selection to be right after it. >+ res = aSelection->Collapse(blockParent, offset + 1); >+ NS_ENSURE_SUCCESS(res, res); >+ >+ // Move to the start of the next node if it's a text. >+ nsCOMPtr<nsIContent> nextNode = mHTMLEditor->GetNextNode(blockParent, >+ offset + 1, true); >+ nsCOMPtr<nsIDOMText> textNode = do_QueryInterface(nextNode); >+ if (textNode) { >+ res = aSelection->Collapse(textNode, 0); >+ NS_ENSURE_SUCCESS(res, res); >+ } >+ } else { >+ // Move to the end of the previous node if it's a text. >+ nsCOMPtr<nsIContent> priorNode = mHTMLEditor->GetPriorNode(blockParent, >+ offset, >+ true); >+ nsCOMPtr<nsIDOMText> textNode = do_QueryInterface(priorNode); >+ if (textNode) { >+ nsAutoString tempString; >+ textNode->GetData(tempString); Why don't you use nsIContent::TextLength()? >+ res = aSelection->Collapse(priorNode, tempString.Length()); I think that NS_ENSURE_SUCCESS(res, res) should be inserted here >+ } else { >+ res = aSelection->Collapse(blockParent, offset + 1); and here >+ } >+ NS_ENSURE_SUCCESS(res, res); Instead of here because the line number of the warning message tells us if the failure occurred in text node. >+<div id="backspaceCSS" contenteditable><p style="color:red;">12345</p>67</div> >+<div id="backspace" contenteditable><p><font color="red">12345</font></p>67</div> >+<div id="deleteCSS" contenteditable><p style="color:red;">x</p></div> >+<div id="delete" contenteditable><p><font color="red">y</font></p></div> >+ >+<pre id="test"> >+</pre> >+ >+<script class="testbody" type="application/javascript"> >+ >+/** Test for Bug 1257363 **/ >+SimpleTest.waitForExplicitFinish(); >+SimpleTest.waitForFocus(function() { >+ >+ // ***** Backspace test ***** >+ var div = document.getElementById("backspaceCSS"); >+ div.focus(); >+ synthesizeMouse(div, 100, 2, {}); /* click behind and down */ You specify the pixels directly here. I think that you should specify font-size to the <div> elements. Then, this becomes safer. >+ // Return and backspace should take us to the following line. >+ synthesizeKey("VK_RETURN", {}); >+ synthesizeKey("VK_DELETE", {}); The comment says "Return and backspace", but this is Delete key. >+ // Return and backspace should take us to the following line. >+ synthesizeKey("VK_RETURN", {}); >+ synthesizeKey("VK_DELETE", {}); The comment says "Return and backspace", but this is Delete key. >+ // ***** Delete test ***** >+ div = document.getElementById("deleteCSS"); >+ div.focus(); >+ synthesizeMouse(div, 100, 2, {}); /* click behind and down */ >+ >+ var sel = window.getSelection(); >+ var selRange = sel.getRangeAt(0); >+ is(selRange.endContainer.nodeName, "#text", "selection should be at the end of text node"); >+ is(selRange.endOffset, 1, "offset should be 1"); >+ >+ // left, enter, up-arrow and delete should take us to where we started. >+ synthesizeKey("VK_LEFT", {}); >+ synthesizeKey("VK_RETURN", {}); >+ synthesizeKey("VK_UP", {}); I hope that you test the caret position after each synthesizeKey() because it's easier to investigate for other developers when this test detects a regression. And you tested text node name in a lot of place. But I think that you should test its parent element's tag name too. >+ // left, enter, up-arrow and delete should take us to where we started. >+ synthesizeKey("VK_LEFT", {}); >+ synthesizeKey("VK_RETURN", {}); >+ synthesizeKey("VK_UP", {}); >+ synthesizeKey("VK_DELETE", {}); Same here. Thank you for your work, looks almost good except above nits. I'd like to check the new patch, so, r-'ing for now.

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #19) > I'm very sorry for the long delay. No problem, I was busy with other stuff in the meantime. Thank you so much for the thorough review, much appreciated. > the if condition of the |else| block is checking if the empty block > is a list item. Correct. I won't touch list processing for now. > Why don't you use nsIContent::TextLength()? Didn't know it, using it now. > I think that NS_ENSURE_SUCCESS(res, res) should be inserted here Done. > >+ synthesizeMouse(div, 100, 2, {}); /* click behind and down */ > You specify the pixels directly here. I think that you should specify > font-size to the <div> elements. Then, this becomes safer. This is common practise: https://dxr.mozilla.org/mozilla-central/source/editor/libeditor/tests/test_bug772796.html#165 + two more. https://dxr.mozilla.org/mozilla-central/source/layout/generic/test/test_bug756984.html#42 + one more. > The comment says "Return and backspace", but this is Delete key. Comment fixed. > I hope that you test the caret position after each synthesizeKey() because > it's easier to investigate for other developers when this test detects a > regression. Done. I added more testing after each keystroke.

Comment on attachment 8736236 [details] [diff] [review] Possible solution (v2b) for backspace and delete case, tests for both casesb Thank you very much!

Fixing terrible grammar in summary ;-)

Sadly this crashed under certain circumstances since next and previous aren't checked for null. Patch coming.

Masayuki-san, we created a crash that was noted in TB testing in bug 1261602. Please review asap to fix this.

(In reply to Jorg K (GMT+2) from comment #26) > Created attachment 8737543 [details] [diff] [review] > Add null check to prevent crash. > > Masayuki-san, we created a crash that was noted in TB testing in bug 1261602. > > Please review asap to fix this. Forgot to r? to me?

Comment on attachment 8737543 [details] [diff] [review] Add null check to prevent crash. Sorry.

Comment on attachment 8737543 [details] [diff] [review] Add null check to prevent crash. np ;-)

Dear Sheriff, please land the second patch "Add null check to prevent crash". Thanks.

https://hg.mozilla.org/integration/mozilla-inbound/rev/823040191adc78b25f1c8ec6d0523c209b97598f Bug 1257363 - add null check so it doesn't crash. r=masayuki

> https://hg.mozilla.org/mozilla-central/rev/823040191adc I just added the crash signature for the crashes fixed by this commit, so that crash reports will be linked to this bug.

There's only a very brief interval of the faulty code being live: https://hg.mozilla.org/mozilla-central/rev/36888eb8c404 - broken, landed 1st April. https://hg.mozilla.org/mozilla-central/rev/823040191adc - fixed, landed 3rd April.

Since I happened to stumble over this crasher in a Nightly that happened to contain it, I can happily verify that it's now gone :) Good work Jorg, thanks!

This bug was landed in THUNDERBIRD_45_VERBRANCH (see bug 1260975)

Pushed to a release branch on mozilla-beta for Thunderbird 47.0b1