Bug 1633797 - Make callers of `EditorBase::DeleteSelectionWithTransaction()` stop calling it if selection is collapsed but the caller tries to delete non-collapsed range r=m_kato

Our editor's deletion code removes nodes step-by-step.  Therefore, even when
somebodies call `DeleteSelectionWithTransaction()` for removing non-collapsed
ranges, they may have already removed all contents in the range.  In such
case, all callers shouldn't call `DeleteSelectionWithTransaction()`.

This makes `test_bug1425997.html` allow to run nexted `execCommand`.  It'll be
disabled even in the release channel, but we should keep testing it for
detecting bug of edge cases (like this bug).  Note that all crashtests which
test nested `execCommand` calls run with allowing it with the pref for same
reason.

Differential Revision: https://phabricator.services.mozilla.com/D73402

editor/libeditor/EditorBase.cpp

@@ -4061,17 +4061,21 @@ nsresult EditorBase::DeleteSelectionWithTransaction(
     return NS_ERROR_EDITOR_DESTROYED;
   }
 
+  HowToHandleCollapsedRange howToHandleCollapsedRange =
+      EditorBase::HowToHandleCollapsedRangeFor(aDirectionAndAmount);
   if (NS_WARN_IF(!SelectionRefPtr()->RangeCount()) ||
-      NS_WARN_IF(
-          SelectionRefPtr()->IsCollapsed() &&
-          EditorBase::HowToHandleCollapsedRangeFor(aDirectionAndAmount) ==
-              HowToHandleCollapsedRange::Ignore)) {
+      NS_WARN_IF(SelectionRefPtr()->IsCollapsed() &&
+                 howToHandleCollapsedRange ==
+                     HowToHandleCollapsedRange::Ignore)) {
+    NS_ASSERTION(
+        false,
+        "For avoiding to throw incompatible exception for `execCommand`, fix "
+        "the caller");
     return NS_ERROR_FAILURE;
   }
 
   RefPtr<EditAggregateTransaction> deleteSelectionTransaction =
-      CreateTransactionForDeleteSelection(
-          EditorBase::HowToHandleCollapsedRangeFor(aDirectionAndAmount));
+      CreateTransactionForDeleteSelection(howToHandleCollapsedRange);
   if (!deleteSelectionTransaction) {
     NS_WARNING("EditorBase::CreateTransactionForDeleteSelection() failed");
     return NS_ERROR_FAILURE;

editor/libeditor/HTMLEditSubActionHandler.cpp

@@ -2359,9 +2359,13 @@ EditActionResult HTMLEditor::HandleDeleteSelection(
                          "HTMLEditor::HandleDeleteSelectionInternal() failed");
     return result;
   }
-  if (!result.Handled()) {
-    // If it's just ignored, we should fall this back to
-    // `DeleteSelectionWithTransaction()`.
+  // If it's just ignored, we should fall this back to
+  // `DeleteSelectionWithTransaction()` when selection is not collapsed or
+  // content around collapsed range should be deleted.
+  if (!result.Handled() && SelectionRefPtr()->RangeCount() > 0 &&
+      (!SelectionRefPtr()->IsCollapsed() ||
+       EditorBase::HowToHandleCollapsedRangeFor(aDirectionAndAmount) !=
+           HowToHandleCollapsedRange::Ignore)) {
     nsresult rv =
         DeleteSelectionWithTransaction(aDirectionAndAmount, aStripWrappers);
     if (rv == NS_ERROR_EDITOR_DESTROYED) {
@@ -2375,6 +2379,9 @@ EditActionResult HTMLEditor::HandleDeleteSelection(
         "EditorBase::DeleteSelectionWithTransaction() failed, but ignored");
   }
 
+  // XXX At here, selection may have no range because of mutation event
+  //     listeners can do anything so that we should just return NS_OK instead
+  //     of returning error.
   EditorDOMPoint atNewStartOfSelection(
       EditorBase::GetStartPoint(*SelectionRefPtr()));
   if (NS_WARN_IF(!atNewStartOfSelection.IsSet())) {
@@ -3212,7 +3219,10 @@ EditActionResult HTMLEditor::HandleDeleteNonCollapsedSelection(
   // XXX This is odd.  We do we simply use `DeleteSelectionWithTransaction()`
   //     only when **first** range is in same container?
   if (firstRangeStart.GetContainer() == firstRangeEnd.GetContainer()) {
-    {
+    // Because of previous DOM tree changes, the range may be collapsed.
+    // If we've already removed all contents in the range, we shouldn't
+    // delete anything around the caret.
+    if (firstRangeStart != firstRangeEnd) {
       AutoTrackDOMPoint startTracker(RangeUpdaterRef(), &firstRangeStart);
       AutoTrackDOMPoint endTracker(RangeUpdaterRef(), &firstRangeEnd);
 
@@ -3223,6 +3233,8 @@ EditActionResult HTMLEditor::HandleDeleteNonCollapsedSelection(
         return EditActionHandled(rv);
       }
     }
+    // However, even if the range is removed, we may need to clean up the
+    // containers which become empty.
     nsresult rv = DeleteUnnecessaryNodesAndCollapseSelection(
         aDirectionAndAmount, firstRangeStart, firstRangeEnd);
     NS_WARNING_ASSERTION(
@@ -3464,7 +3476,7 @@ nsresult HTMLEditor::DeleteUnnecessaryNodesAndCollapseSelection(
   EditorDOMPoint selectionEndPoint(aSelectionEndPoint);
 
   // If we're handling D&D, this is called to delete dragging item from the
-  // tree.  In this case, we should move parent blocks if it becomes empty.
+  // tree.  In this case, we should remove parent blocks if it becomes empty.
   if (GetEditAction() == EditAction::eDrop ||
       GetEditAction() == EditAction::eDeleteByDrag) {
     MOZ_ASSERT((atCaret.GetContainer() == selectionEndPoint.GetContainer() &&

editor/libeditor/tests/test_bug1425997.html

@@ -26,8 +26,9 @@ https://bugzilla.mozilla.org/show_bug.cgi?id=1425997
 SimpleTest.waitForExplicitFinish();
 // 3 assertions are recorded due to nested execCommand() but not a problem.
 // They are necessary to detect invalid method call without mutation event listers.
-SimpleTest.expectAssertions(0, 3);
-SimpleTest.waitForFocus(function() {
+SimpleTest.expectAssertions(3, 3);
+SimpleTest.waitForFocus(async function() {
+  await SpecialPowers.pushPrefEnv({set: [["dom.document.exec_command.nested_calls_allowed", true]]});
   let selection = window.getSelection();
   let editor = document.getElementById("editor");
   function onCharacterDataModified() {