Bug 1258085 - Avoid empty whitespace nodes when editing
authorAryeh Gregor <ayg@aryeh.name>
Wed, 20 Apr 2016 21:19:20 +0300
changeset 317926 f8cc52e2b8f90ebe8982e927d31a0174c6bfe71e
parent 317925 22d56f931cb6f762f97557927d56c0bd11968cfa
child 317927 7672199e4347692cf5cd6bee43506502769a1d11
push id9480
push userjlund@mozilla.com
push dateMon, 25 Apr 2016 17:12:58 +0000
treeherdermozilla-aurora@0d6a91c76a9e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
bugs1258085
milestone48.0a1
Bug 1258085 - Avoid empty whitespace nodes when editing There are probably a lot more places that could use HandleEmptyText thrown in, but this covers a few simple ones.
editor/libeditor/nsHTMLEditRules.cpp
editor/libeditor/nsHTMLEditRules.h
editor/libeditor/tests/mochitest.ini
editor/libeditor/tests/test_bug1258085.html
editor/libeditor/tests/test_bug772796.html
--- a/editor/libeditor/nsHTMLEditRules.cpp
+++ b/editor/libeditor/nsHTMLEditRules.cpp
@@ -2025,16 +2025,19 @@ nsHTMLEditRules::WillDeleteSelection(Sel
       res = nsWSRunObject::PrepareToDeleteRange(mHTMLEditor,
           address_of(visNode), &so, address_of(visNode), &eo);
       NS_ENSURE_SUCCESS(res, res);
       NS_ENSURE_STATE(mHTMLEditor);
       res = mHTMLEditor->DeleteText(nodeAsText, std::min(so, eo),
                                     DeprecatedAbs(eo - so));
       *aHandled = true;
       NS_ENSURE_SUCCESS(res, res);
+
+      DeleteNodeIfCollapsedText(nodeAsText);
+
       res = InsertBRIfNeeded(aSelection);
       NS_ENSURE_SUCCESS(res, res);
 
       // Remember that we did a ranged delete for the benefit of
       // AfterEditInner().
       mDidRangedDelete = true;
 
       return NS_OK;
@@ -2466,16 +2469,28 @@ nsHTMLEditRules::WillDeleteSelection(Sel
         if (join) {
           res = JoinBlocks(GetAsDOMNode(leftParent), GetAsDOMNode(rightParent),
                            aCancel);
           NS_ENSURE_SUCCESS(res, res);
         }
       }
     }
   }
+
+  // We might have left only collapsed whitespace in the start/end nodes
+  {
+    nsAutoTrackDOMPoint startTracker(mHTMLEditor->mRangeUpdater,
+                                     address_of(startNode), &startOffset);
+    nsAutoTrackDOMPoint endTracker(mHTMLEditor->mRangeUpdater,
+                                   address_of(endNode), &endOffset);
+
+    DeleteNodeIfCollapsedText(*startNode);
+    DeleteNodeIfCollapsedText(*endNode);
+  }
+
   // If we're joining blocks: if deleting forward the selection should be
   // collapsed to the end of the selection, if deleting backward the selection
   // should be collapsed to the beginning of the selection. But if we're not
   // joining then the selection should collapse to the beginning of the
   // selection if we'redeleting forward, because the end of the selection will
   // still be in the next block. And same thing for deleting backwards
   // (selection should collapse to the end, because the beginning will still be
   // in the first block). See Bug 507936
@@ -2483,16 +2498,38 @@ nsHTMLEditRules::WillDeleteSelection(Sel
     res = aSelection->Collapse(endNode, endOffset);
   } else {
     res = aSelection->Collapse(startNode, startOffset);
   }
   NS_ENSURE_SUCCESS(res, res);
   return NS_OK;
 }
 
+/**
+ * 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::DeleteNodeIfCollapsedText(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);
+  }
+}
+
 
 /*****************************************************************************************************
 *    InsertBRIfNeeded: determines if a br is needed for current selection to not be spastic.
 *    If so, it inserts one.  Callers responsibility to only call with collapsed selection.
 *         Selection* aSelection      the collapsed selection
 */
 nsresult
 nsHTMLEditRules::InsertBRIfNeeded(Selection* aSelection)
