Bug 1316302 - Part 1: Helper methods for HTMLEditRules::WillDeleteSelection() should have an out argument to indicates if it actually handles the action

When HTMLEditRules::WillDeleteSelection() tries to remove something from the end/start of a block to its last/first text node but it's contained by block elements, it tries to join the container and the block. However, JoinBlocks() always fails to join them since it's impossible operation. In this case, HTMLEditRules::WillDeleteSelection() should retry to remove something in the leaf, however, it's impossible for now because JoinBlocks() and its helper methods don't return if it handles the action actually.

This patch renames |JoinBlocks()| to |TryToJoinBlocks()| for representing what it is. And this patch adds |bool* aHandled| to the helper methods. Then, *aHandled and *aCancel are now always returns the result of each method. Therefore, for merging the result of multiple helper methods, callers need to receive the result with temporary variables and merge them by themselves.

Note that when they modify DOM node actually or the action should do nothing (for example, selection is across tables), aHandled is set to true.

Tag #1563

2 files changed, 149 insertions, 59 deletions

diff --git a/editor/libeditor/HTMLEditRules.cpp b/editor/libeditor/HTMLEditRules.cpp
index 805092eb7..5cd370f13 100644
--- a/editor/libeditor/HTMLEditRules.cpp
+++ b/editor/libeditor/HTMLEditRules.cpp
@@ -1844,10 +1844,10 @@ HTMLEditRules::WillDeleteSelection(Selection* aSelection,
 
   // origCollapsed is used later to determine whether we should join blocks. We
   // don't really care about bCollapsed because it will be modified by
-  // ExtendSelectionForDelete later. JoinBlocks should happen if the original
-  // selection is collapsed and the cursor is at the end of a block element, in
-  // which case ExtendSelectionForDelete would always make the selection not
-  // collapsed.
+  // ExtendSelectionForDelete later. TryToJoinBlocks() should happen if the
+  // original selection is collapsed and the cursor is at the end of a block
+  // element, in which case ExtendSelectionForDelete would always make the
+  // selection not collapsed.
   bool bCollapsed = aSelection->Collapsed();
   bool join = false;
   bool origCollapsed = bCollapsed;
@@ -2196,9 +2196,13 @@ HTMLEditRules::WillDeleteSelection(Selection* aSelection,
                                   address_of(selPointNode), &selPointOffset);
         NS_ENSURE_STATE(leftNode && leftNode->IsContent() &&
                         rightNode && rightNode->IsContent());
+        bool handled = false, canceled = false;
+        rv = TryToJoinBlocks(*leftNode->AsContent(), *rightNode->AsContent(),
+                             &canceled, &handled);
+        // TODO: If it does nothing and previous or next node is a text node,
+        //       we should modify it.
         *aHandled = true;
-        rv = JoinBlocks(*leftNode->AsContent(), *rightNode->AsContent(),
-                        aCancel);
+        *aCancel |= canceled;
         NS_ENSURE_SUCCESS(rv, rv);
       }
       aSelection->Collapse(selPointNode, selPointOffset);
@@ -2247,9 +2251,14 @@ HTMLEditRules::WillDeleteSelection(Selection* aSelection,
         AutoTrackDOMPoint tracker(mHTMLEditor->mRangeUpdater,
                                   address_of(selPointNode), &selPointOffset);
         NS_ENSURE_STATE(leftNode->IsContent() && rightNode->IsContent());
+        bool handled = false, canceled = false;
+        rv = TryToJoinBlocks(*leftNode->AsContent(), *rightNode->AsContent(),
+                             &canceled, &handled);
+        // This should claim that trying to join the block means that
+        // this handles the action because the caller shouldn't do anything
+        // anymore in this case.
         *aHandled = true;
-        rv = JoinBlocks(*leftNode->AsContent(), *rightNode->AsContent(),
-                        aCancel);
+        *aCancel |= canceled;
         NS_ENSURE_SUCCESS(rv, rv);
       }
       aSelection->Collapse(selPointNode, selPointOffset);
@@ -2421,7 +2430,10 @@ HTMLEditRules::WillDeleteSelection(Selection* aSelection,
         }
 
         if (join) {
-          rv = JoinBlocks(*leftParent, *rightParent, aCancel);
+          bool handled = false, canceled = false;
+          rv = TryToJoinBlocks(*leftParent, *rightParent, &canceled, &handled);
+          MOZ_ASSERT(*aHandled);
+          *aCancel |= canceled;
           NS_ENSURE_SUCCESS(rv, rv);
         }
       }
@@ -2571,21 +2583,17 @@ HTMLEditRules::GetGoodSelPointForNode(nsINode& aNode,
   return ret;
 }
 
