Bug 1727185 - part 2: Add removable node checks for more callers of `DeleteNodeTransaction::MaybeCreate()` r=m_kato
authorMasayuki Nakano <masayuki@d-toybox.com>
Fri, 27 Aug 2021 00:49:35 +0000
changeset 590111 42ed78fa08b226f3de66ce279740432bcd393a84
parent 590110 f79cc68cd2feedbb1eb10f2b6fb89974522226a9
child 590112 d3801075f8d6d5883498e3d6d64eb677f1e54507
push id148735
push usermasayuki@d-toybox.com
push dateFri, 27 Aug 2021 00:51:56 +0000
treeherderautoland@42ed78fa08b2 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersm_kato
bugs1727185
milestone93.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 1727185 - part 2: Add removable node checks for more callers of `DeleteNodeTransaction::MaybeCreate()` r=m_kato I just missed some callers of it. And I also make `EditorBase::DeleteNodeTransaction()` a virtual method and `HTMLEditor` override it. Then, even if it's called by helper classes, the `HTMLEditor` version is always called. Differential Revision: https://phabricator.services.mozilla.com/D123578
editor/libeditor/DeleteRangeTransaction.cpp
editor/libeditor/EditorBase.cpp
editor/libeditor/EditorBase.h
editor/libeditor/HTMLEditor.h
--- a/editor/libeditor/DeleteRangeTransaction.cpp
+++ b/editor/libeditor/DeleteRangeTransaction.cpp
@@ -2,16 +2,18 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #include "DeleteRangeTransaction.h"
 
 #include "DeleteNodeTransaction.h"
 #include "DeleteTextTransaction.h"
+#include "EditorUtils.h"
+#include "HTMLEditUtils.h"
 
 #include "mozilla/Assertions.h"
 #include "mozilla/ContentIterator.h"
 #include "mozilla/EditorBase.h"
 #include "mozilla/Logging.h"
 #include "mozilla/mozalloc.h"
 #include "mozilla/RangeBoundary.h"
 #include "mozilla/ToString.h"
