Bug 1677566 - part 3: Ignore non-deletable ranges in `HTMLEditor::Han…

…dleDeleteSelection()` r=m_kato For making delete handlers simpler, and set better target ranges to the corresponding `beforeinput` event, we should ignore non-editable ranges before handling deletion. This patch makes editor stop handling deleteion when a range crosses editing host boundaries. In this case, Gecko has done nothing, but fired `beforeinput` event. Note that Blink deletes editable contents in the range **until** it meets first non-editable content, but I don't think this is a good behavior because it makes things complicated. Therefore, I filed a spec issue: w3c/editing#283 On the other hand, this behavior change causes different behavior in https://searchfox.org/mozilla-central/source/editor/libeditor/crashtests/1345015.html It tries to insert paragraph into `<html>` element, but our editor currently does not support it. Therefore, it hits `MOZ_ASSERT`. Therefore, this patch added a new check into `HTMLEditor::InsertParagraphSeparatorAsSubAction()`. Differential Revision: https://phabricator.services.mozilla.com/D107588
dothq · Mar 9, 2021 · 7af10ce · 7af10ce
1 parent 1229430
commit 7af10ce
Show file tree

Hide file tree

Showing 13 changed files with 762 additions and 53 deletions.
diff --git a/editor/libeditor/EditorBase.cpp b/editor/libeditor/EditorBase.cpp
@@ -5385,6 +5385,11 @@ nsresult EditorBase::AutoEditActionDataSetter::MaybeDispatchBeforeInputEvent(
           NS_WARNING("HTMLEditor::ComputeTargetRanges() destroyed the editor");
           return NS_ERROR_EDITOR_DESTROYED;
         }
+        if (rv == NS_ERROR_EDITOR_NO_EDITABLE_RANGE) {
+          // For now, keep dispatching `beforeinput` event even if no selection
+          // range can be editable.
+          rv = NS_OK;
+        }
         NS_WARNING_ASSERTION(
             NS_SUCCEEDED(rv),
             "HTMLEditor::ComputeTargetRanges() failed, but ignored");

diff --git a/editor/libeditor/EditorBase.h b/editor/libeditor/EditorBase.h
@@ -2084,6 +2084,13 @@ class EditorBase : public nsIEditor,
       // DOM_SUCCESS_DOM_NO_OPERATION here.
       case NS_ERROR_EDITOR_ACTION_CANCELED:
         return NS_SUCCESS_DOM_NO_OPERATION;
+      // If there is no selection range or editable selection ranges, editor
+      // needs to stop handling it.  However, editor shouldn't return error for
+      // the callers to avoid throwing exception.  However, they may want to
+      // check whether it works or not.  Therefore, we should return
+      // NS_SUCCESS_DOM_NO_OPERATION instead.
+      case NS_ERROR_EDITOR_NO_EDITABLE_RANGE:
+        return NS_SUCCESS_DOM_NO_OPERATION;
       default:
         return aRv;
     }