-
-/**
- * This method is used to join two block elements.  The right element is always
- * joined to the left element.  If the elements are the same type and not
- * nested within each other, JoinNodesSmart is called (example, joining two
- * list items together into one).  If the elements are not the same type, or
- * one is a descendant of the other, we instead destroy the right block placing
- * its children into leftblock.  DTD containment rules are followed throughout.
- */
 nsresult
-HTMLEditRules::JoinBlocks(nsIContent& aLeftNode,
-                          nsIContent& aRightNode,
-                          bool* aCanceled)
+HTMLEditRules::TryToJoinBlocks(nsIContent& aLeftNode,
+                               nsIContent& aRightNode,
+                               bool* aCanceled,
+                               bool* aHandled)
 {
   MOZ_ASSERT(aCanceled);
+  MOZ_ASSERT(aHandled);
+
+  *aCanceled = false;
+  *aHandled = false;
 
   NS_ENSURE_STATE(mHTMLEditor);
   RefPtr<HTMLEditor> htmlEditor(mHTMLEditor);
@@ -2601,6 +2609,7 @@ HTMLEditRules::JoinBlocks(nsIContent& aLeftNode,
       HTMLEditUtils::IsTableElement(rightBlock)) {
     // Do not try to merge table elements
     *aCanceled = true;
+    *aHandled = true;
     return NS_OK;
   }
 
@@ -2617,6 +2626,7 @@ HTMLEditRules::JoinBlocks(nsIContent& aLeftNode,
   // Bail if both blocks the same
   if (leftBlock == rightBlock) {
     *aCanceled = true;
+    *aHandled = true;
     return NS_OK;
   }
 
@@ -2624,6 +2634,7 @@ HTMLEditRules::JoinBlocks(nsIContent& aLeftNode,
   if (HTMLEditUtils::IsList(leftBlock) &&
       HTMLEditUtils::IsListItem(rightBlock) &&
       rightBlock->GetParentNode() == leftBlock) {
+    *aHandled = true;
     return NS_OK;
   }
 
@@ -2694,11 +2705,15 @@ HTMLEditRules::JoinBlocks(nsIContent& aLeftNode,
         rv = htmlEditor->MoveNode(child, leftList, -1);
         NS_ENSURE_SUCCESS(rv, rv);
       }
+      // XXX Should this set to true only when above for loop moves the node?
+      *aHandled = true;
     } else {
-      MoveBlock(*leftBlock, *rightBlock, leftOffset, rightOffset);
+      bool handled = false;
+      MoveBlock(*leftBlock, *rightBlock, leftOffset, rightOffset, &handled);
+      *aHandled |= handled;
     }
-    if (brNode) {
-      htmlEditor->DeleteNode(brNode);
+    if (brNode && NS_SUCCEEDED(htmlEditor->DeleteNode(brNode))) {
+      *aHandled = true;
     }
   // Offset below is where you find yourself in leftBlock when you traverse
   // upwards from rightBlock
@@ -2730,7 +2745,9 @@ HTMLEditRules::JoinBlocks(nsIContent& aLeftNode,
     nsCOMPtr<Element> brNode =
       CheckForInvisibleBR(*leftBlock, BRLocation::beforeBlock, leftOffset);
     if (mergeLists) {
-      MoveContents(*rightList, *leftList, &leftOffset);
+      bool handled = false;
+      MoveContents(*rightList, *leftList, &leftOffset, &handled);
+      *aHandled |= handled;
     } else {
       // Left block is a parent of right block, and the parent of the previous
       // visible content.  Right block is a child and contains the contents we
@@ -2786,12 +2803,14 @@ HTMLEditRules::JoinBlocks(nsIContent& aLeftNode,
 
       NS_ENSURE_TRUE(previousContentParent, NS_ERROR_NULL_POINTER);
 
+      bool handled = false;
       rv = MoveBlock(*previousContentParent->AsElement(), *rightBlock,
-                     previousContentOffset, rightOffset);
+                     previousContentOffset, rightOffset, &handled);
+      *aHandled |= handled;
       NS_ENSURE_SUCCESS(rv, rv);
     }
-    if (brNode) {
-      htmlEditor->DeleteNode(brNode);
+    if (brNode && NS_SUCCEEDED(htmlEditor->DeleteNode(brNode))) {
+      *aHandled = true;
     }
   } else {
     // Normal case.  Blocks are siblings, or at least close enough.  An example
@@ -2815,32 +2834,37 @@ HTMLEditRules::JoinBlocks(nsIContent& aLeftNode,
         ConvertListType(rightBlock, getter_AddRefs(newBlock),
                         existingList, nsGkAtoms::li);
       }
+      *aHandled = true;
     } else {
       // Nodes are dissimilar types.
-      rv = MoveBlock(*leftBlock, *rightBlock, leftOffset, rightOffset);
+      bool handled = false;
+      rv =
+        MoveBlock(*leftBlock, *rightBlock, leftOffset, rightOffset, &handled);
+      *aHandled |= handled;
       NS_ENSURE_SUCCESS(rv, rv);
     }
     if (brNode) {
       rv = htmlEditor->DeleteNode(brNode);
+      // XXX In other top level if/else-if blocks, the result of DeleteNode()
+      //     is ignored.  Why does only this result is respected?
       NS_ENSURE_SUCCESS(rv, rv);
+      *aHandled = true;
     }
   }
   return NS_OK;
 }
 
-
-/**
- * Moves the content from aRightBlock starting from aRightOffset into
- * aLeftBlock at aLeftOffset. Note that the "block" might merely be inline
- * nodes between <br>s, or between blocks, etc.  DTD containment rules are
- * followed throughout.
- */
 nsresult
 HTMLEditRules::MoveBlock(Element& aLeftBlock,
                          Element& aRightBlock,
                          int32_t aLeftOffset,
-                         int32_t aRightOffset)
+                         int32_t aRightOffset,
+                         bool* aHandled)
 {
+  MOZ_ASSERT(aHandled);
+
+  *aHandled = false;
+
   nsTArray<OwningNonNull<nsINode>> arrayOfNodes;
   // GetNodesFromPoint is the workhorse that figures out what we wnat to move.
   nsresult rv = GetNodesFromPoint(EditorDOMPoint(&aRightBlock, aRightOffset),
@@ -2851,15 +2875,20 @@ HTMLEditRules::MoveBlock(Element& aLeftBlock,
     // get the node to act on
     if (IsBlockNode(arrayOfNodes[i])) {
       // For block nodes, move their contents only, then delete block.
+      bool handled = false;
       rv = MoveContents(*arrayOfNodes[i]->AsElement(), aLeftBlock,
-                        &aLeftOffset);
+                        &aLeftOffset, &handled);
+      *aHandled |= handled;
       NS_ENSURE_SUCCESS(rv, rv);
       NS_ENSURE_STATE(mHTMLEditor);
       rv = mHTMLEditor->DeleteNode(arrayOfNodes[i]);
+      *aHandled = true;
     } else {
       // Otherwise move the content as is, checking against the DTD.
+      bool handled = false;
       rv = MoveNodeSmart(*arrayOfNodes[i]->AsContent(), aLeftBlock,
-                         &aLeftOffset);
+                         &aLeftOffset, &handled);
+      *aHandled |= handled;
     }
   }
 
@@ -2868,17 +2897,16 @@ HTMLEditRules::MoveBlock(Element& aLeftBlock,
   return NS_OK;
 }
 
-/**
- * This method is used to move node aNode to (aDestElement, aInOutDestOffset).
- * DTD containment rules are followed throughout.  aInOutDestOffset is updated
- * to point _after_ inserted content.
- */
 nsresult
 HTMLEditRules::MoveNodeSmart(nsIContent& aNode,
                              Element& aDestElement,
-                             int32_t* aInOutDestOffset)
+                             int32_t* aInOutDestOffset,
+                             bool* aHandled)
 {
   MOZ_ASSERT(aInOutDestOffset);
+  MOZ_ASSERT(aHandled);
+
+  *aHandled = false;
 
   NS_ENSURE_STATE(mHTMLEditor);
   RefPtr<HTMLEditor> htmlEditor(mHTMLEditor);
@@ -2892,37 +2920,43 @@ HTMLEditRules::MoveNodeSmart(nsIContent& aNode,
     if (*aInOutDestOffset != -1) {
       (*aInOutDestOffset)++;
     }
+    // XXX Should we check if the node is actually moved in this case?
+    *aHandled = true;
   } else {
     // If it can't, move its children (if any), and then delete it.
     if (aNode.IsElement()) {
-      nsresult rv =
-        MoveContents(*aNode.AsElement(), aDestElement, aInOutDestOffset);
+      bool handled = false;
+      nsresult rv = MoveContents(*aNode.AsElement(), aDestElement,
+                                 aInOutDestOffset, &handled);
+      *aHandled |= handled;
       NS_ENSURE_SUCCESS(rv, rv);
     }
 
     nsresult rv = htmlEditor->DeleteNode(&aNode);
     NS_ENSURE_SUCCESS(rv, rv);
+    *aHandled = true;
   }
   return NS_OK;
 }
 
