Bug 1460509 - part 23: Make HTMLEditRules::RemoveListStructure() return NS_ERROR_EDITOR_DESTROYED if it causes destroying the editor r?m_kato draft
authorMasayuki Nakano <masayuki@d-toybox.com>
Mon, 14 May 2018 17:47:21 +0900
changeset 798741 6c3ad9fe0dc90a239f2a322472552457fe861e1b
parent 798740 03d9b44f58d72c3a1613937ea47586c215321442
child 798742 55aececc29764efadc654cb547c325a1a43bbf8c
push id110840
push usermasayuki@d-toybox.com
push dateWed, 23 May 2018 13:41:58 +0000
reviewersm_kato
bugs1460509
milestone62.0a1
Bug 1460509 - part 23: Make HTMLEditRules::RemoveListStructure() return NS_ERROR_EDITOR_DESTROYED if it causes destroying the editor r?m_kato MozReview-Commit-ID: JLfjjtQS6Ar
editor/libeditor/HTMLEditRules.cpp
editor/libeditor/HTMLEditRules.h
--- a/editor/libeditor/HTMLEditRules.cpp
+++ b/editor/libeditor/HTMLEditRules.cpp
@@ -395,17 +395,20 @@ HTMLEditRules::BeforeEdit(EditAction aAc
       return NS_ERROR_FAILURE;
     }
     if (htmlDoc->GetEditingState() == nsIHTMLDocument::eContentEditable) {
       htmlDoc->ChangeContentEditableCount(nullptr, +1);
       mRestoreContentEditableCount = true;
     }
 
     // Check that selection is in subtree defined by body node
-    ConfirmSelectionInBody();
+    nsresult rv = ConfirmSelectionInBody();
+    if (NS_WARN_IF(rv == NS_ERROR_EDITOR_DESTROYED)) {
+      return NS_ERROR_EDITOR_DESTROYED;
+    }
     // Let rules remember the top level action
     mTheAction = aAction;
   }
   return NS_OK;
 }
 
 
 nsresult
@@ -459,17 +462,21 @@ HTMLEditRules::AfterEdit(EditAction aAct
 }
 
 nsresult
 HTMLEditRules::AfterEditInner(EditAction aAction,
                               nsIEditor::EDirection aDirection)
 {
   MOZ_ASSERT(IsEditorDataAvailable());
 
-  ConfirmSelectionInBody();
+  nsresult rv = ConfirmSelectionInBody();
+  if (NS_WARN_IF(rv == NS_ERROR_EDITOR_DESTROYED)) {
+    return NS_ERROR_EDITOR_DESTROYED;
+  }
+  NS_WARNING_ASSERTION(NS_SUCCEEDED(rv), "Failed to normalize Selection");
   if (aAction == EditAction::ignore) {
     return NS_OK;
   }
 
   nsCOMPtr<nsINode> rangeStartContainer, rangeEndContainer;
   uint32_t rangeStartOffset = 0, rangeEndOffset = 0;
   // do we have a real range to act on?
   bool bDamagedRange = false;
@@ -577,25 +584,23 @@ HTMLEditRules::AfterEditInner(EditAction
       rv = ReapplyCachedStyles();
       if (NS_WARN_IF(NS_FAILED(rv))) {
         return rv;
       }
       ClearCachedStyles();
     }
   }
 
-  nsresult rv =
-    HTMLEditorRef().HandleInlineSpellCheck(
-                      aAction, SelectionRef(),
-                      mRangeItem->mStartContainer,
-                      mRangeItem->mStartOffset,
-                      rangeStartContainer,
-                      rangeStartOffset,
-                      rangeEndContainer,
-                      rangeEndOffset);
+  rv = HTMLEditorRef().HandleInlineSpellCheck(aAction, SelectionRef(),
+                                              mRangeItem->mStartContainer,
+                                              mRangeItem->mStartOffset,
+                                              rangeStartContainer,
+                                              rangeStartOffset,
+                                              rangeEndContainer,
+                                              rangeEndOffset);
   if (NS_WARN_IF(NS_FAILED(rv))) {
     return rv;
   }
 
   // detect empty doc
   rv = CreateBogusNodeIfNeeded();
   if (NS_WARN_IF(NS_FAILED(rv))) {
     return rv;
@@ -9307,54 +9312,78 @@ HTMLEditRules::PopListItem(nsIContent& a
     if (aOutOfList) {
       *aOutOfList = true;
     }
   }
   return NS_OK;
 }
 
 nsresult