diff --git a/editor/libeditor/EditorUtils.cpp b/editor/libeditor/EditorUtils.cpp
@@ -85,6 +85,56 @@ EditActionResult& EditActionResult::operator|=(
  * mozilla::AutoRangeArray
  *****************************************************************************/
 
+// static
+bool AutoRangeArray::IsEditableRange(const dom::AbstractRange& aRange,
+                                     const Element& aEditingHost) {
+  // TODO: Perhaps, we should check whether the start/end boundaries are
+  //       first/last point of non-editable element.
+  //       See https://github.com/w3c/editing/issues/283#issuecomment-788654850
+  EditorRawDOMPoint atStart(aRange.StartRef());
+  const bool isStartEditable =
+      atStart.IsInContentNode() &&
+      EditorUtils::IsEditableContent(*atStart.ContainerAsContent(),
+                                     EditorUtils::EditorType::HTML);
+  if (!isStartEditable) {
+    return false;
+  }
+
+  if (!aRange.Collapsed()) {
+    EditorRawDOMPoint atEnd(aRange.EndRef());
+    const bool isEndEditable =
+        atEnd.IsInContentNode() &&
+        EditorUtils::IsEditableContent(*atEnd.ContainerAsContent(),
+                                       EditorUtils::EditorType::HTML);
+    if (!isEndEditable) {
+      return false;
+    }
+
+    // Now, both start and end points are editable, but if they are in
+    // different editing host, we cannot edit the range.
+    if (atStart.ContainerAsContent() != atEnd.ContainerAsContent() &&
+        atStart.ContainerAsContent()->GetEditingHost() !=
+            atEnd.ContainerAsContent()->GetEditingHost()) {
+      return false;
+    }
+  }
+
+  // HTMLEditor does not support modifying outside `<body>` element for now.
+  nsINode* commonAncestor = aRange.GetClosestCommonInclusiveAncestor();
+  return commonAncestor && commonAncestor->IsContent() &&
+         commonAncestor->IsInclusiveDescendantOf(&aEditingHost);
+}
+
+void AutoRangeArray::EnsureOnlyEditableRanges(const Element& aEditingHost) {
+  for (size_t i = mRanges.Length(); i > 0; i--) {
+    const OwningNonNull<nsRange>& range = mRanges[i - 1];
+    if (!AutoRangeArray::IsEditableRange(range, aEditingHost)) {
+      mRanges.RemoveElementAt(i - 1);
+    }
+  }
+  mAnchorFocusRange = mRanges.IsEmpty() ? nullptr : mRanges.LastElement().get();
+}
+
 Result<nsIEditor::EDirection, nsresult>
 AutoRangeArray::ExtendAnchorFocusRangeFor(
     const EditorBase& aEditorBase, nsIEditor::EDirection aDirectionAndAmount) {
@@ -116,6 +166,14 @@ AutoRangeArray::ExtendAnchorFocusRangeFor(
     return Err(NS_ERROR_NOT_INITIALIZED);
   }
 
+  RefPtr<Element> editingHost;
+  if (aEditorBase.IsHTMLEditor()) {
+    editingHost = aEditorBase.AsHTMLEditor()->GetActiveEditingHost();
+    if (!editingHost) {
+      return Err(NS_ERROR_FAILURE);
+    }
+  }
+
   Result<RefPtr<nsRange>, nsresult> result(NS_ERROR_UNEXPECTED);
   nsIEditor::EDirection directionAndAmountResult = aDirectionAndAmount;
   switch (aDirectionAndAmount) {
@@ -243,6 +301,12 @@ AutoRangeArray::ExtendAnchorFocusRangeFor(
     return directionAndAmountResult;
   }
 
+  // If the new range isn't editable, keep using the original range.
+  if (aEditorBase.IsHTMLEditor() &&
+      !AutoRangeArray::IsEditableRange(*extendedRange, *editingHost)) {
+    return aDirectionAndAmount;
+  }
+
   if (NS_WARN_IF(!frameSelection->IsValidSelectionPoint(
           extendedRange->GetStartContainer())) ||
       NS_WARN_IF(!frameSelection->IsValidSelectionPoint(

diff --git a/editor/libeditor/EditorUtils.h b/editor/libeditor/EditorUtils.h
@@ -756,6 +756,15 @@ class MOZ_STACK_CLASS AutoRangeArray final {
     }
   }
 
+  /**
+   * EnsureOnlyEditableRanges() removes ranges which cannot modify.
+   * Note that this is designed only for `HTMLEditor` because this must not
+   * be required by `TextEditor`.
+   */
+  void EnsureOnlyEditableRanges(const dom::Element& aEditingHost);
+  static bool IsEditableRange(const dom::AbstractRange& aRange,
+                              const dom::Element& aEditingHost);
+
   auto& Ranges() { return mRanges; }
   const auto& Ranges() const { return mRanges; }
   auto& FirstRangeRef() { return mRanges[0]; }
@@ -928,6 +937,12 @@ class MOZ_STACK_CLASS AutoRangeArray final {
     return FocusRef().IsSet() ? FocusRef().GetChildAtOffset() : nullptr;
   }
 
+  void RemoveAllRanges() {
+    mRanges.Clear();
+    mAnchorFocusRange = nullptr;
+    mDirection = nsDirection::eDirNext;
+  }
+
  private:
   AutoTArray<mozilla::OwningNonNull<nsRange>, 8> mRanges;
   RefPtr<nsRange> mAnchorFocusRange;

diff --git a/editor/libeditor/HTMLEditSubActionHandler.cpp b/editor/libeditor/HTMLEditSubActionHandler.cpp
@@ -1247,6 +1247,19 @@ EditActionResult HTMLEditor::InsertParagraphSeparatorAsSubAction() {
   if (NS_WARN_IF(!editingHost)) {
     return EditActionIgnored(NS_ERROR_FAILURE);
   }
+  // If the editing host parent element is editable, it means that the editing
+  // host must be a <body> element and the selection may be outside the body
+  // element.  If the selection is outside the editing host, we should not
+  // insert new paragraph nor <br> element.
+  // XXX Currently, we don't support editing outside <body> element, but Blink
+  //     does it.
+  if (editingHost->GetParentElement() &&
+      HTMLEditUtils::IsSimplyEditableNode(*editingHost->GetParentElement()) &&
+      (!atStartOfSelection.IsInContentNode() ||
+       !nsContentUtils::ContentIsFlattenedTreeDescendantOf(
+           atStartOfSelection.ContainerAsContent(), editingHost))) {
+    return EditActionHandled(NS_ERROR_EDITOR_NO_EDITABLE_RANGE);
+  }
 
   // Look for the nearest parent block.  However, don't return error even if
   // there is no block parent here because in such case, i.e., editing host

diff --git a/editor/libeditor/HTMLEditor.cpp b/editor/libeditor/HTMLEditor.cpp
@@ -6118,7 +6118,9 @@ bool HTMLEditor::IsActiveInDOMWindow() const {
   return true;
 }
 
-Element* HTMLEditor::GetActiveEditingHost() const {
+Element* HTMLEditor::GetActiveEditingHost(
+    LimitInBodyElement aLimitInBodyElement /* = LimitInBodyElement::Yes */)
+    const {
   Document* document = GetDocument();
   if (NS_WARN_IF(!document)) {
     return nullptr;
@@ -6145,7 +6147,17 @@ Element* HTMLEditor::GetActiveEditingHost() const {
       content->HasIndependentSelection()) {
     return nullptr;
   }
-  return content->GetEditingHost();
+  Element* candidateEditingHost = content->GetEditingHost();
+  if (!candidateEditingHost) {
+    return nullptr;
+  }
+  // Currently, we don't support editing outside of `<body>` element.
+  return aLimitInBodyElement != LimitInBodyElement::Yes ||
+                 (document->GetBodyElement() &&
+                  nsContentUtils::ContentIsFlattenedTreeDescendantOf(
+                      candidateEditingHost, document->GetBodyElement()))
+             ? candidateEditingHost
+             : document->GetBodyElement();
 }
 
 void HTMLEditor::NotifyEditingHostMaybeChanged() {
@@ -6408,7 +6420,7 @@ nsresult HTMLEditor::GetPreferredIMEState(IMEState* aState) {
 }
 
 already_AddRefed<Element> HTMLEditor::GetInputEventTargetElement() const {
-  RefPtr<Element> target = GetActiveEditingHost();
+  RefPtr<Element> target = GetActiveEditingHost(LimitInBodyElement::No);
   if (target) {
     return target.forget();
   }

diff --git a/editor/libeditor/HTMLEditor.h b/editor/libeditor/HTMLEditor.h
@@ -631,7 +631,9 @@ class HTMLEditor final : public TextEditor,
    * Get an active editor's editing host in DOM window.  If this editor isn't
    * active in the DOM window, this returns NULL.
    */
-  Element* GetActiveEditingHost() const;
+  enum class LimitInBodyElement { No, Yes };
+  Element* GetActiveEditingHost(
+      LimitInBodyElement aLimitInBodyElement = LimitInBodyElement::Yes) const;
 
   /**
    * Retruns true if we're in designMode.

diff --git a/editor/libeditor/HTMLEditorDeleteHandler.cpp b/editor/libeditor/HTMLEditorDeleteHandler.cpp
@@ -1033,6 +1033,12 @@ nsresult HTMLEditor::ComputeTargetRanges(
     AutoRangeArray& aRangesToDelete) {
   MOZ_ASSERT(IsEditActionDataAvailable());
 
+  Element* editingHost = GetActiveEditingHost();
+  if (!editingHost) {
+    aRangesToDelete.RemoveAllRanges();
+    return NS_ERROR_EDITOR_NO_EDITABLE_RANGE;
+  }
+
   // First check for table selection mode.  If so, hand off to table editor.
   SelectedTableCellScanner scanner(aRangesToDelete);
   if (scanner.IsInTableCellSelectionMode()) {
@@ -1047,14 +1053,22 @@ nsresult HTMLEditor::ComputeTargetRanges(
       if (HTMLEditUtils::GetTableCellElementIfOnlyOneSelected(
               aRangesToDelete.Ranges()[i - removedRanges]) !=
           scanner.ElementsRef()[i]) {
+        // XXX Need to manage anchor-focus range too!
         aRangesToDelete.Ranges().RemoveElementAt(i - removedRanges);
         removedRanges++;
       }
     }
     return NS_OK;
   }
 
+  aRangesToDelete.EnsureOnlyEditableRanges(*editingHost);
+  if (aRangesToDelete.Ranges().IsEmpty()) {
+    NS_WARNING(
+        "There is no range which we can delete entire of or around the caret");
+    return NS_ERROR_EDITOR_NO_EDITABLE_RANGE;
+  }
   AutoDeleteRangesHandler deleteHandler;
+  // Should we delete target ranges which cannot delete actually?
   nsresult rv = deleteHandler.ComputeRangesToDelete(*this, aDirectionAndAmount,
                                                     aRangesToDelete);
   NS_WARNING_ASSERTION(
@@ -1071,7 +1085,12 @@ EditActionResult HTMLEditor::HandleDeleteSelection(
              aStripWrappers == nsIEditor::eNoStrip);
 
   if (!SelectionRefPtr()->RangeCount()) {
-    return EditActionCanceled();
+    return EditActionHandled(NS_ERROR_EDITOR_NO_EDITABLE_RANGE);
+  }
+
+  Element* editingHost = GetActiveEditingHost();
+  if (!editingHost) {
+    return EditActionHandled(NS_ERROR_EDITOR_NO_EDITABLE_RANGE);
   }
 
   // Remember that we did a selection deletion.  Used by
@@ -1097,6 +1116,13 @@ EditActionResult HTMLEditor::HandleDeleteSelection(
   }
 
   AutoRangeArray rangesToDelete(*SelectionRefPtr());
+  rangesToDelete.EnsureOnlyEditableRanges(*editingHost);
+  if (rangesToDelete.Ranges().IsEmpty()) {
+    NS_WARNING(
+        "There is no range which we can delete entire the ranges or around the "
+        "caret");
+    return EditActionHandled(NS_ERROR_EDITOR_NO_EDITABLE_RANGE);
+  }
   AutoDeleteRangesHandler deleteHandler;
   EditActionResult result = deleteHandler.Run(*this, aDirectionAndAmount,
                                               aStripWrappers, rangesToDelete);

diff --git a/editor/libeditor/crashtests/crashtests.list b/editor/libeditor/crashtests/crashtests.list
@@ -27,7 +27,7 @@ load 499844-1.html
 load 503709-1.xhtml
 load 513375-1.xhtml
 load 535632-1.xhtml
-load 574558-1.xhtml
+asserts(1) load 574558-1.xhtml # assertion in constructor of TextFragmentData (initialized for non-editable content)
 load 580151-1.xhtml
 load 582138-1.xhtml
 load 612565-1.html
@@ -82,7 +82,7 @@ load 1348851.html
 load 1350772.html
 load 1364133.html
 load 1366176.html
-asserts(2) load 1375131.html # assertion in WSRunScanner::GetEditableBlockParentOrTopmostEditableInlineContent()
+load 1375131.html
 load 1381541.html
 load 1383747.html
 load 1383755.html
@@ -138,4 +138,4 @@ load 1659717.html
 load 1663725.html # throws
 load 1655508.html
 pref(dom.document.exec_command.nested_calls_allowed,true) load 1666556.html
-asserts(2) load 1677566.html # assertion in constructor of TextFragmentData (initialized for non-editable content)
+asserts(1) load 1677566.html # assertion in constructor of TextFragmentData (initialized for non-editable content)
diff --git a/editor/libeditor/tests/test_dom_input_event_on_htmleditor.html b/editor/libeditor/tests/test_dom_input_event_on_htmleditor.html
@@ -69,10 +69,8 @@
 
     // Root element never can be edit target.  If the editTarget is the root
     // element, replace with its body.
-    let isEditTargetIsDescendantOfEditingHost = false;
     if (editTarget == aDocument.documentElement) {
       editTarget = body;
-      isEditTargetIsDescendantOfEditingHost = true;
     }
 
     editTarget.innerHTML = "";
@@ -397,9 +395,7 @@
           synthesizeKey("KEY_Enter", {}, aWindow);
         },
         {
-          innerHTML: !isEditTargetIsDescendantOfEditingHost
-            ? "<div>B</div><div><br></div>"
-            : "B<br><br>",
+          innerHTML:  "<div>B</div><div><br></div>",
           beforeInputEvent: {
             cancelable: true,
             inputType: "insertParagraph",
@@ -423,23 +419,16 @@
     });
 
     function test_typing_C_in_empty_last_line(aTestData) {
-      if (!isEditTargetIsDescendantOfEditingHost) {
-        editTarget.innerHTML = "<div>B</div><div><br></div>";
-        selection.collapse(editTarget.querySelector("div + div"), 0);
-      } else {
-        editTarget.innerHTML = "B<br><br>";
-        selection.collapse(editTarget, 2);
-      }
+      editTarget.innerHTML = "<div>B</div><div><br></div>";
+      selection.collapse(editTarget.querySelector("div + div"), 0);
 
       runTest(
         aTestData,
         () => {
           synthesizeKey("C", {shiftKey: true}, aWindow);
         },
         {
-          innerHTML: !isEditTargetIsDescendantOfEditingHost
-            ? "<div>B</div><div>C<br></div>"
-            : "B<br>C<br>",
+          innerHTML: "<div>B</div><div>C<br></div>",
           beforeInputEvent: {
             cancelable: true,
             inputType: "insertText",
@@ -463,23 +452,16 @@
     });
 
     function test_typing_enter_in_non_empty_last_line(aTestData) {
-      if (!isEditTargetIsDescendantOfEditingHost) {
-        editTarget.innerHTML = "<div>B</div><div>C<br></div>";
-        selection.collapse(editTarget.querySelector("div + div").firstChild, 1);
-      } else {
-        editTarget.innerHTML = "B<br>C<br>";
-        selection.collapse(editTarget.querySelector("br").nextSibling, 1);
-      }
+      editTarget.innerHTML = "<div>B</div><div>C<br></div>";
+      selection.collapse(editTarget.querySelector("div + div").firstChild, 1);
 
       runTest(
         aTestData,
         () => {
           synthesizeKey("KEY_Enter", {}, aWindow);
         },
         {
-          innerHTML: !isEditTargetIsDescendantOfEditingHost
-            ? "<div>B</div><div>C</div><div><br></div>"
-            : "B<br>C<br><br>",
+          innerHTML: "<div>B</div><div>C</div><div><br></div>",
           beforeInputEvent: {
             cancelable: true,
             inputType: "insertParagraph",

diff --git a/.../meta/editing/other/select-all-and-delete-in-html-element-having-contenteditable.html.ini b/.../meta/editing/other/select-all-and-delete-in-html-element-having-contenteditable.html.ini
@@ -1,24 +1,4 @@
 [select-all-and-delete-in-html-element-having-contenteditable.html]
-  [Select All, then, Backspace]
-    expected:
-      if (os == "linux"): FAIL
-      PASS
-
-  [Select All, then, Delete]
-    expected:
-      if (os == "linux"): FAIL
-      PASS
-
-  [Select All, then, execCommand("forwarddelete")]
-    expected:
-      if (os == "linux"): FAIL
-      PASS
-
-  [Select All, then, execCommand("delete")]
-    expected:
-      if (os == "linux"): FAIL
-      PASS
-
   [getSelection().selectAllChildren(document.documentElement), then, Backspace]
     expected: FAIL