-/**
- * Moves the _contents_ of aElement to (aDestElement, aInOutDestOffset).  DTD
- * containment rules are followed throughout.  aInOutDestOffset is updated to
- * point _after_ inserted content.
- */
 nsresult
 HTMLEditRules::MoveContents(Element& aElement,
                             Element& aDestElement,
-                            int32_t* aInOutDestOffset)
+                            int32_t* aInOutDestOffset,
+                            bool* aHandled)
 {
   MOZ_ASSERT(aInOutDestOffset);
+  MOZ_ASSERT(aHandled);
+
+  *aHandled = false;
 
   NS_ENSURE_TRUE(&aElement != &aDestElement, NS_ERROR_ILLEGAL_VALUE);
 
   while (aElement.GetFirstChild()) {
+    bool handled = false;
     nsresult rv = MoveNodeSmart(*aElement.GetFirstChild(), aDestElement,
-                                aInOutDestOffset);
+                                aInOutDestOffset, &handled);
+    *aHandled |= handled;
     NS_ENSURE_SUCCESS(rv, rv);
   }
   return NS_OK;
diff --git a/editor/libeditor/HTMLEditRules.h b/editor/libeditor/HTMLEditRules.h
index 40c5e2afd..6cdfa57cf 100644
--- a/editor/libeditor/HTMLEditRules.h
+++ b/editor/libeditor/HTMLEditRules.h
@@ -163,14 +163,70 @@ protected:
   nsresult InsertBRIfNeeded(Selection* aSelection);
   mozilla::EditorDOMPoint GetGoodSelPointForNode(nsINode& aNode,
                                                  nsIEditor::EDirection aAction);