-HTMLEditRules::RemoveListStructure(Element& aList)
+HTMLEditRules::RemoveListStructure(Element& aListElement)
 {
   MOZ_ASSERT(IsEditorDataAvailable());
-
-  while (aList.GetFirstChild()) {
-    OwningNonNull<nsIContent> child = *aList.GetFirstChild();
+  MOZ_ASSERT(HTMLEditUtils::IsList(&aListElement));
+
+  while (aListElement.GetFirstChild()) {
+    OwningNonNull<nsIContent> child = *aListElement.GetFirstChild();
 
     if (HTMLEditUtils::IsListItem(child)) {
       bool isOutOfList;
       // Keep popping it out until it's not in a list anymore
+      // XXX Using PopuListItem() is too expensive for this purpose.  Looks
+      //     like the reason why this method uses it is, only this loop
+      //     wants to work with first child of aList.  However, what it
+      //     actually does is removing <li> as container.  So, just using
+      //     RemoveBlockContainerWithTransaction() is reasonable.
+      // XXX This loop means that if aListElement is is a child of another
+      //     list element (although it's invalid tree), this moves the
+      //     list item to outside of aListElement's parent.  Is that really
+      //     intentional behavior?
       do {
         nsresult rv = PopListItem(child, &isOutOfList);
-        NS_ENSURE_SUCCESS(rv, rv);
+        if (NS_WARN_IF(NS_FAILED(rv))) {
+          return rv;
+        }
       } while (!isOutOfList);
-    } else if (HTMLEditUtils::IsList(child)) {
+      continue;
+    }
+
+    if (HTMLEditUtils::IsList(child)) {
       nsresult rv = RemoveListStructure(*child->AsElement());
       if (NS_WARN_IF(NS_FAILED(rv))) {
         return rv;
       }
-    } else {
-      // Delete any non-list items for now
-      nsresult rv = HTMLEditorRef().DeleteNodeWithTransaction(*child);
-      if (NS_WARN_IF(NS_FAILED(rv))) {
-        return rv;
-      }
+      continue;
+    }
+
+    // Delete any non-list items for now
+    // XXX This is not HTML5 aware.  HTML5 allows all list elements to have
+    //     <script> and <template> and <dl> element to have <div> to group
+    //     some <dt> and <dd> elements.  So, this may break valid children.
+    nsresult rv = HTMLEditorRef().DeleteNodeWithTransaction(*child);
+    if (NS_WARN_IF(!CanHandleEditAction())) {
+      return NS_ERROR_EDITOR_DESTROYED;
+    }
+    if (NS_WARN_IF(NS_FAILED(rv))) {
+      return rv;
     }
   }
 
   // Delete the now-empty list
-  nsresult rv = HTMLEditorRef().RemoveBlockContainerWithTransaction(aList);
+  nsresult rv =
+    HTMLEditorRef().RemoveBlockContainerWithTransaction(aListElement);
+  if (NS_WARN_IF(!CanHandleEditAction())) {
+    return NS_ERROR_EDITOR_DESTROYED;
+  }
   if (NS_WARN_IF(NS_FAILED(rv))) {
     return rv;
   }
   return NS_OK;
 }
 
-// XXX This method is not necessary because even if selection is outside the
-//     <body> element, the element can be editable.
 nsresult
 HTMLEditRules::ConfirmSelectionInBody()
 {
   MOZ_ASSERT(IsEditorDataAvailable());
 
   Element* rootElement = HTMLEditorRef().GetRoot();
   if (NS_WARN_IF(!rootElement)) {
     return NS_ERROR_UNEXPECTED;
@@ -9372,16 +9401,19 @@ HTMLEditRules::ConfirmSelectionInBody()
   while (temp && !temp->IsHTMLElement(nsGkAtoms::body)) {
     temp = temp->GetParentOrHostNode();
   }
 
   // If we aren't in the <body> element, force the issue.
   if (!temp) {
     IgnoredErrorResult ignoredError;
     SelectionRef().Collapse(RawRangeBoundary(rootElement, 0), ignoredError);
+    if (NS_WARN_IF(!CanHandleEditAction())) {
+      return NS_ERROR_EDITOR_DESTROYED;
+    }
     NS_WARNING_ASSERTION(!ignoredError.Failed(),
       "Failed to collapse selection at start of the root element");
     return NS_OK;
   }
 
   EditorRawDOMPoint selectionEndPoint(EditorBase::GetEndPoint(&SelectionRef()));
   if (NS_WARN_IF(!selectionEndPoint.IsSet())) {
     return NS_ERROR_FAILURE;
@@ -9393,16 +9425,19 @@ HTMLEditRules::ConfirmSelectionInBody()
   while (temp && !temp->IsHTMLElement(nsGkAtoms::body)) {
     temp = temp->GetParentOrHostNode();
   }
 
   // If we aren't in the <body> element, force the issue.
   if (!temp) {
     IgnoredErrorResult ignoredError;
     SelectionRef().Collapse(RawRangeBoundary(rootElement, 0), ignoredError);
+    if (NS_WARN_IF(!CanHandleEditAction())) {
+      return NS_ERROR_EDITOR_DESTROYED;
+    }
     NS_WARNING_ASSERTION(!ignoredError.Failed(),
       "Failed to collapse selection at start of the root element");
   }
 
   return NS_OK;
 }
 
 nsresult
--- a/editor/libeditor/HTMLEditRules.h
+++ b/editor/libeditor/HTMLEditRules.h
@@ -553,17 +553,31 @@ protected:
    * @return            The point at the first child of aRightNode.
    */
   EditorDOMPoint
   JoinNearestEditableNodesWithTransaction(nsIContent& aLeftNode,
                                           nsIContent& aRightNode);
 
   Element* GetTopEnclosingMailCite(nsINode& aNode);
   nsresult PopListItem(nsIContent& aListItem, bool* aOutOfList = nullptr);
-  nsresult RemoveListStructure(Element& aList);
+
+  /**
+   * RemoveListStructure() destroys the list structure of aListElement.
+   * If aListElement has <li>, <dl> or <dt> as a child, the element is removed
+   * but its descendants are moved to where the list item element was.
+   * If aListElement has another <ul>, <ol> or <dl> as a child, this method
+   * is called recursively.
+   * If aListElement has other nodes as its child, they are just removed.
+   * Finally, aListElement is removed. and its all children are moved to
+   * where the aListElement was.
+   *
+   * @param aListElement        A <ul>, <ol> or <dl> element.
+   */
+  MOZ_MUST_USE nsresult RemoveListStructure(Element& aListElement);
+
   nsresult CacheInlineStyles(nsINode* aNode);
   nsresult ReapplyCachedStyles();
   void ClearCachedStyles();
   void AdjustSpecialBreaks();
   nsresult AdjustWhitespace();
   nsresult PinSelectionToNewBlock();
   void CheckInterlinePosition();
   nsresult AdjustSelection(nsIEditor::EDirection aAction);
@@ -590,17 +604,27 @@ protected:
    * element" here includes not just <table> but also <td>, <tbody>, <tr>, etc.
    * The nodes count as being their own descendants for this purpose, so a
    * table element is its own nearest table element ancestor.
    */
   bool InDifferentTableElements(nsINode* aNode1, nsINode* aNode2);
   nsresult RemoveEmptyNodes();
   nsresult SelectionEndpointInNode(nsINode* aNode, bool* aResult);
   nsresult UpdateDocChangeRange(nsRange* aRange);
-  nsresult ConfirmSelectionInBody();
+
+  /**
+   * ConfirmSelectionInBody() makes sure that Selection is in editor root
+   * element typically <body> element (see HTMLEditor::UpdateRootElement())
+   * and only one Selection range.
+   * XXX This method is not necessary because even if selection is outside the
+   *     <body> element, elements outside the <body> element should be
+   *     editable, e.g., any element can be inserted siblings as <body> element
+   *     and other browsers allow to edit such elements.
+   */
+  MOZ_MUST_USE nsresult ConfirmSelectionInBody();
 
   bool IsEmptyInline(nsINode& aNode);
   bool ListIsEmptyLine(nsTArray<OwningNonNull<nsINode>>& arrayOfNodes);
 
   /**
    * RemoveAlignment() removes align attributes, text-align properties and
    * <center> elements in aNode.
    *