--- a/editor/libeditor/nsHTMLEditRules.h
+++ b/editor/libeditor/nsHTMLEditRules.h
@@ -101,16 +101,17 @@ public:
   NS_IMETHOD WillJoinNodes(nsIDOMNode *aLeftNode, nsIDOMNode *aRightNode, nsIDOMNode *aParent) override;
   NS_IMETHOD DidJoinNodes(nsIDOMNode  *aLeftNode, nsIDOMNode *aRightNode, nsIDOMNode *aParent, nsresult aResult) override;
   NS_IMETHOD WillInsertText(nsIDOMCharacterData *aTextNode, int32_t aOffset, const nsAString &aString) override;
   NS_IMETHOD DidInsertText(nsIDOMCharacterData *aTextNode, int32_t aOffset, const nsAString &aString, nsresult aResult) override;
   NS_IMETHOD WillDeleteText(nsIDOMCharacterData *aTextNode, int32_t aOffset, int32_t aLength) override;
   NS_IMETHOD DidDeleteText(nsIDOMCharacterData *aTextNode, int32_t aOffset, int32_t aLength, nsresult aResult) override;
   NS_IMETHOD WillDeleteSelection(nsISelection *aSelection) override;
   NS_IMETHOD DidDeleteSelection(nsISelection *aSelection) override;
+  void DeleteNodeIfCollapsedText(nsINode& aNode);
 
 protected:
   virtual ~nsHTMLEditRules();
 
   enum RulesEndpoint
   {
     kStart,
     kEnd
--- a/editor/libeditor/tests/mochitest.ini
+++ b/editor/libeditor/tests/mochitest.ini
@@ -163,8 +163,9 @@ skip-if = toolkit == 'android'
 [test_bug1186799.html]
 [test_bug1181130-1.html]
 [test_bug1181130-2.html]
 [test_backspace_vs.html]
 [test_css_chrome_load_access.html]
 skip-if = toolkit == 'android' # chrome urls not available due to packaging
 [test_bug1247483.html]
 skip-if = toolkit == 'android'
+[test_bug1258085.html]
new file mode 100644
--- /dev/null
+++ b/editor/libeditor/tests/test_bug1258085.html
@@ -0,0 +1,66 @@
+<!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");
+
+  // Make sure we're not deleting if the whitespace isn't actually collapsed
+  div.style.whiteSpace = "pre-wrap";
+  reset();
+  synthesizeKey("VK_RIGHT", {});
+  synthesizeKey("VK_RIGHT", {});
+  synthesizeKey("VK_BACK_SPACE", {});
+  if (div.innerHTML, "x<br> ", "pre-wrap: Don't delete uncollapsed space");
+  ok(getSelection().isCollapsed, "pre-wrap: Selection must be collapsed");
+  is(getSelection().focusNode, div.lastChild,
+     "pre-wrap: Focus must be in final text node");
+  is(getSelection().focusOffset, 1, "pre-wrap: Focus must be at end of node");
+
+  SimpleTest.finish();
+});
+</script>
--- a/editor/libeditor/tests/test_bug772796.html
+++ b/editor/libeditor/tests/test_bug772796.html
@@ -133,17 +133,17 @@ https://bugzilla.mozilla.org/show_bug.cg
 /*66*/[ "<ul><pre><li>test</li><b>foobar\nbaz</b></pre></ul>",   "<ul><pre><li>test<b>foobar\n</b></li><b>baz</b></pre></ul>" ],
 /*67*/[ "<ul><pre><li>test</li><b>foo</b>bar\nbaz</pre></ul>",   "<ul><pre><li>test<b>foo</b>bar\n</li>baz</pre></ul>" ],
 /*68*/[ "<ul><pre><li>test</li><b>foo</b>\nbar</pre></ul>",      "<ul><pre><li>test<b>foo</b>\n</li>bar</pre></ul>" ],
 /*69*/[ "<ul><pre><li>test</li><b>foo\n</b>bar\nbaz</pre></ul>", "<ul><pre><li>test<b>foo\n</b></li>bar\nbaz</pre></ul>" ],
 
       /* Last not least, some simple edge case tests. */
 /*70*/[ "<div>test</div><pre>foobar\n</pre>baz", "<div>testfoobar\n</div>baz" ],
 /*71*/[ "<div>test</div><pre>\nfoo\nbar</pre>", "<div>testfoo\n</div><pre>bar</pre>" ],
-/*72*/[ "<div>test</div><pre>\n\nfoo\nbar</pre>", "<div>test\n</div><pre>foo\nbar</pre>" ],
+/*72*/[ "<div>test</div><pre>\n\nfoo\nbar</pre>", "<div>test</div><pre>foo\nbar</pre>", "<div>test\n</div><pre>foo\nbar</pre>" ],
     ];
 
     /** Test for Bug 772796 **/
 
     SimpleTest.waitForExplicitFinish();
 
     SimpleTest.waitForFocus(function() {