Pressing backspace after paragraph loses caret and also mispositions caret to beginning of preceding paragraph. See enclosed example. Note: Works OK in Chrome.

I'm moving this to Core::Layout since it doesn't see to have anything to do with the editor. The editor is fine, but the caret is misplaced. Simpler test case and debug patch coming.

Click, backspace and bad caret position.

As far as I can see, the editor has worked OK, but the Collapse() shows the caret in the wrong location. Note that it all depends on the \n after the <br>: Not working: ==== <p style="color:red;">Position behind the x on the next line, press backspace<br> x</p> ==== Working: <p style="color:red;">Position behind the x on the next line, press backspace<br>x</p> ====

Gentlemen, this bug doesn't seem to be in the unowned editor, neither the unowned selection. It simply seems to be a layout problem. So perhaps I can get someone interested. I'm happy to help, but I have no idea were I would start looking. The Collapse() call from the editor is clearly: Position at offset 1 in a text node containing a newline. That doesn't happen. Instead it draws the caret somewhere else sort of at half height. You can already see that it's confused. NI from David since he's the owner of layout and also NI from Mats who's been working right in Collapse() very recently.

Or maybe Ehsan is also appropriate since he's recently reviewed work in Collapse(). Sorry about the NI Spam, hard to tell who looks after this area.

The caret code lives in layout/base/nsCaret.cpp, with some "caret hints" managed by by the the selection code in layout/generic/nsFrameSelection.h and layout/generic/nsSelection.cpp. It looks like you've found the relevant Editor code already. I'm not convinced this isn't an Editor bug (yet). If it's not Editor then it's likely Selection (and the bug is in one of the files mentioned above). What was the output from your debug code?

As I said in comment #4. It dumps out a text node that contains a \n and the offset is 1. Do you need to see it or do you believe me?

OK, seeing is believing, here you go: Text@13D67830 flags=[07000208] ranges:1 primaryframe=13D6A9F8 refcount=28<\u000a> ===== Offset 1 Right, Selection::Collapse() is in layout/generic/nsSelection.cpp, but that's part of Layout, no? Or is that the unwanted unowned orphan?