@@ -25,16 +27,18 @@
 #include "nsIContent.h"
 #include "nsINode.h"
 #include "nsAString.h"
 
 namespace mozilla {
 
 using namespace dom;
 
+using EditorType = EditorUtils::EditorType;
+
 DeleteRangeTransaction::DeleteRangeTransaction(EditorBase& aEditorBase,
                                                const nsRange& aRangeToDelete)
     : mEditorBase(&aEditorBase), mRangeToDelete(aRangeToDelete.CloneRange()) {}
 
 NS_IMPL_CYCLE_COLLECTION_INHERITED(DeleteRangeTransaction,
                                    EditAggregateTransaction, mEditorBase,
                                    mRangeToDelete)
 
@@ -169,16 +173,22 @@ nsresult DeleteRangeTransaction::CreateT
   }
 
   if (NS_WARN_IF(!mEditorBase)) {
     return NS_ERROR_NOT_AVAILABLE;
   }
 
   // see what kind of node we have
   if (Text* textNode = Text::FromNode(aStart.Container())) {
+    if (mEditorBase->IsHTMLEditor() &&
+        NS_WARN_IF(
+            !EditorUtils::IsEditableContent(*textNode, EditorType::HTML))) {
+      // Just ignore to append the transaction for non-editable node.
+      return NS_OK;
+    }
     // if the node is a chardata node, then delete chardata content
     uint32_t textLengthToDelete;
     if (aStart == aEnd) {
       textLengthToDelete = 1;
     } else {
       textLengthToDelete =
           *aEnd.Offset(RawRangeBoundary::OffsetFilter::kValidOffsets) -
           *aStart.Offset(RawRangeBoundary::OffsetFilter::kValidOffsets);
@@ -238,22 +248,21 @@ nsresult DeleteRangeTransaction::CreateT
 
   // Even if we detect invalid range, we should ignore it for removing
   // specified range's nodes as far as possible.
   // XXX This is super expensive.  Probably, we should make
   //     DeleteNodeTransaction() can treat multiple siblings.
   for (nsIContent* child = aStart.GetChildAtOffset();
        child && child != aEnd.GetChildAtOffset();
        child = child->GetNextSibling()) {
+    if (NS_WARN_IF(!HTMLEditUtils::IsRemovableNode(*child))) {
+      continue;  // Should we abort?
+    }
     RefPtr<DeleteNodeTransaction> deleteNodeTransaction =
         DeleteNodeTransaction::MaybeCreate(*mEditorBase, *child);
-    // XXX This is odd handling.  Even if some children are not editable,
-    //     editor should append transactions because they could be editable
-    //     at undoing/redoing.  Additionally, if the transaction needs to
-    //     delete/restore all nodes, it should at undoing/redoing.
     if (deleteNodeTransaction) {
       DebugOnly<nsresult> rvIgnored = AppendChild(deleteNodeTransaction);
       NS_WARNING_ASSERTION(
           NS_SUCCEEDED(rvIgnored),
           "DeleteRangeTransaction::AppendChild() failed, but ignored");
     }
   }
 
@@ -320,27 +329,24 @@ nsresult DeleteRangeTransaction::CreateT
   }
 
   for (; !subtreeIter.IsDone(); subtreeIter.Next()) {
     nsINode* node = subtreeIter.GetCurrentNode();
     if (NS_WARN_IF(!node) || NS_WARN_IF(!node->IsContent())) {
       return NS_ERROR_FAILURE;
     }
 
+    if (NS_WARN_IF(!HTMLEditUtils::IsRemovableNode(*node->AsContent()))) {
+      continue;
+    }
     RefPtr<DeleteNodeTransaction> deleteNodeTransaction =
         DeleteNodeTransaction::MaybeCreate(*mEditorBase, *node->AsContent());
-    // XXX This is odd handling.  Even if some nodes in the range are not
-    //     editable, editor should append transactions because they could
-    //     at undoing/redoing.  Additionally, if the transaction needs to
-    //     delete/restore all nodes, it should at undoing/redoing.
-    if (!deleteNodeTransaction) {
-      NS_WARNING("DeleteNodeTransaction::MaybeCreate() failed");
-      return NS_ERROR_FAILURE;
+    if (deleteNodeTransaction) {
+      DebugOnly<nsresult> rvIgnored = AppendChild(deleteNodeTransaction);
+      NS_WARNING_ASSERTION(
+          NS_SUCCEEDED(rvIgnored),
+          "DeleteRangeTransaction::AppendChild() failed, but ignored");
     }
-    DebugOnly<nsresult> rvIgnored = AppendChild(deleteNodeTransaction);
-    NS_WARNING_ASSERTION(
-        NS_SUCCEEDED(rvIgnored),
-        "DeleteRangeTransaction::AppendChild() failed, but ignored");
   }
   return NS_OK;
 }
 
 }  // namespace mozilla
--- a/editor/libeditor/EditorBase.cpp
+++ b/editor/libeditor/EditorBase.cpp
@@ -3938,16 +3938,20 @@ EditorBase::CreateTransactionForCollapse
       if (!deleteTextTransaction) {
         NS_WARNING(
             "DeleteTextTransaction::MaybeCreateForPreviousCharacter() failed");
         return nullptr;
       }
       return deleteTextTransaction.forget();
     }
 
+    if (IsHTMLEditor() &&
+        NS_WARN_IF(!HTMLEditUtils::IsRemovableNode(*previousEditableContent))) {
+      return nullptr;
+    }
     RefPtr<DeleteNodeTransaction> deleteNodeTransaction =
         DeleteNodeTransaction::MaybeCreate(*this, *previousEditableContent);
     if (!deleteNodeTransaction) {
       NS_WARNING("DeleteNodeTransaction::MaybeCreate() failed");
       return nullptr;
     }
     return deleteNodeTransaction.forget();
   }
@@ -3982,16 +3986,20 @@ EditorBase::CreateTransactionForCollapse
       if (!deleteTextTransaction) {
         NS_WARNING(
             "DeleteTextTransaction::MaybeCreateForNextCharacter() failed");
         return nullptr;
       }
       return deleteTextTransaction.forget();
     }
 
