Bug 1517028 - Make `HTMLEditor::SplitNodeDeepWithTransaction` not try to split non-splittable node and `HTMLEditor::HandleInsertParagraphInHeadingElement` check its result r=m_kato

There are 2 bugs.  One is that `SplitNodeDeepWithTransaction` tries to split
comment node, but it fails.  The other is, the failure result is not checked
by `HandleInsertParagraphInHeadingElement`.  Therefore, the original head
element's previous node may not be a heading element.

Depends on D106591

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

editor/libeditor/HTMLEditSubActionHandler.cpp

@@ -6396,9 +6396,10 @@ nsresult HTMLEditor::HandleInsertParagraphInHeadingElement(Element& aHeader,
   if (NS_WARN_IF(Destroyed())) {
     return NS_ERROR_EDITOR_DESTROYED;
   }
-  NS_WARNING_ASSERTION(
-      splitHeaderResult.Succeeded(),
-      "HTMLEditor::SplitNodeDeepWithTransaction() failed, but ignored");
+  if (splitHeaderResult.Failed()) {
+    NS_WARNING("HTMLEditor::SplitNodeDeepWithTransaction() failed");
+    return NS_ERROR_FAILURE;
+  }
 
   // If the previous heading of split point is empty, put a padding <br>
   // element for empty last line into it.

editor/libeditor/HTMLEditUtils.cpp

@@ -545,14 +545,14 @@ static const ElementInfo kElements[eHTMLTag_userdefined] = {
     ELEM(main, true, true, GROUP_BLOCK, GROUP_FLOW_ELEMENT),
     ELEM(map, true, true, GROUP_SPECIAL, GROUP_BLOCK | GROUP_MAP_CONTENT),
     ELEM(mark, true, true, GROUP_PHRASE, GROUP_INLINE_ELEMENT),
-    ELEM(marquee, false, false, GROUP_NONE, GROUP_NONE),
+    ELEM(marquee, true, false, GROUP_NONE, GROUP_NONE),
     ELEM(menu, true, true, GROUP_BLOCK, GROUP_LI | GROUP_FLOW_ELEMENT),
     ELEM(menuitem, false, false, GROUP_NONE, GROUP_NONE),
     ELEM(meta, false, false, GROUP_HEAD_CONTENT, GROUP_NONE),
     ELEM(meter, true, false, GROUP_SPECIAL, GROUP_FLOW_ELEMENT),
     ELEM(multicol, false, false, GROUP_NONE, GROUP_NONE),
     ELEM(nav, true, true, GROUP_BLOCK, GROUP_FLOW_ELEMENT),
-    ELEM(nobr, false, false, GROUP_NONE, GROUP_NONE),
+    ELEM(nobr, true, false, GROUP_NONE, GROUP_NONE),
     ELEM(noembed, false, false, GROUP_NONE, GROUP_NONE),
     ELEM(noframes, true, true, GROUP_BLOCK, GROUP_FLOW_ELEMENT),
     ELEM(noscript, true, true, GROUP_BLOCK, GROUP_FLOW_ELEMENT),

editor/libeditor/HTMLEditUtils.h

@@ -205,6 +205,19 @@ class HTMLEditUtils final {
     return HTMLEditUtils::IsContainerNode(tagEnum);
   }
 
+  /**
+   * IsSplittableNode() returns true if aContent can split.
+   */
+  static bool IsSplittableNode(const nsIContent& aContent) {
+    if (aContent.IsElement()) {
+      // XXX Perhaps, instead of using container, we should have "splittable"
+      //     information in the DB.  E.g., `<template>`, `<script>` elements
+      //     can have children, but shouldn't be split.
+      return HTMLEditUtils::IsContainerNode(aContent);
+    }
+    return aContent.IsText() && aContent.Length() > 0;
+  }
+
   /**
    * See execCommand spec:
    * https://w3c.github.io/editing/execCommand.html#non-list-single-line-container

editor/libeditor/HTMLEditor.cpp

@@ -4252,22 +4252,31 @@ SplitNodeResult HTMLEditor::SplitNodeDeepWithTransaction(
 
   nsCOMPtr<nsIContent> newLeftNodeOfMostAncestor;
   EditorDOMPoint atStartOfRightNode(aStartOfDeepestRightNode);
+  SplitNodeResult lastSplitNodeResult(atStartOfRightNode);
   while (true) {
     // Need to insert rules code call here to do things like not split a list
     // if you are after the last <li> or before the first, etc.  For now we
     // just have some smarts about unneccessarily splitting text nodes, which
     // should be universal enough to put straight in this EditorBase routine.
-    if (NS_WARN_IF(!atStartOfRightNode.GetContainerAsContent())) {
+    nsIContent* currentRightNode = atStartOfRightNode.GetContainerAsContent();
+    if (NS_WARN_IF(!currentRightNode)) {
       return SplitNodeResult(NS_ERROR_FAILURE);
     }
     // If we meet an orphan node before meeting aMostAncestorToSplit, we need
     // to stop splitting.  This is a bug of the caller.
-    if (NS_WARN_IF(atStartOfRightNode.GetContainer() != &aMostAncestorToSplit &&
+    if (NS_WARN_IF(currentRightNode != &aMostAncestorToSplit &&
                    !atStartOfRightNode.GetContainerParentAsContent())) {
       return SplitNodeResult(NS_ERROR_FAILURE);
     }
-
-    nsIContent* currentRightNode = atStartOfRightNode.GetContainerAsContent();
+    // If the container is not splitable node such as comment node, atomic
+    // element, etc, we should keep it as-is, and try to split its parents.
+    if (!HTMLEditUtils::IsSplittableNode(*currentRightNode)) {
+      if (currentRightNode == &aMostAncestorToSplit) {
+        return lastSplitNodeResult;
+      }
+      atStartOfRightNode.Set(currentRightNode);
+      continue;
+    }
 
     // If the split point is middle of the node or the node is not a text node
     // and we're allowed to create empty element node, split it.
@@ -4283,9 +4292,10 @@ SplitNodeResult HTMLEditor::SplitNodeDeepWithTransaction(
         return SplitNodeResult(error.StealNSResult());
       }
 
+      lastSplitNodeResult = SplitNodeResult(newLeftNode, currentRightNode);
       if (currentRightNode == &aMostAncestorToSplit) {
         // Actually, we split aMostAncestorToSplit.
-        return SplitNodeResult(newLeftNode, &aMostAncestorToSplit);
+        return lastSplitNodeResult;
       }
 
       // Then, try to split its parent before current node.
@@ -4294,8 +4304,9 @@ SplitNodeResult HTMLEditor::SplitNodeDeepWithTransaction(
     // If the split point is end of the node and it is a text node or we're not
     // allowed to create empty container node, try to split its parent after it.
     else if (!atStartOfRightNode.IsStartOfContainer()) {
+      lastSplitNodeResult = SplitNodeResult(currentRightNode, nullptr);
       if (currentRightNode == &aMostAncestorToSplit) {
-        return SplitNodeResult(&aMostAncestorToSplit, nullptr);
+        return lastSplitNodeResult;
       }
 
       // Try to split its parent after current node.
@@ -4307,11 +4318,13 @@ SplitNodeResult HTMLEditor::SplitNodeDeepWithTransaction(
     // If the split point is start of the node and it is a text node or we're
     // not allowed to create empty container node, try to split its parent.
     else {
+      lastSplitNodeResult = SplitNodeResult(nullptr, currentRightNode);
       if (currentRightNode == &aMostAncestorToSplit) {
-        return SplitNodeResult(nullptr, &aMostAncestorToSplit);
+        return lastSplitNodeResult;
       }
 
       // Try to split its parent before current node.
+      lastSplitNodeResult = SplitNodeResult(atStartOfRightNode);
       atStartOfRightNode.Set(currentRightNode);
     }
   }

editor/libeditor/crashtests/1517028.html

@@ -0,0 +1,18 @@
+<!-- COMMENT -->
+abc
+<head>
+<script>
+  function start() {
+    document.execCommand('justifyleft', false, ) 
+    document.designMode = 'on' 
+    document.execCommand('insertparagraph', false, ) 
+  }
+  document.addEventListener('DOMContentLoaded', start)
+</script>  
+</head>
+<h4>
+<em contenteditable="">
+<big>
+<button autofocus formnovalidate formtarget="">
+</button>
+<!-- COMMENT -->

editor/libeditor/crashtests/crashtests.list

@@ -110,7 +110,8 @@ skip-if(Android) needs-focus load 1444630.html
 load 1446451.html
 pref(dom.document.exec_command.nested_calls_allowed,true) asserts(2) load 1464251.html # assertion is that mutation event listener caused by execCommand calls another execCommand
 pref(layout.accessiblecaret.enabled,true) load 1470926.html
-pref(dom.document.exec_command.nested_calls_allowed,true) asserts(1) load 1474978.html # assertion is that mutation event listener caused by execCommand calls another execCommand
+pref(dom.document.exec_command.nested_calls_allowed,true) asserts(2) load 1474978.html # assertion is that mutation event listener caused by execCommand calls another execCommand
+asserts(1) load 1517028.html
 load 1525481.html
 load 1533913.html
 asserts(3) load 1534394.html # assertion in WSRunScanner::GetEditableBlockParentOrTopmotEditableInlineContent()