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
authorMasayuki Nakano <masayuki@d-toybox.com>
Thu, 07 May 2020 01:23:21 +0000
changeset 528555 8740667be31ef2258b4910d7df2cbba28776472a
parent 528554 ceee88ee8a92abe0a39d068299ba0d7a3a1be087
child 528556 174327b3c0cfe71a838d65e589f380e79713592e
push id115172
push usermasayuki@d-toybox.com
push dateThu, 07 May 2020 01:23:54 +0000
treeherderautoland@8740667be31e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersm_kato
bugs1633797, 1425997
milestone78.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 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
editor/libeditor/HTMLEditSubActionHandler.cpp
editor/libeditor/tests/test_bug1425997.html
--- a/editor/libeditor/EditorBase.cpp
+++ b/editor/libeditor/EditorBase.cpp
@@ -4056,27 +4056,31 @@ nsresult EditorBase::DeleteSelectionWith
     nsIEditor::EStripWrappers aStripWrappers) {
   MOZ_ASSERT(IsEditActionDataAvailable());
   MOZ_ASSERT(aStripWrappers == eStrip || aStripWrappers == eNoStrip);
 
   if (NS_WARN_IF(Destroyed())) {
     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;
   }
 
   // XXX This is odd, this assumes that there are no multiple collapsed
   //     ranges in `Selection`, but it's possible scenario.
   // XXX This loop looks slow, but it's rarely so because of multiple
--- a/editor/libeditor/HTMLEditSubActionHandler.cpp
+++ b/editor/libeditor/HTMLEditSubActionHandler.cpp
@@ -2354,32 +2354,39 @@ EditActionResult HTMLEditor::HandleDelet
 
   EditActionResult result =
       HandleDeleteSelectionInternal(aDirectionAndAmount, aStripWrappers);
   if (result.Failed() || result.Canceled()) {
     NS_WARNING_ASSERTION(result.Succeeded(),
                          "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) {
       NS_WARNING(
           "EditorBase::DeleteSelectionWithTransaction() caused destroying the "
           "editor");
       return EditActionResult(NS_ERROR_EDITOR_DESTROYED);
     }
     NS_WARNING_ASSERTION(
         NS_SUCCEEDED(rv),
         "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())) {
     return EditActionHandled(NS_ERROR_FAILURE);
   }
   if (atNewStartOfSelection.GetContainerAsContent()) {
     nsresult rv = DeleteMostAncestorMailCiteElementIfEmpty(
         MOZ_KnownLive(*atNewStartOfSelection.GetContainerAsContent()));
@@ -3207,27 +3214,32 @@ EditActionResult HTMLEditor::HandleDelet
       NS_WARNING("Mutation event listener changed the DOM tree");
       return EditActionHandled(NS_ERROR_EDITOR_UNEXPECTED_DOM_TREE);
     }
   }
 
   // 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);
 
       nsresult rv =
           DeleteSelectionWithTransaction(aDirectionAndAmount, aStripWrappers);
       if (NS_FAILED(rv)) {
         NS_WARNING("EditorBase::DeleteSelectionWithTransaction() failed");
         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(
         NS_SUCCEEDED(rv),
         "HTMLEditor::DeleteUnnecessaryNodesAndCollapseSelection() failed");
     return EditActionHandled(rv);
   }
 
@@ -3459,17 +3471,17 @@ nsresult HTMLEditor::DeleteUnnecessaryNo
     const EditorDOMPoint& aSelectionStartPoint,
     const EditorDOMPoint& aSelectionEndPoint) {
   MOZ_ASSERT(IsTopLevelEditSubActionDataAvailable());
 
   EditorDOMPoint atCaret(aSelectionStartPoint);
   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() &&
                 atCaret.Offset() == selectionEndPoint.Offset()) ||
                (atCaret.GetContainer()->GetNextSibling() ==
                     selectionEndPoint.GetContainer() &&
                 atCaret.IsEndOfContainer() &&
                 selectionEndPoint.IsStartOfContainer()));
--- a/editor/libeditor/tests/test_bug1425997.html
+++ b/editor/libeditor/tests/test_bug1425997.html
@@ -21,18 +21,19 @@ https://bugzilla.mozilla.org/show_bug.cg
 </div>
 
 <pre id="test">
 
 <script class="testbody" type="application/javascript">
 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() {
     // Until removing all NBSPs which were inserted by the editor,
     // emulates Backspace key with "delete" command.
     // When this test is created, the behavior is:
     //   after 1st delete: "\n<!-- -->&nbsp;&nbsp;\n"
     //   after 2nd delete: "\n<!-- -->&nbsp;\n"