Bug 1315065 When selection is collapsed in an empty text node, Backspace/Delete key press should modify the nearest text node r=smaug
authorMasayuki Nakano <masayuki@d-toybox.com>
Fri, 11 Nov 2016 12:24:21 +0900
changeset 352069 9e166b2418ca1dbb82c0c6a1bdf9fdcf3de78f6b
parent 352067 4769fe1cc0041723b7fef9da422719cf3125a4c7
child 352070 cac0f33c99191fc7e82018c1fad1e7c11735d377
push id6795
push userjlund@mozilla.com
push dateMon, 23 Jan 2017 14:19:46 +0000
treeherdermozilla-esr52@76101b503191 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssmaug
bugs1315065
milestone52.0a1
Bug 1315065 When selection is collapsed in an empty text node, Backspace/Delete key press should modify the nearest text node r=smaug Currently, when selection is collapsed at an empty text node, the behavior of each major browser is different. When you remove the last character of non-empty text node followed by empty text nodes, Chromium removes all following empty text nodes. However, Edge never removes empty text nodes even when selection is collapsed at an empty text node. With this patch, our behavior becomes same as Edge. I think that we should take this for keeping backward compatibility since Gecko never removes empty text nodes. So, in other words, this patch makes Backspace key press at an empty text node modify the preceding non-empty text node. When you remove the first character of non-empty text node preceded with empty text nodes, Edge removes all preceding empty text nodes. However, Chromium and Gecko keeps previous empty text nodes than caret position. So, we should keep current behavior for backward compatibility. In other words, this patch makes Delete key press at an empty text node modify the following non-empty text node and keep current behavior. The fixing approach of this is, making WSRunObject::PriorVisibleNode() and WSRunObject::NextVisibleNode() ignore empty text node. This should make sense because empty text node is not a visible node. (On the other hand, when the DOMPoint has a null character, it should treat as visible character. That is visible with Unicode codepoint.) MozReview-Commit-ID: 11YtqBktEvK
editor/libeditor/HTMLEditRules.cpp
editor/libeditor/WSRunObject.cpp
editor/libeditor/tests/mochitest.ini
editor/libeditor/tests/test_bug1315065.html
--- a/editor/libeditor/HTMLEditRules.cpp
+++ b/editor/libeditor/HTMLEditRules.cpp
@@ -1951,16 +1951,29 @@ HTMLEditRules::WillDeleteSelection(Selec
                                              &so, address_of(visNode), &eo);
       NS_ENSURE_SUCCESS(rv, rv);
       NS_ENSURE_STATE(mHTMLEditor);
       *aHandled = true;
       rv = mHTMLEditor->DeleteText(nodeAsText, std::min(so, eo),
                                    DeprecatedAbs(eo - so));
       NS_ENSURE_SUCCESS(rv, rv);
 
+      // XXX When Backspace key is pressed, Chromium removes following empty
+      //     text nodes when removing the last character of the non-empty text
+      //     node.  However, Edge never removes empty text nodes even if
+      //     selection is in the following empty text node(s).  For now, we
+      //     should keep our traditional behavior same as Edge for backward
+      //     compatibility.
+      // XXX When Delete key is pressed, Edge removes all preceding empty
+      //     text nodes when removing the first character of the non-empty
+      //     text node.  Chromium removes only selected empty text node and
+      //     following empty text nodes and the first character of the
+      //     non-empty text node.  For now, we should keep our traditional
+      //     behavior same as Chromium for backward compatibility.
+
       DeleteNodeIfCollapsedText(nodeAsText);
 
       rv = InsertBRIfNeeded(aSelection);
       NS_ENSURE_SUCCESS(rv, rv);
 
       // Remember that we did a ranged delete for the benefit of
       // AfterEditInner().
       mDidRangedDelete = true;