-  nsresult JoinBlocks(nsIContent& aLeftNode, nsIContent& aRightNode,
-                      bool* aCanceled);
+
+  /**
+   * TryToJoinBlocks() tries to join two block elements.  The right element is
+   * always joined to the left element.  If the elements are the same type and
+   * not nested within each other, JoinNodesSmart() is called (example, joining
+   * two list items together into one).  If the elements are not the same type,
+   * or one is a descendant of the other, we instead destroy the right block
+   * placing its children into leftblock.  DTD containment rules are followed
+   * throughout.
+   *
+   * @param aCanceled   returns true if the operation should do nothing anymore
+   *                    even if this doesn't join the blocks.
+   *                    NOTE: When this returns an error, nobody should refer
+   *                          the result of this.
+   * @param aHandled    returns true if this actually handles the request.
+   *                    Note that this may return true even if this does not
+   *                    join the block.  E.g., if the blocks shouldn't be
+   *                    joined or it's impossible to join them but it's not
+   *                    unexpected case, this returns true with this.
+   *                    NOTE: When this returns an error, nobody should refer
+   *                          the result of this.
+   */
+  nsresult TryToJoinBlocks(nsIContent& aLeftNode, nsIContent& aRightNode,
+                           bool* aCanceled, bool* aHandled);
+
+  /**
+   * MoveBlock() moves the content from aRightBlock starting from aRightOffset
+   * into aLeftBlock at aLeftOffset. Note that the "block" can be inline nodes
+   * between <br>s, or between blocks, etc.  DTD containment rules are followed
+   * throughout.
+   *
+   * @param aHandled    returns true if this actually joins the nodes.
+   *                    NOTE: When this returns an error, nobody should refer
+   *                          the result of this.
+   */
   nsresult MoveBlock(Element& aLeftBlock, Element& aRightBlock,
-                     int32_t aLeftOffset, int32_t aRightOffset);
+                     int32_t aLeftOffset, int32_t aRightOffset,
+                     bool* aHandled);
+
+  /**
+   * MoveNodeSmart() moves aNode to (aDestElement, aInOutDestOffset).
+   * DTD containment rules are followed throughout.
+   *
+   * @param aOffset                 returns the point after inserted content.
+   * @param aHandled                returns true if this actually moves the
+   *                                nodes.
+   *                                NOTE: When this returns an error, nobody
+   *                                      should refer the result of this.
+   */
   nsresult MoveNodeSmart(nsIContent& aNode, Element& aDestElement,
-                         int32_t* aOffset);
+                         int32_t* aInOutDestOffset, bool* aHandled);
+
+  /**
+   * MoveContents() moves the contents of aElement to (aDestElement,
+   * aInOutDestOffset).  DTD containment rules are followed throughout.
+   *
+   * @param aInOutDestOffset        updated to point after inserted content.
+   * @param aHandled                returns true if this actually moves the
+   *                                nodes.
+   *                                NOTE: When this returns an error, nobody
+   *                                      should refer the result of this.
+   */
   nsresult MoveContents(Element& aElement, Element& aDestElement,
-                        int32_t* aOffset);
+                        int32_t* aInOutDestOffset, bool* aHandled);
   nsresult DeleteNonTableElements(nsINode* aNode);
   nsresult WillMakeList(Selection* aSelection,
                         const nsAString* aListType,