Oh, I missed that in comment 4, sorry. Can you check if the Selection has a range and what its nodes/offsets are after the text node is removed and where it's removed? > layout/generic/nsSelection.cpp, but that's part of Layout, no? No, that file belongs to Core/Selection. It really should be moved under dom/base/ where nsRange lives (it's an historical artifact that it was created under layout/)

I'm not sure I understand the question. When you click after the x, you're most likely in the text node with a collapsed selection, or maybe behind the paragraph, but you get moved into the text so you have something to delete. The text deleting happens here: https://dxr.mozilla.org/mozilla-central/source/editor/libeditor/DeleteTextTxn.cpp#59 Right after, the program sets a new collapsed selection and that ain't working. What else should I check? (Let's take Ehsan out of the loop.)

OK, the "x" was already removed a few lines earlier. So yeah, the remaining text node (after the <br>) is now "\n" and collapsing at offset 1 seems correct to me. If you dump the frame tree before and after deleting the "x" (give -layoutdebug on the command line when you start Firefox) you can see that the text frame goes from: Text(2)"\nx"@7f6b4ca25c70 {0,1140,540,1140} ... to Text(2)"\n"@7f6b4ca25c70 {0,1140,0,0} ... (those numbers are x,y,width,height) so the caret is positioned correctly it's just that the frame has zero height so the baseline is wrong and that's why it's got the wrong y-offset. I think Ehsan has been dabbling with this problem in the past...

(if you put this into Core::Editor, no one will look at it ;-()

Bug 389321 is the one I was thinking about that have some background info on caret positioning that might help. So the patches in that bug, and comment 6 / comment 11 in this bug should be enough to debug this and fix it I think.

Well, Ehsan, Robert and David worked on bug 389321. So who should be looking into this? I'm already looking into editor problems (currently bug 1257363), so it would be great if someone else could fix the layout problems. Layout is not unowned, unlike editor and selection.

I have seen issues related to text frames without anything other than collapsible whitespace having the wrong baseline in the past, but I couldn't find the bug I was thinking about with a quick search. Bug 904846 may be related. I don't remember much more concrete details, but Mats' analysis in comment 11 seems to be on the right path. Someone needs to debug this further.

Jet, can you help find a resource for this.

FWIW we've had the underlying bug for years now, this is not a new issue.

This testcase shows the state of layout following the character deletion. I've added a yellow background to the p for clarity. This testcase appears to be displayed interoperably across browsers; the <br> at the end of the p element does not add anything to its height.

I don't believe this is a layout bug as such. The layout of the document is what's required for Web compatibility. Unfortunately, layout on the Web wasn't really designed with editing in mind. There are many cases where you can place a caret at a position that currently doesn't take up space, but if you start typing, then suddenly things will start taking up space. The solutions to these situations generally involve making the editor do somewhat non-obvious things that don't match the user's editing inputs. (You might suggest doing editing-dependent layout instead, such as having layout produce different results when the caret is present at a location. However, I don't know of a way to do this without yielding user interface that jumps around in horrible and incomprehensible ways.) In this case, the editor allows the creation of one of those situations through character deletion. Chrome's editor, on the other hand, inserts a *second* <br> in this situation (when the character is deleted) to avoid entering such a layout situation. (You can see that if you have Chrome's inspector open while following the steps to reproduce.) So if you want things to work like Chrome, that's an Editor bug. That also seems like the most reasonable path forward.

One final note: making any change here is somewhat risky in terms of Web-compat, since there are a number of JS editing libraries that work around various browser bugs, and which may be assuming that Gecko behaves differently from Chromium in this case. It is worth investigating whether that's the case if we're going to try changing the editing behavior, and if it is, trying to get those libraries updated as needed.

The problem is very simple: when you delete the "x", the <br> becomes collapsed. The deletion code needs to check for this and add an additional <br> to prevent the original one from collapsing. The editing spec does this in a lot of cases already, and so do we, e.g.: <div contenteditable>x<br>yz</div> If you backspace the "yz", we insert a <br>. Actually, if you type text after the "z", we also insert a <br>. But if you backspace the "z", we don't insert a <br>. The lazy fix would be to insert a <br> here even if you only backspace the "z". This would leave useless <br>s lying around, but we already do that anyway. The cleaner approach would be to check if the preceding <br> is collapsed and only insert an extra <br> if we collapsed a previously non-collapsed <br>, and conversely to remove a <br> that previously was needed to stop another <br> from collapsing and now is not needed. This is what Chrome does, and what the editing spec tries to do, but it's much more complicated and error-prone, so probably best to be lazy.

Try run from previous patch version with one mochitest failure: https://treeherder.mozilla.org/#/jobs?repo=try&revision=989715b32764 New mochitest-only try run with updated patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf5f7f92d8b6 This is based on the whole gigantic series of bug 1193762 and its dependencies, which hopefully should be ready for checkin soon!

New try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7d521cb1b978

Comment on attachment 8742444 [details] [diff] [review] Simple patch v2, with test fix >+ { >+ nsAutoTrackDOMPoint startTracker(mHTMLEditor->mRangeUpdater, >+ address_of(startNode), &startOffset); >+ nsAutoTrackDOMPoint endTracker(mHTMLEditor->mRangeUpdater, >+ address_of(endNode), &endOffset); >+ >+ HandleEmptyText(*startNode); >+ HandleEmptyText(*endNode); >+ } Well, I want some comments at start of this block for explaining why we need to do this. >+/** >+ * If aNode is a text node that contains only collapsed whitespace, delete it. >+ * It doesn't serve any useful purpose, and we don't want it to confuse code >+ * that doesn't correctly skip over it. >+ * >+ * If deleting the node fails (like if it's not editable), the caller should >+ * proceed as usual, so don't return any errors. >+ */ >+void >+nsHTMLEditRules::HandleEmptyText(nsINode& aNode) >+{ >+ if (!aNode.GetAsText()) { >+ return; >+ } >+ bool empty; >+ nsresult res = mHTMLEditor->IsVisTextNode(aNode.AsContent(), &empty, false); >+ NS_ENSURE_SUCCESS_VOID(res); >+ if (empty) { >+ mHTMLEditor->DeleteNode(&aNode); >+ } >+} How about |DeleteNodeIfEmptyTextNode()|? >diff --git a/editor/libeditor/tests/test_bug1258085.html b/editor/libeditor/tests/test_bug1258085.html >new file mode 100644 >index 0000000..4884d95 >--- /dev/null >+++ b/editor/libeditor/tests/test_bug1258085.html >@@ -0,0 +1,54 @@ >+<!DOCTYPE html> >+<title>Test for Bug 1186799</title> >+<script src="/tests/SimpleTest/SimpleTest.js"></script> >+<script src="/tests/SimpleTest/EventUtils.js"></script> >+<link rel="stylesheet" href="/tests/SimpleTest/test.css"> >+<div contenteditable></div> >+<script> >+var div = document.querySelector("div"); >+ >+function reset() { >+ div.innerHTML = "x<br> y"; >+ div.focus(); >+ synthesizeKey("VK_DOWN", {}); >+} >+ >+function checks(msg) { >+ is(div.innerHTML, "x<br><br>", >+ msg + ": Should add a second <br> to prevent collapse of first"); >+ is(div.childNodes.length, 3, msg + ": No empty text nodes allowed"); >+ ok(getSelection().isCollapsed, msg + ": Selection must be collapsed"); >+ is(getSelection().focusNode, div, msg + ": Focus must be in div"); >+ is(getSelection().focusOffset, 2, >+ msg + ": Focus must be between the two <br>s"); >+} >+ >+SimpleTest.waitForExplicitFinish(); >+SimpleTest.waitForFocus(function() { >+ // Put selection after the "y" and backspace >+ reset(); >+ synthesizeKey("VK_RIGHT", {}); >+ synthesizeKey("VK_BACK_SPACE", {}); >+ checks("Collapsed backspace"); >+ >+ // Now do the same with delete >+ reset(); >+ synthesizeKey("VK_DELETE", {}); >+ checks("Collapsed delete"); >+ >+ // Forward selection >+ reset(); >+ synthesizeKey("VK_RIGHT", {shiftKey: true}); >+ synthesizeKey("VK_BACK_SPACE", {}); >+ checks("Forward-selected backspace"); >+ >+ // Backward selection >+ reset(); >+ synthesizeKey("VK_RIGHT", {}); >+ synthesizeKey("VK_LEFT", {shiftKey: true}); >+ synthesizeKey("VK_BACK_SPACE", {}); >+ checks("Backward-selected backspace"); >+ >+ SimpleTest.finish(); >+}); >+</script> I wonder, how does this work with "white-space: pre-wrap;"? If whitespace won't be collapsed, the text node shouldn't be removed in this case, isn't it? If so, I hope you add the testcases for that.

I didn't think it was necessary to do a new try run, since there were no behavior changes. (In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #24) > >+ { > >+ nsAutoTrackDOMPoint startTracker(mHTMLEditor->mRangeUpdater, > >+ address_of(startNode), &startOffset); > >+ nsAutoTrackDOMPoint endTracker(mHTMLEditor->mRangeUpdater, > >+ address_of(endNode), &endOffset); > >+ > >+ HandleEmptyText(*startNode); > >+ HandleEmptyText(*endNode); > >+ } > > Well, I want some comments at start of this block for explaining why we need > to do this. I added a comment, but I'm not sure why it's not obvious, so let me know if you want a different comment. It's needed if the start/end nodes of the deletion lie in text nodes, and after the deletion only collapsed whitespace is left. > How about |DeleteNodeIfEmptyTextNode()|? Good idea. I went with "Collapsed" instead of "Empty" because "Empty" sounds like zero-length to me. > I wonder, how does this work with "white-space: pre-wrap;"? If whitespace > won't be collapsed, the text node shouldn't be removed in this case, isn't > it? If so, I hope you add the testcases for that. Good catch! Fortunately it does work.

(In reply to :Aryeh Gregor (working until May 8) from comment #25) > Created attachment 8742751 [details] [diff] [review] > Patch v3 > > I didn't think it was necessary to do a new try run, since there were no > behavior changes. > > (In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #24) > > >+ { > > >+ nsAutoTrackDOMPoint startTracker(mHTMLEditor->mRangeUpdater, > > >+ address_of(startNode), &startOffset); > > >+ nsAutoTrackDOMPoint endTracker(mHTMLEditor->mRangeUpdater, > > >+ address_of(endNode), &endOffset); > > >+ > > >+ HandleEmptyText(*startNode); > > >+ HandleEmptyText(*endNode); > > >+ } > > > > Well, I want some comments at start of this block for explaining why we need > > to do this. > > I added a comment, but I'm not sure why it's not obvious, so let me know if > you want a different comment. It's needed if the start/end nodes of the > deletion lie in text nodes, and after the deletion only collapsed whitespace > is left. It was unclear the code looks like that might removing the selection start and/or end node but how selection range is managed and the new range.

This bug was landed in THUNDERBIRD_45_VERBRANCH https://hg.mozilla.org/releases/mozilla-esr45/rev/70c9bb7161ca

I'm not sure why/how I became the author of this patch. This is Aryeh Gregor's work. I just adapted the patch for ESR 45.

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