Bug 1524266 - Should be able to delete non-selectable and non-editable content in a contenteditable subtree. r=mats
☠☠ backed out by 34d90155fe7a ☠ ☠
authorEmilio Cobos Álvarez <emilio@crisal.io>
Sun, 03 Feb 2019 23:13:09 +0000
changeset 456596 4d5cbdd058591f298ca12a881cd3259dbdacc6d0
parent 456595 a62c6b273d4a9af1b60a48c81dd5e119bc9ef1ab
child 456597 c514d7a4330b6caf0eef46d37f9f3ba17d9953f4
push id35494
push useropoprus@mozilla.com
push dateMon, 04 Feb 2019 09:23:09 +0000
treeherdermozilla-central@965b28c159d6 [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
@@ -3970,18 +3970,30 @@ nsresult nsFrame::GetDataForTableSelecti
   return NS_OK;
 }
 
 static bool IsEditingHost(const nsIFrame* aFrame) {
   auto* element = nsGenericHTMLElement::FromNodeOrNull(aFrame->GetContent());
   return element && element->IsEditableRoot();
 }
 
+static bool IsUnselectable(const nsIFrame* aFrame) {
+  // Pseudo-elements are not selectable.
+  if (aFrame->HasAnyStateBits(NS_FRAME_GENERATED_CONTENT)) {
+    return true;
+  }
+
+  // Native anonymous content that is not editable is not selectable either.
+  auto* content = aFrame->GetContent();
+  return content && !content->IsEditable() &&
+         content->IsInNativeAnonymousSubtree();
+}
+
 static StyleUserSelect UsedUserSelect(const nsIFrame* aFrame) {
-  if (aFrame->HasAnyStateBits(NS_FRAME_GENERATED_CONTENT)) {
+  if (IsUnselectable(aFrame)) {
     return StyleUserSelect::None;
   }
 
   // Per https://drafts.csswg.org/css-ui-4/#content-selection:
   //
   // The computed value is the specified value, except:
   //
   //   1 - on editable elements where the computed value is always 'contain'
@@ -8548,21 +8560,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();