Bug 1673816 - DeleteNodeWithTransaction should allow non-editable node which parent is editable. r=masayuki default tip
authorMakoto Kato <m_kato@ga2.so-net.ne.jp>
Fri, 30 Oct 2020 01:38:41 +0000
changeset 555185 81249476fb796048253b9788b1a1b803de0242c9
parent 555184 0ac2bc30a33b33ef9ea81910f4a7430687baa1f8
push id129810
push userm_kato@ga2.so-net.ne.jp
push dateFri, 30 Oct 2020 04:15:00 +0000
treeherderautoland@81249476fb79 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmasayuki
bugs1673816
milestone84.0a1
Bug 1673816 - DeleteNodeWithTransaction should allow non-editable node which parent is editable. r=masayuki Actually, `DeleteNodeWithTransaction` cannot remove non-editable node. But we should allow it which parent node is ediatble. Differential Revision: https://phabricator.services.mozilla.com/D94937
editor/libeditor/HTMLEditUtils.h
editor/libeditor/HTMLEditor.cpp
editor/libeditor/WSRunObject.cpp
testing/web-platform/tests/editing/data/delete.js
testing/web-platform/tests/editing/data/forwarddelete.js
--- a/editor/libeditor/HTMLEditUtils.h
+++ b/editor/libeditor/HTMLEditUtils.h
@@ -38,16 +38,24 @@ class HTMLEditUtils final {
    * IsSimplyEditableNode() returns true when aNode is simply editable.
    * This does NOT means that aNode can be removed from current parent nor
    * aNode's data is editable.
    */
   static bool IsSimplyEditableNode(const nsINode& aNode) {
     return aNode.IsEditable();
   }
 
+  /*
+   * IsRemovalNode() returns true when parent of aContent is editable even
+   * if aContent isn't editable.
+   */
+  static bool IsRemovableNode(const nsIContent& aContent) {
+    return aContent.GetParentNode() && aContent.GetParentNode()->IsEditable();
+  }
+
   /**
    * IsRemovableFromParentNode() returns true when aContent is editable, has a
    * parent node and the parent node is also editable.
    */
   static bool IsRemovableFromParentNode(const nsIContent& aContent) {
     return aContent.IsEditable() && aContent.GetParentNode() &&
            aContent.GetParentNode()->IsEditable();
   }
--- a/editor/libeditor/HTMLEditor.cpp
+++ b/editor/libeditor/HTMLEditor.cpp
@@ -3147,17 +3147,17 @@ nsresult HTMLEditor::RemoveEmptyInclusiv
                        "HTMLEditor::DeleteNodeWithTransaction() failed");
   return rv;
 }
 
 nsresult HTMLEditor::DeleteNodeWithTransaction(nsIContent& aContent) {
   // Do nothing if the node is read-only.
   // XXX This is not a override method of EditorBase's method.  This might
   //     cause not called accidentally.  We need to investigate this issue.
-  if (NS_WARN_IF(!HTMLEditUtils::IsSimplyEditableNode(aContent) &&
+  if (NS_WARN_IF(!HTMLEditUtils::IsRemovableNode(aContent) &&
                  !EditorUtils::IsPaddingBRElementForEmptyEditor(aContent))) {
     return NS_ERROR_FAILURE;
   }
   nsresult rv = EditorBase::DeleteNodeWithTransaction(aContent);
   NS_WARNING_ASSERTION(NS_SUCCEEDED(rv),
                        "EditorBase::DeleteNodeWithTransaction() failed");
   return rv;
 }
--- a/editor/libeditor/WSRunObject.cpp
+++ b/editor/libeditor/WSRunObject.cpp
@@ -1152,16 +1152,20 @@ nsresult WhiteSpaceVisibilityKeeper::Del
 nsresult WhiteSpaceVisibilityKeeper::DeleteContentNodeAndJoinTextNodesAroundIt(
     HTMLEditor& aHTMLEditor, nsIContent& aContentToDelete,
     const EditorDOMPoint& aCaretPoint) {
   EditorDOMPoint atContent(&aContentToDelete);
   if (!atContent.IsSet()) {
     NS_WARNING("Deleting content node was an orphan node");
     return NS_ERROR_FAILURE;
   }
+  if (!HTMLEditUtils::IsRemovableNode(aContentToDelete)) {
+    NS_WARNING("Deleting content node wasn't removable");
+    return NS_ERROR_FAILURE;
+  }
   nsresult rv = WhiteSpaceVisibilityKeeper::
       MakeSureToKeepVisibleStateOfWhiteSpacesAroundDeletingRange(
           aHTMLEditor, EditorDOMRange(atContent, atContent.NextPoint()));
   if (NS_FAILED(rv)) {
     NS_WARNING(
         "WhiteSpaceVisibilityKeeper::"
         "MakeSureToKeepVisibleStateOfWhiteSpacesAroundDeletingRange() failed");
     return rv;
--- a/testing/web-platform/tests/editing/data/delete.js
+++ b/testing/web-platform/tests/editing/data/delete.js
@@ -2550,9 +2550,14 @@ var browserTests = [
     "<div>abcdef</div>",
     [true],
     {"delete":[false,false,"",false,false,""]}],
 ["<div>abc  </div> <div>  []def</div>",
     [["delete",""]],
     "<div>abcdef</div>",
     [true],
     {"delete":[false,false,"",false,false,""]}],
+["foo<img contenteditable=false src=/img/lion.svg>[]bar",
+    [["delete",""]],
+    "foo{}bar",
+    [true],
+    {"delete":[false,false,"",false,false,""]}],
 ]
--- a/testing/web-platform/tests/editing/data/forwarddelete.js
+++ b/testing/web-platform/tests/editing/data/forwarddelete.js
@@ -2435,9 +2435,14 @@ var browserTests = [
     "<div>abcdef</div>",
     [true],
     {"forwarddelete":[false,false,"",false,false,""]}],
 ["<div>abc[]  </div> <div>  def</div>",
     [["forwarddelete",""]],
     "<div>abcdef</div>",
     [true],
     {"forwarddelete":[false,false,"",false,false,""]}],
+["foo[]<img contenteditable=false src=/img/lion.svg>bar",
+    [["forwarddelete",""]],
+    "foo{}bar",
+    [true],
+    {"forwarddelete":[false,false,"",false,false,""]}],
 ]