--- a/editor/libeditor/WSRunObject.cpp
+++ b/editor/libeditor/WSRunObject.cpp
@@ -478,24 +478,22 @@ WSRunObject::PriorVisibleNode(nsINode* a
 
   WSFragment* run;
   FindRun(aNode, aOffset, &run, false);
 
   // Is there a visible run there or earlier?
   for (; run; run = run->mLeft) {
     if (run->mType == WSType::normalWS) {
       WSPoint point = GetCharBefore(aNode, aOffset);
-      if (point.mTextNode) {
+      // When it's a non-empty text node, return it.
+      if (point.mTextNode && point.mTextNode->Length()) {
         *outVisNode = point.mTextNode;
         *outVisOffset = point.mOffset + 1;
         if (nsCRT::IsAsciiSpace(point.mChar) || point.mChar == nbsp) {
           *outType = WSType::normalWS;
-        } else if (!point.mChar) {
-          // MOOSE: not possible?
-          *outType = WSType::none;
         } else {
           *outType = WSType::text;
         }
         return;
       }
       // If no text node, keep looking.  We should eventually fall out of loop
     }
   }
@@ -522,24 +520,22 @@ WSRunObject::NextVisibleNode(nsINode* aN
 
   WSFragment* run;
   FindRun(aNode, aOffset, &run, true);
 
   // Is there a visible run there or later?
   for (; run; run = run->mRight) {
     if (run->mType == WSType::normalWS) {
       WSPoint point = GetCharAfter(aNode, aOffset);
-      if (point.mTextNode) {
+      // When it's a non-empty text node, return it.
+      if (point.mTextNode && point.mTextNode->Length()) {
         *outVisNode = point.mTextNode;
         *outVisOffset = point.mOffset;
         if (nsCRT::IsAsciiSpace(point.mChar) || point.mChar == nbsp) {
           *outType = WSType::normalWS;
-        } else if (!point.mChar) {
-          // MOOSE: not possible?
-          *outType = WSType::none;
         } else {
           *outType = WSType::text;
         }
         return;
       }
       // If no text node, keep looking.  We should eventually fall out of loop
     }
   }
--- a/editor/libeditor/tests/mochitest.ini
+++ b/editor/libeditor/tests/mochitest.ini
@@ -205,16 +205,17 @@ skip-if = os == 'android'
 subsuite = clipboard
 skip-if = toolkit == 'android'
 [test_bug1248128.html]
 [test_bug1250010.html]
 [test_bug1257363.html]
 [test_bug1248185.html]
 [test_bug1258085.html]
 [test_bug1268736.html]
+[test_bug1315065.html]
 
 [test_CF_HTML_clipboard.html]
 subsuite = clipboard
 [test_composition_event_created_in_chrome.html]
 [test_contenteditable_focus.html]
 [test_dom_input_event_on_htmleditor.html]
 skip-if = toolkit == 'android' # bug 1054087
 [test_dom_input_event_on_texteditor.html]
new file mode 100644
--- /dev/null
+++ b/editor/libeditor/tests/test_bug1315065.html
@@ -0,0 +1,145 @@
+<!DOCTYPE HTML>
+<html>
+<!--
+https://bugzilla.mozilla.org/show_bug.cgi?id=1315065
+-->
+<head>
+  <meta charset="utf-8">
+  <title>Test for Bug 1315065</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"/>
+</head>
+<body>
+<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=1315065">Mozilla Bug 1315065</a>
+<div contenteditable><p>abc<br></p></div>
+<script type="application/javascript">
+/** Test for Bug 1315065 **/
+SimpleTest.waitForExplicitFinish();
+SimpleTest.waitForFocus(()=>{
+  var editor = document.getElementsByTagName("div")[0];
+  function initForBackspace(aSelectionCollapsedTo /* = 0 ~ 3 */) {
+    editor.innerHTML = "<p id='p'>abc<br></p>";
+    var p = document.getElementById("p");
+    // FYI: We cannot inserting empty text nodes as expected with
+    //      Node.appendChild() nor Node.insertBefore(). Therefore, let's use
+    //      Range.insertNode() like actual web apps.
+    var selection = window.getSelection();
+    selection.collapse(p, 1);
+    var range = selection.getRangeAt(0);
+    var emptyTextNode3 = document.createTextNode("");
+    range.insertNode(emptyTextNode3);
+    var emptyTextNode2 = document.createTextNode("");
+    range.insertNode(emptyTextNode2);
+    var emptyTextNode1 = document.createTextNode("");
+    range.insertNode(emptyTextNode1);
+    is(p.childNodes.length, 5, "Failed to initialize the editor");
+    is(p.childNodes.item(1), emptyTextNode1, "1st text node should be emptyTextNode1");
+    is(p.childNodes.item(2), emptyTextNode2, "2nd text node should be emptyTextNode2");
+    is(p.childNodes.item(3), emptyTextNode3, "3rd text node should be emptyTextNode3");
+    switch (aSelectionCollapsedTo) {
+      case 0:
+        selection.collapse(p.firstChild, 3); // next to 'c'
+        break;
+      case 1:
+        selection.collapse(emptyTextNode1, 0);
+        break;
+      case 2:
+        selection.collapse(emptyTextNode2, 0);
+        break;
+      case 3:
+        selection.collapse(emptyTextNode3, 0);
+        break;
+      default:
+        ok(false, "aSelectionCollapsedTo is illegal value");
+    }
+  }
+
+  for (var i = 0; i < 4; i++) {
+    const kDescription = i == 0 ? "Backspace from immediately after the last character" :
+                                  "Backspace from " + i + "th empty text node";
+    editor.focus();
+    initForBackspace(i);
+    synthesizeKey("KEY_Backspace", { code: "Backspace" });
+    var p = document.getElementById("p");
+    ok(p, kDescription + ": <p> element shouldn't be removed by Backspace key press");
+    is(p.tagName.toLowerCase(), "p", kDescription + ": <p> element shouldn't be removed by Backspace key press");
+    // When Backspace key is pressed even in empty text nodes, Gecko should not remove empty text nodes for now
+    // because we should keep our traditional behavior (same as Edge) for backward compatibility as far as possible.
+    // In this case, Chromium removes all empty text nodes, but Edge doesn't remove any empty text nodes.
+    is(p.childNodes.length, 5, kDescription + ": <p> should have 5 children after pressing Backspace key");
+    is(p.childNodes.item(0).textContent, "ab", kDescription + ": 'c' should be removed by pressing Backspace key");
+    is(p.childNodes.item(1).textContent, "", kDescription + ": 1st empty text node should not be removed by pressing Backspace key");
+    is(p.childNodes.item(2).textContent, "", kDescription + ": 2nd empty text node should not be removed by pressing Backspace key");
+    is(p.childNodes.item(3).textContent, "", kDescription + ": 3rd empty text node should not be removed by pressing Backspace key");
+    editor.blur();
+  }
+
+  function initForDelete(aSelectionCollapsedTo /* = 0 ~ 3 */) {
+    editor.innerHTML = "<p id='p'>abc<br></p>";
+    var p = document.getElementById("p");
+    // FYI: We cannot inserting empty text nodes as expected with
+    //      Node.appendChild() nor Node.insertBefore(). Therefore, let's use
+    //      Range.insertNode() like actual web apps.
+    var selection = window.getSelection();
+    selection.collapse(p, 0);
+    var range = selection.getRangeAt(0);
+    var emptyTextNode1 = document.createTextNode("");
+    range.insertNode(emptyTextNode1);
+    var emptyTextNode2 = document.createTextNode("");
+    range.insertNode(emptyTextNode2);
+    var emptyTextNode3 = document.createTextNode("");
+    range.insertNode(emptyTextNode3);
+    is(p.childNodes.length, 5, "Failed to initialize the editor");
+    is(p.childNodes.item(0), emptyTextNode3, "1st text node should be emptyTextNode3");
+    is(p.childNodes.item(1), emptyTextNode2, "2nd text node should be emptyTextNode2");
+    is(p.childNodes.item(2), emptyTextNode1, "3rd text node should be emptyTextNode1");
+    switch (aSelectionCollapsedTo) {
+      case 0:
+        selection.collapse(p.childNodes.item(3), 0); // next to 'a'
+        break;
+      case 1:
+        selection.collapse(emptyTextNode1, 0);
+        break;
+      case 2:
+        selection.collapse(emptyTextNode2, 0);
+        break;
+      case 3:
+        selection.collapse(emptyTextNode3, 0);
+        break;
+      default:
+        ok(false, "aSelectionCollapsedTo is illegal value");
+    }
+  }
+
+  for (var i = 0; i < 4; i++) {
+    const kDescription = i == 0 ? "Delete from immediately before the first character" :
+                                  "Delete from " + i + "th empty text node";
+    editor.focus();
+    initForDelete(i);
+    synthesizeKey("KEY_Delete", { code: "Delete" });
+    var p = document.getElementById("p");
+    ok(p, kDescription + ": <p> element shouldn't be removed by Delete key press");
+    is(p.tagName.toLowerCase(), "p", kDescription + ": <p> element shouldn't be removed by Delete key press");
+    if (i == 0) {
+      // If Delete key is pressed in non-empty text node, only the text node should be modified.
+      // This is same behavior as Chromium, but different from Edge.  Edge removes all empty text nodes in this case.
+      is(p.childNodes.length, 5, kDescription + ": <p> should have only 2 children after pressing Delete key (empty text nodes should be removed");
+      is(p.childNodes.item(0).textContent, "", kDescription + ": 1st empty text node should not be removed by pressing Delete key");
+      is(p.childNodes.item(1).textContent, "", kDescription + ": 2nd empty text node should not be removed by pressing Delete key");
+      is(p.childNodes.item(2).textContent, "", kDescription + ": 3rd empty text node should not be removed by pressing Delete key");
+      is(p.childNodes.item(3).textContent, "bc", kDescription + ": 'a' should be removed by pressing Delete key");
+    } else {
+      // If Delete key is pressed in an empty text node, it and following empty text nodes should be removed and the non-empty text node should be modified.
+      // This is same behavior as Chromium, but different from Edge.  Edge removes all empty text nodes in this case.
+      var expectedEmptyTextNodes = 3 - i;
+      is(p.childNodes.length, expectedEmptyTextNodes + 2, kDescription + ": <p> should have only " + i + " children after pressing Delete key (" + i + " empty text nodes should be removed");
+      is(p.childNodes.item(expectedEmptyTextNodes).textContent, "bc", kDescription + ": empty text nodes and 'a' should be removed by pressing Delete key");
+    }
+    editor.blur();
+  }
+  SimpleTest.finish();
+});
+</script>
+</body>
+</html>