Bug 1524266 - Should be able to delete non-selectable and non-editable content in a contenteditable subtree. r=mats
authorEmilio Cobos Álvarez <emilio@crisal.io>
Fri, 01 Feb 2019 18:13:28 +0100
changeset 514488 53951b4b6ac20a1e7543a37d867bae2641e7926f
parent 514487 13a4db7bd6f5091d8c4bb4465ddeadb08e27ddd5
child 514489 d0b5d0922998143dcdf3bdbb7dedd2125625c197
push id10862
push userffxbld-merge
push dateMon, 11 Mar 2019 13:01:11 +0000
treeherdermozilla-beta@a2e7f5c935da [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmats
bugs1524266, 989012
milestone67.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1524266 - Should be able to delete non-selectable and non-editable content in a contenteditable subtree. r=mats This makes our behavior a bit closer to Blink / WebKit. This patch fixes multiple issues: First, fixes the caret movement getting stuck on a <select> element inside an editor. This is because of the IsRootOfAnonymousSubtree() check that I'm removing. Instead of that, consider NAC unselectable in UsedUserSelect, just like generated content. This makes us jump across it correctly, and doesn't regress the test-case that was added in bug 989012. Second, it allows to select nodes with user-select: none as long as you're on an editor. This matches WebKit and Blink. It's something you could do earlier regardless with user-select: all on the parent, which is why the reporter's test-case worked before my patch. I think being able to jump across these and delete them on an editor is the right thing to do. It adds tests for all this plus the same thing working for non-editable contents (there was no pre-existing test for that). Differential Revision: https://phabricator.services.mozilla.com/D18494
dom/base/Selection.cpp
dom/base/test/test_user_select.html
layout/base/tests/bug1524266-1-ref.html
layout/base/tests/bug1524266-1.html
layout/base/tests/bug1524266-2-ref.html
layout/base/tests/bug1524266-2.html
layout/base/tests/bug1524266-3.html
layout/base/tests/bug1524266-4.html
layout/base/tests/mochitest.ini
layout/base/tests/test_reftests_with_caret.html
layout/generic/nsFrame.cpp
--- a/dom/base/Selection.cpp
+++ b/dom/base/Selection.cpp
@@ -454,30 +454,33 @@ void Selection::SetInterlinePosition(boo
 bool Selection::GetInterlinePosition(ErrorResult& aRv) {
   if (!mFrameSelection) {
     aRv.Throw(NS_ERROR_NOT_INITIALIZED);  // Can't do selection
     return false;
   }
   return mFrameSelection->GetHint() == CARET_ASSOCIATE_AFTER;
 }
 
-bool Selection::IsEditorSelection() const {
-  nsINode* focusNode = GetFocusNode();
-  if (!focusNode) {
+static bool IsEditorNode(const nsINode* aNode) {
+  if (!aNode) {
     return false;
   }
 
-  if (focusNode->IsEditable()) {
+  if (aNode->IsEditable()) {
     return true;
   }
 
-  auto* element = Element::FromNode(focusNode);
+  auto* element = Element::FromNode(aNode);
   return element && element->State().HasState(NS_EVENT_STATE_MOZ_READWRITE);
 }
 
+bool Selection::IsEditorSelection() const {
+  return IsEditorNode(GetFocusNode());
+}
+
 Nullable<int16_t> Selection::GetCaretBidiLevel(
     mozilla::ErrorResult& aRv) const {
   if (!mFrameSelection) {
     aRv.Throw(NS_ERROR_NOT_INITIALIZED);
     return Nullable<int16_t>();
   }
   nsBidiLevel caretBidiLevel = mFrameSelection->GetCaretBidiLevel();
   return (caretBidiLevel & BIDI_LEVEL_UNDEFINED)
@@ -908,26 +911,26 @@ nsresult Selection::SubtractRange(RangeD
     }
   }
 
   return NS_OK;
 }
 
 void Selection::UserSelectRangesToAdd(nsRange* aItem,
                                       nsTArray<RefPtr<nsRange>>& aRangesToAdd) {
-  aItem->ExcludeNonSelectableNodes(&aRangesToAdd);
-  if (aRangesToAdd.IsEmpty()) {
-    ErrorResult err;
-    nsINode* node = aItem->GetStartContainer(err);
-    if (node && node->IsContent() && node->AsContent()->GetEditingHost()) {
-      // A contenteditable node with user-select:none, for example.
-      // Allow it to have a collapsed selection (for the caret).
-      aItem->Collapse(GetDirection() == eDirPrevious);
-      aRangesToAdd.AppendElement(aItem);
-    }
+  // We cannot directly call IsEditorSelection() because we may be in an
+  // inconsistent state during Collapse() (we're cleared already but we haven't
+  // got a new focus node yet).
+  if (IsEditorNode(aItem->GetStartContainer()) &&
+      IsEditorNode(aItem->GetEndContainer())) {
+    // Don't mess with the selection ranges for editing, editor doesn't really
+    // deal well with multi-range selections.
+    aRangesToAdd.AppendElement(aItem);
+  } else {
+    aItem->ExcludeNonSelectableNodes(&aRangesToAdd);
   }
 }
 
 nsresult Selection::AddItem(nsRange* aItem, int32_t* aOutIndex,
                             bool aNoStartSelect) {
   if (!aItem) return NS_ERROR_NULL_POINTER;
   if (!aItem->IsPositioned()) return NS_ERROR_UNEXPECTED;
 
--- a/dom/base/test/test_user_select.html
+++ b/dom/base/test/test_user_select.html
@@ -29,16 +29,17 @@ a { position:absolute; bottom: 0; right:
 <div id="test6">aaaaaaabbbbbbbb<x><s>ccccccc</s></x></div>
 <div id="test7">aaaaaaa<x><s><n>bbbb</n>bbbb</s></x>ccccccc</div>
 <div id="test8"><x><s>aa<n>aaa</n>aa</s></x>bbbbbbbbccccccc</div>
 <div id="test9">aaaaaaabbbbbbbb<x><s>cc<n>ccccc</n></s></x></div>
 <div id="testA">aaaaaaa<n>bbb<s>bbbbb</s></n>ccccccc</div>
 <div id="testB"><n><s>aaaa</s>aaa</n>bbbbbbbbccccccc</div>
 <div id="testC">aaaaaaabbbbbbbb<n>cc<s>c</s>cccc</n></div>
 <div id="testE">aaa<s id="testEc1">aaaa<a class="text">bbbb</a>dd<a>cccc</a>ddddddd</s>eeee</div>
+<div id="testI">aaa<span contenteditable="true" spellcheck="false">bbb</span><s>ccc</s>ddd</div>
 <div id="testF">aaaa
 <div class="non-selectable">x</div>
 <div class="non-selectable">x</div>
 <div class="non-selectable">x</div>
 bbbb</div>
 <div id="testG" style="white-space:pre">aaaa
 <div class="non-selectable">x</div>
 <div class="non-selectable">x</div>
@@ -240,16 +241,23 @@ function test()
   dragSelect(e, 20, 360, 295);
   checkText('aa\nbbbb\nee', e);
   checkRangeCount(3, e);
   checkRange(0, [0,1,-1,1], e);
   checkRange(1, [1,0,-1,2], e.children[0]);
   checkRange(2, [2,0,2,2], e);
   doneTest(e);
 
+  clear();
+  e = document.getElementById('testI');
+  dragSelect(e, 200, 80);
+  checkText('bbd', e);
+  checkRangeCount(2, e);
+  doneTest(e);
+
   // ======================================================
   // ================== shift+click tests =================
   // ======================================================
 
   // test extending a selection that starts in a -moz-user-select:none node
   clear();
   e = document.getElementById('test2');
   init([[0,0,0,1]], e);
new file mode 100644
--- /dev/null
+++ b/layout/base/tests/bug1524266-1-ref.html
@@ -0,0 +1,30 @@
+<!doctype html>
+<html class="reftest-wait">
+<title>Caret doesn't get stuck in a select element inside an editor</title>
+<script src="/tests/SimpleTest/EventUtils.js"></script>
+<script src="/tests/SimpleTest/SimpleTest.js"></script>
+<style>
+  div:focus-within {
+    outline: 3px solid blue;
+  }
+  span {
+    outline: none;
+  }
+</style>
+<div>
+  xx
+  <select>
+    <option value="">Placeholder</option>
+  </select>
+  <span contenteditable="true" spellcheck="false">xxx</span>
+</div>
+<script>
+SimpleTest.waitForFocus(function() {
+  document.querySelector('[contenteditable="true"]').focus();
+  requestAnimationFrame(function() {
+    for (let i = 0; i < 2; ++i)
+      synthesizeKey("KEY_ArrowRight");
+    document.documentElement.removeAttribute("class");
+  });
+});
+</script>
new file mode 100644
--- /dev/null
+++ b/layout/base/tests/bug1524266-1.html
@@ -0,0 +1,27 @@
+<!doctype html>
+<html class="reftest-wait">
+<title>Caret doesn't get stuck in a select element inside an editor</title>
+<script src="/tests/SimpleTest/EventUtils.js"></script>
+<script src="/tests/SimpleTest/SimpleTest.js"></script>
+<style>
+  div:focus {
+    outline: 3px solid blue;
+  }
+</style>
+<div contenteditable="true" spellcheck="false">
+  xx
+  <select>
+    <option value="">Placeholder</option>
+  </select>
+  xxx
+</div>
+<script>
+SimpleTest.waitForFocus(function() {
+  document.querySelector('[contenteditable="true"]').focus();
+  requestAnimationFrame(function() {
+    for (let i = 0; i < 7; ++i)
+      synthesizeKey("KEY_ArrowRight");
+    document.documentElement.removeAttribute("class");
+  });
+});
+</script>
new file mode 100644
--- /dev/null
+++ b/layout/base/tests/bug1524266-2-ref.html
@@ -0,0 +1,24 @@
+<!doctype html>
+<html class="reftest-wait">
+<title>Can delete non-selectable content in an editor</title>
+<script src="/tests/SimpleTest/EventUtils.js"></script>
+<script src="/tests/SimpleTest/SimpleTest.js"></script>
+<style>
+  div:focus {
+    outline: 3px solid blue;
+  }
+</style>
+<div contenteditable="true" spellcheck="false">
+  xxxxx
+</div>
+<script>
+SimpleTest.waitForFocus(function() {
+  document.querySelector('[contenteditable="true"]').focus();
+  requestAnimationFrame(function() {
+    // Move after the two x
+    for (let i = 0; i < 2; ++i)
+      synthesizeKey("KEY_ArrowRight");
+    document.documentElement.removeAttribute("class");
+  });
+});
+</script>
new file mode 100644
--- /dev/null
+++ b/layout/base/tests/bug1524266-2.html
@@ -0,0 +1,38 @@
+<!doctype html>
+<html class="reftest-wait">
+<title>Can delete non-selectable content in an editor</title>
+<script src="/tests/SimpleTest/EventUtils.js"></script>
+<script src="/tests/SimpleTest/SimpleTest.js"></script>
+<style>
+  div:focus {
+    outline: 3px solid blue;
+  }
+  /* <select> has user-select: none in the UA sheet, but just in case */
+  select {
+    -moz-user-select: none;
+    user-select: none;
+  }
+</style>
+<div contenteditable="true" spellcheck="false">
+  xx
+  <select>
+    <option value="">Placeholder</option>
+  </select>
+  xxx
+</div>
+<script>
+SimpleTest.waitForFocus(function() {
+  document.querySelector('[contenteditable="true"]').focus();
+  requestAnimationFrame(function() {
+    // Move after the two x
+    for (let i = 0; i < 2; ++i)
+      synthesizeKey("KEY_ArrowRight");
+    // Select whitespace + <select> + whitespace.
+    for (let i = 0; i < 3; ++i)
+      synthesizeKey("KEY_ArrowRight", { shiftKey: true });
+    // Rip it off.
+    synthesizeKey("KEY_Delete");
+    document.documentElement.removeAttribute("class");
+  });
+});
+</script>
new file mode 100644
--- /dev/null
+++ b/layout/base/tests/bug1524266-3.html
@@ -0,0 +1,37 @@
+<!doctype html>
+<html class="reftest-wait">
+<title>Can delete non-selectable content in an editor</title>
+<script src="/tests/SimpleTest/EventUtils.js"></script>
+<script src="/tests/SimpleTest/SimpleTest.js"></script>
+<style>
+  div:focus {
+    outline: 3px solid blue;
+  }
+  span {
+    -moz-user-select: none;
+    user-select: none;
+  }
+</style>
+<div contenteditable="true" spellcheck="false">
+  xx
+  <span>
+    NOT EDITABLE
+  </span>
+  xxx
+</div>
+<script>
+SimpleTest.waitForFocus(function() {
+  document.querySelector('[contenteditable="true"]').focus();
+  requestAnimationFrame(function() {
+    // Move after the two x
+    for (let i = 0; i < 2; ++i)
+      synthesizeKey("KEY_ArrowRight");
+    // Select whitespace + <span>
+    for (let i = 0; i < 2; ++i)
+      synthesizeKey("KEY_ArrowRight", { shiftKey: true });
+    // Rip it off.
+    synthesizeKey("KEY_Delete");
+    document.documentElement.removeAttribute("class");
+  });
+});
+</script>
new file mode 100644
--- /dev/null
+++ b/layout/base/tests/bug1524266-4.html
@@ -0,0 +1,33 @@
+<!doctype html>
+<html class="reftest-wait">
+<title>Can delete non-editable content in an editor</title>
+<script src="/tests/SimpleTest/EventUtils.js"></script>
+<script src="/tests/SimpleTest/SimpleTest.js"></script>
+<style>
+  div:focus {
+    outline: 3px solid blue;
+  }
+</style>
+<div contenteditable="true" spellcheck="false">
+  xx
+  <span contenteditable="false">
+    NOT EDITABLE
+  </span>
+  xxx
+</div>
+<script>
+SimpleTest.waitForFocus(function() {
+  document.querySelector('[contenteditable="true"]').focus();
+  requestAnimationFrame(function() {
+    // Move after the two x
+    for (let i = 0; i < 2; ++i)
+      synthesizeKey("KEY_ArrowRight");
+    // Select whitespace + <span>
+    for (let i = 0; i < 2; ++i)
+      synthesizeKey("KEY_ArrowRight", { shiftKey: true });
+    // Rip it off.
+    synthesizeKey("KEY_Delete");
+    document.documentElement.removeAttribute("class");
+  });
+});
+</script>
--- a/layout/base/tests/mochitest.ini
+++ b/layout/base/tests/mochitest.ini
@@ -326,16 +326,22 @@ support-files =
   bug1484094-1.html
   bug1484094-1-ref.html
   bug1484094-2.html
   bug1484094-2-ref.html
   bug1510942-1.html
   bug1510942-1-ref.html
   bug1510942-2.html
   bug1510942-2-ref.html
+  bug1524266-1.html
+  bug1524266-1-ref.html
+  bug1524266-2.html
+  bug1524266-2-ref.html
+  bug1524266-3.html
+  bug1524266-4.html
   image_rgrg-256x256.png
   input-invalid-ref.html
   input-maxlength-invalid-change.html
   input-maxlength-ui-invalid-change.html
   input-maxlength-ui-valid-change.html
   input-maxlength-valid-before-change.html
   input-maxlength-valid-change.html
   input-minlength-invalid-change.html
--- a/layout/base/tests/test_reftests_with_caret.html
+++ b/layout/base/tests/test_reftests_with_caret.html
@@ -211,16 +211,17 @@ var tests = [
     [ 'bug1506547-2.html' , 'bug1506547-3.html' ] ,
     [ 'bug1506547-4.html' , 'bug1506547-4-ref.html' ] ,
     [ 'bug1506547-5.html' , 'bug1506547-5-ref.html' ] ,
     [ 'bug1506547-6.html' , 'bug1506547-5-ref.html' ] ,
     [ 'bug1510942-1.html' , 'bug1510942-1-ref.html' ] ,
     [ 'bug1510942-2.html' , 'bug1510942-2-ref.html' ] ,
     [ 'bug1518339-1.html' , 'bug1518339-1-ref.html' ] ,
     [ 'bug1518339-2.html' , 'bug1518339-2-ref.html' ] ,
+    [ 'bug1524266-1.html' , 'bug1524266-1-ref.html' ] ,
     function() {SpecialPowers.pushPrefEnv({'clear': [['layout.accessiblecaret.enabled']]}, nextTest);} ,
 ];
 
 if (!navigator.appVersion.includes("Android")) {
   tests.push([ 'bug512295-1.html' , 'bug512295-1-ref.html' ]);
   tests.push([ 'bug512295-2.html' , 'bug512295-2-ref.html' ]);
   tests.push([ 'bug923376.html'   , 'bug923376-ref.html'   ]);
   tests.push(function() {SpecialPowers.pushPrefEnv({'set': [['layout.css.overflow-clip-box.enabled', true]]}, nextTest);});
@@ -322,16 +323,22 @@ if (navigator.platform.includes("Linux")
     [ 'multi-range-script-select.html#next6SA' , 'multi-range-script-select-ref.html#next6SA'  ] ,
     [ 'multi-range-script-select.html#next7SA' , 'multi-range-script-select-ref.html#next7SA'  ] ,
     // eDirNext, Accel+drag-select (adding an additional range)
     [ 'multi-range-script-select.html#next1AD' , 'multi-range-script-select-ref.html#next1AD'  ] ,
     [ 'multi-range-script-select.html#next7AD' , 'multi-range-script-select-ref.html#next7AD'  ] ,
     // eDirNext, VK_RIGHT / LEFT
     [ 'multi-range-script-select.html#next1SR' , 'multi-range-script-select-ref.html#next1SR'  ] ,
     [ 'multi-range-script-select.html#next1SL' , 'multi-range-script-select-ref.html#next1SL'  ] ,
+
+    // Tries to select and delete non-selectable content in a user-select subtree.
+    [ 'bug1524266-2.html' , 'bug1524266-2-ref.html' ] ,
+    [ 'bug1524266-3.html' , 'bug1524266-2-ref.html' ] ,
+    // Tries to select and delete non-editable content in a user-select subtree.
+    [ 'bug1524266-4.html' , 'bug1524266-2-ref.html' ] ,
   ]);
 }
 
 var testIndex = 0;
 
 function nextTest() {
   if (testIndex < tests.length) {
     if (typeof(tests[testIndex]) == 'function') {
--- a/layout/generic/nsFrame.cpp
+++ b/layout/generic/nsFrame.cpp
@@ -8548,21 +8548,17 @@ nsresult nsIFrame::GetFrameFromDirection
     if (NS_FAILED(result)) return result;
 
     if (aDirection == eDirNext)
       frameTraversal->Next();
     else
       frameTraversal->Prev();
 
     traversedFrame = frameTraversal->CurrentItem();
-
-    // Skip anonymous elements, but watch out for generated content
-    if (!traversedFrame ||
-        (!traversedFrame->IsGeneratedContentFrame() &&
-         traversedFrame->GetContent()->IsRootOfNativeAnonymousSubtree())) {
+    if (!traversedFrame) {
       return NS_ERROR_FAILURE;
     }
 
     auto IsSelectable = [aForceEditableRegion](const nsIFrame* aFrame) {
       if (!aFrame->IsSelectable(nullptr)) {
         return false;
       }
       return !aForceEditableRegion || aFrame->GetContent()->IsEditable();