Bug 1574852 - part 100: Get rid of `TextEditRules::DidDeleteSelection()` r=m_kato

There are only 3 callers and it does simple but different 2 things.  One of
the callers is `HTMLEditRules::DidDeleteSelection()` so that if same things
are done by `TextEditor::DeleteSelectionAsSubAction()`, it does not need to
duplicate the code.  Therefore, we need to duplicate the code into
`TextEditor::DeleteSelectionAsSubAction()` and `TextEditRules::WillSetText()`.
Then, `TextEditRules::WillSetText()` can avoid accessing `Selection` since
it still grabs the modified text node.

Note that only when it's called by `TextEditRules::DidDoAction()`,
`AutoTransactionsConserveSelection` has been set.  However, neither
`DeleteNodeWithTransaction()` nor `DeleteNodeTransaction::DoTransaction()`
changes `Selection`.  Therefore, it hasn't do anything.  So, we can remove
it right now.

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

--HG--
extra : moz-landing-system : lando

editor/libeditor/HTMLEditRules.cpp

@@ -4109,12 +4109,7 @@ nsresult HTMLEditRules::DidDeleteSelection() {
       }
     }
   }
-
+  return NS_OK;
-  // call through to base class
-  nsresult rv = TextEditRules::DidDeleteSelection();
-  NS_WARNING_ASSERTION(NS_SUCCEEDED(rv),
-                       "TextEditRules::DidDeleteSelection() failed");
-  return rv;
 }
 
 EditActionResult HTMLEditor::MakeOrChangeListAndListItemAsSubAction(

editor/libeditor/TextEditRules.cpp

@@ -259,16 +259,10 @@ nsresult TextEditRules::DidDoAction(EditSubActionInfo& aInfo,
     return NS_ERROR_EDITOR_DESTROYED;
   }
 
-  AutoSafeEditorData setData(*this, *mTextEditor);
-
-  // don't let any txns in here move the selection around behind our back.
-  // Note that this won't prevent explicit selection setting from working.
-  AutoTransactionsConserveSelection dontChangeMySelection(TextEditorRef());
-
   switch (aInfo.mEditSubAction) {
     case EditSubAction::eDeleteSelectedContent:
       MOZ_ASSERT(!mIsHTMLEditRules);
-      return DidDeleteSelection();
+      return NS_OK;
     case EditSubAction::eInsertElement:
     case EditSubAction::eUndo:
     case EditSubAction::eRedo:
@@ -869,11 +863,21 @@ nsresult TextEditRules::WillSetText(bool* aCancel, bool* aHandled,
 
   // If we replaced non-empty value with empty string, we need to delete the
   // text node.
-  if (tString.IsEmpty()) {
+  if (tString.IsEmpty() && !textNode->Length()) {
-    DebugOnly<nsresult> rvIgnored = DidDeleteSelection();
+    nsresult rv =
-    MOZ_ASSERT(rvIgnored != NS_ERROR_EDITOR_DESTROYED);
+        MOZ_KnownLive(TextEditorRef()).DeleteNodeWithTransaction(*textNode);
-    NS_WARNING_ASSERTION(NS_SUCCEEDED(rvIgnored),
+    if (NS_WARN_IF(rv == NS_ERROR_EDITOR_DESTROYED)) {
-                         "DidDeleteSelection() failed");
+      return NS_ERROR_EDITOR_DESTROYED;
+    }
+    NS_WARNING_ASSERTION(NS_SUCCEEDED(rv),
+                         "DeleteNodeWithTransaction() failed, but ignored");
+    // XXX I don't think this is necessary because the anonymous `<div>`
+    //     element has now only padding `<br>` element even if there are
+    //     something.
+    IgnoredErrorResult ignoredError;
+    SelectionRefPtr()->SetInterlinePosition(true, ignoredError);
+    NS_WARNING_ASSERTION(!ignoredError.Failed(),
+                         "Selection::SetInterlinePoisition() failed");
   }
 
   *aHandled = true;
@@ -998,43 +1002,6 @@ nsresult TextEditRules::DeleteSelectionWithTransaction(
   return NS_OK;
 }
 
-nsresult TextEditRules::DidDeleteSelection() {
-  MOZ_ASSERT(IsEditorDataAvailable());
-
-  EditorDOMPoint selectionStartPoint(
-      EditorBase::GetStartPoint(*SelectionRefPtr()));
-  if (NS_WARN_IF(!selectionStartPoint.IsSet())) {
-    return NS_ERROR_FAILURE;
-  }
-
-  // Delete empty text nodes at selection.
-  if (selectionStartPoint.IsInTextNode() &&
-      !selectionStartPoint.GetContainer()->Length()) {
-    nsresult rv = MOZ_KnownLive(TextEditorRef())
-                      .DeleteNodeWithTransaction(
-                          MOZ_KnownLive(*selectionStartPoint.GetContainer()));
-    if (NS_WARN_IF(!CanHandleEditAction())) {
-      return NS_ERROR_EDITOR_DESTROYED;
-    }
-    if (NS_WARN_IF(NS_FAILED(rv))) {
-      return rv;
-    }
-  }
-
-  // Note that this may be true only when this is HTMLEditRules.
-  if (TextEditorRef()
-          .TopLevelEditSubActionDataRef()
-          .mDidExplicitlySetInterLine) {
-    return NS_OK;
-  }
-  // We prevent the caret from sticking on the left of prior BR
-  // (i.e. the end of previous line) after this deletion.  Bug 92124
-  ErrorResult err;
-  SelectionRefPtr()->SetInterlinePosition(true, err);
-  NS_WARNING_ASSERTION(!err.Failed(), "Failed to set interline position");
-  return err.StealNSResult();
-}
-
 nsresult TextEditRules::WillOutputText(const nsAString* aOutputFormat,
                                        nsAString* aOutString, uint32_t aFlags,
                                        bool* aCancel, bool* aHandled) {

editor/libeditor/TextEditRules.h

@@ -207,13 +207,6 @@ class TextEditRules {
   MOZ_MUST_USE nsresult DeleteSelectionWithTransaction(
       nsIEditor::EDirection aCollapsedAction, bool* aCancel, bool* aHandled);
 
-  /**
-   * Called after deleted selected content.
-   * This method may remove empty text node and makes guarantee that caret
-   * is never at left of <br> element.
-   */
-  MOZ_CAN_RUN_SCRIPT MOZ_MUST_USE nsresult DidDeleteSelection();
-
   nsresult WillSetTextProperty(bool* aCancel, bool* aHandled);
 
   nsresult WillRemoveTextProperty(bool* aCancel, bool* aHandled);

editor/libeditor/TextEditor.cpp

@@ -661,20 +661,54 @@ nsresult TextEditor::DeleteSelectionAsSubAction(EDirection aDirection,
   subActionInfo.stripWrappers = aStripWrappers;
   bool cancel, handled;
   nsresult rv = rules->WillDoAction(subActionInfo, &cancel, &handled);
+  if (NS_WARN_IF(NS_FAILED(rv)) || cancel) {
+    return rv;
+  }
+  if (!handled) {
+    rv = DeleteSelectionWithTransaction(aDirection, aStripWrappers);
+  }
+  // post-process
+  rv = rules->DidDoAction(subActionInfo, rv);
   if (NS_WARN_IF(NS_FAILED(rv))) {
     return rv;
   }
-  if (!cancel && !handled) {
+
-    rv = DeleteSelectionWithTransaction(aDirection, aStripWrappers);
+  // XXX This is odd.  We just tries to remove empty text node here but we
-  }
+  //     refer `Selection`.  It may be modified by mutation event listeners
-  if (!cancel) {
+  //     so that we should remove the empty text node when we make it empty.
-    // post-process
+  EditorDOMPoint atNewStartOfSelection(
-    rv = rules->DidDoAction(subActionInfo, rv);
+      EditorBase::GetStartPoint(*SelectionRefPtr()));
-    NS_WARNING_ASSERTION(NS_SUCCEEDED(rv),
+  if (NS_WARN_IF(!atNewStartOfSelection.IsSet())) {
-                         "TextEditRules::DidDoAction() failed");
+    // XXX And also it seems that we don't need to return error here.
+    //     Why don't we just ignore?  `Selection::RemoveAllRanges()` may
+    //     have been called by mutation event listeners.
+    return NS_ERROR_FAILURE;
   }
+  if (atNewStartOfSelection.IsInTextNode() &&
+      !atNewStartOfSelection.GetContainer()->Length()) {
+    nsresult rv = DeleteNodeWithTransaction(
+        MOZ_KnownLive(*atNewStartOfSelection.GetContainer()));
+    if (NS_WARN_IF(NS_FAILED(rv))) {
       return rv;
     }
+  }
+
+  // XXX I don't think that this is necessary in anonymous `<div>` element of
+  //     TextEditor since there should be at most one text node and at most
+  //     one padding `<br>` element so that `<br>` element won't be before
+  //     caret.
+  if (!TopLevelEditSubActionDataRef().mDidExplicitlySetInterLine) {
+    // We prevent the caret from sticking on the left of previous `<br>`
+    // element (i.e. the end of previous line) after this deletion. Bug 92124.
+    ErrorResult error;
+    SelectionRefPtr()->SetInterlinePosition(true, error);
+    if (NS_WARN_IF(error.Failed())) {
+      return error.StealNSResult();
+    }
+  }
+
+  return NS_OK;
+}
 
 nsresult TextEditor::DeleteSelectionWithTransaction(
     EDirection aDirection, EStripWrappers aStripWrappers) {