+    if (IsHTMLEditor() &&
+        NS_WARN_IF(!HTMLEditUtils::IsRemovableNode(*nextEditableContent))) {
+      return nullptr;
+    }
     RefPtr<DeleteNodeTransaction> deleteNodeTransaction =
         DeleteNodeTransaction::MaybeCreate(*this, *nextEditableContent);
     if (!deleteNodeTransaction) {
       NS_WARNING("DeleteNodeTransaction::MaybeCreate() failed");
       return nullptr;
     }
     return deleteNodeTransaction.forget();
   }
@@ -4075,16 +4083,19 @@ EditorBase::CreateTransactionForCollapse
             *this, *editableContent->AsText(), 0);
     NS_WARNING_ASSERTION(
         deleteTextTransaction,
         "DeleteTextTransaction::MaybeCreateForNextCharacter() failed");
     return deleteTextTransaction.forget();
   }
 
   MOZ_ASSERT(IsHTMLEditor());
+  if (NS_WARN_IF(!HTMLEditUtils::IsRemovableNode(*editableContent))) {
+    return nullptr;
+  }
   RefPtr<DeleteNodeTransaction> deleteNodeTransaction =
       DeleteNodeTransaction::MaybeCreate(*this, *editableContent);
   NS_WARNING_ASSERTION(deleteNodeTransaction,
                        "DeleteNodeTransaction::MaybeCreate() failed");
   return deleteNodeTransaction.forget();
 }
 
 bool EditorBase::FlushPendingNotificationsIfToHandleDeletionWithFrameSelection(
--- a/editor/libeditor/EditorBase.h
+++ b/editor/libeditor/EditorBase.h
@@ -1718,17 +1718,18 @@ class EditorBase : public nsIEditor,
   [[nodiscard]] MOZ_CAN_RUN_SCRIPT nsresult
   SetTextNodeWithoutTransaction(const nsAString& aString, Text& aTextNode);
 
   /**
    * DeleteNodeWithTransaction() removes aContent from the DOM tree.
    *
    * @param aContent    The node which will be removed form the DOM tree.
    */
-  MOZ_CAN_RUN_SCRIPT nsresult DeleteNodeWithTransaction(nsIContent& aContent);
+  virtual MOZ_CAN_RUN_SCRIPT nsresult
+  DeleteNodeWithTransaction(nsIContent& aContent);
 
   /**
    * InsertNodeWithTransaction() inserts aContentToInsert before the child
    * specified by aPointToInsert.
    *
    * @param aContentToInsert    The node to be inserted.
    * @param aPointToInsert      The insertion point of aContentToInsert.
    *                            If this refers end of the container, the
--- a/editor/libeditor/HTMLEditor.h
+++ b/editor/libeditor/HTMLEditor.h
@@ -750,22 +750,22 @@ class HTMLEditor final : public EditorBa
    *                            <br> node, returns error.
    */
   MOZ_CAN_RUN_SCRIPT Result<RefPtr<Element>, nsresult>
   InsertBRElementWithTransaction(const EditorDOMPoint& aPointToInsert,
                                  EDirection aSelect = eNone);
 
   /**
    * DeleteNodeWithTransaction() removes aContent from the DOM tree if it's
-   * modifiable.  Note that this is not an override of same method of
-   * EditorBase.
+   * modifiable.
    *
    * @param aContent    The node to be removed from the DOM tree.
    */
-  MOZ_CAN_RUN_SCRIPT nsresult DeleteNodeWithTransaction(nsIContent& aContent);
+  MOZ_CAN_RUN_SCRIPT nsresult
+  DeleteNodeWithTransaction(nsIContent& aContent) final;
 
   /**
    * DeleteTextWithTransaction() removes text in the range from aTextNode if
    * it's modifiable.  Note that this not an override of same method of
    * EditorBase.
    *
    * @param aTextNode           The text node which should be modified.
    * @param aOffset             Start offset of removing text in aTextNode.