Bug 1316302 part.1 Helper methods for HTMLEditRules::WillDeleteSelection() should have an out argument to indicates if it actually handles the action r?smaug draft
authorMasayuki Nakano <masayuki@d-toybox.com>
Fri, 18 Nov 2016 17:54:31 +0900
changeset 441063 9efa5e3e3140f490ccd3565e3d4de35a6faf4463
parent 440902 8e476f8bd52d13cc1648e15ea2b72641c2d7bd8a
child 441064 da758179414a8fd40bd9b815f71140e545c75b1d
push id36347
push usermasayuki@d-toybox.com
push dateFri, 18 Nov 2016 10:04:41 +0000
reviewerssmaug
bugs1316302
milestone53.0a1
Bug 1316302 part.1 Helper methods for HTMLEditRules::WillDeleteSelection() should have an out argument to indicates if it actually handles the action r?smaug 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. MozReview-Commit-ID: 7ApUOgtLUog
editor/libeditor/HTMLEditRules.cpp
editor/libeditor/HTMLEditRules.h
--- a/editor/libeditor/HTMLEditRules.cpp
+++ b/editor/libeditor/HTMLEditRules.cpp
@@ -1815,20 +1815,20 @@ HTMLEditRules::WillDeleteSelection(Selec
     rv = mHTMLEditor->DeleteTableCellContents();
     *aHandled = true;
     return rv;
   }
   cell = nullptr;
 
   // 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;
 
   nsCOMPtr<nsINode> selNode;
   int32_t selOffset;
 
   NS_ENSURE_STATE(aSelection->GetRangeAt(0));
@@ -2167,19 +2167,23 @@ HTMLEditRules::WillDeleteSelection(Selec
       nsCOMPtr<nsINode> selPointNode = startNode;
       int32_t selPointOffset = startOffset;
       {
         NS_ENSURE_STATE(mHTMLEditor);
         AutoTrackDOMPoint tracker(mHTMLEditor->mRangeUpdater,
                                   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);
       return NS_OK;
     }
 
     if (wsType == WSType::thisBlock) {
       // At edge of our block.  Look beside it and see if we can join to an
@@ -2218,19 +2222,24 @@ HTMLEditRules::WillDeleteSelection(Selec
 
       nsCOMPtr<nsINode> selPointNode = startNode;
       int32_t selPointOffset = startOffset;
       {
         NS_ENSURE_STATE(mHTMLEditor);
         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);
       return NS_OK;
     }
   }
 
 
@@ -2392,17 +2401,20 @@ HTMLEditRules::WillDeleteSelection(Selec
           NS_ENSURE_STATE(mHTMLEditor);
           OwningNonNull<nsGenericDOMDataNode> dataNode =
             *static_cast<nsGenericDOMDataNode*>(endNode.get());
           rv = mHTMLEditor->DeleteText(dataNode, 0, endOffset);
           NS_ENSURE_SUCCESS(rv, rv);
         }
 
         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);
         }
       }
     }
   }
 
   // We might have left only collapsed whitespace in the start/end nodes
   {
@@ -2542,69 +2554,68 @@ HTMLEditRules::GetGoodSelPointForNode(ns
   NS_ENSURE_TRUE(mHTMLEditor, EditorDOMPoint());
   if ((!aNode.IsHTMLElement(nsGkAtoms::br) ||
        mHTMLEditor->IsVisBreak(&aNode)) && isPreviousAction) {
     ret.offset++;
   }
   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);
 
   nsCOMPtr<Element> leftBlock = htmlEditor->GetBlock(aLeftNode);
   nsCOMPtr<Element> rightBlock = htmlEditor->GetBlock(aRightNode);
 
   // Sanity checks
   NS_ENSURE_TRUE(leftBlock && rightBlock, NS_ERROR_NULL_POINTER);
   NS_ENSURE_STATE(leftBlock != rightBlock);
 
   if (HTMLEditUtils::IsTableElement(leftBlock) ||
       HTMLEditUtils::IsTableElement(rightBlock)) {
     // Do not try to merge table elements
     *aCanceled = true;
+    *aHandled = true;
     return NS_OK;
   }
 
   // Make sure we don't try to move things into HR's, which look like blocks
   // but aren't containers
   if (leftBlock->IsHTMLElement(nsGkAtoms::hr)) {
     leftBlock = htmlEditor->GetBlockNodeParent(leftBlock);
   }
   if (rightBlock->IsHTMLElement(nsGkAtoms::hr)) {
     rightBlock = htmlEditor->GetBlockNodeParent(rightBlock);
   }
   NS_ENSURE_STATE(leftBlock && rightBlock);
 
   // Bail if both blocks the same
   if (leftBlock == rightBlock) {
     *aCanceled = true;
+    *aHandled = true;
     return NS_OK;
   }
 
   // Joining a list item to its parent is a NOP.
   if (HTMLEditUtils::IsList(leftBlock) &&
       HTMLEditUtils::IsListItem(rightBlock) &&
       rightBlock->GetParentNode() == leftBlock) {
+    *aHandled = true;
     return NS_OK;
   }
 
   // Special rule here: if we are trying to join list items, and they are in
   // different lists, join the lists instead.
   bool mergeLists = false;
   nsIAtom* existingList = nsGkAtoms::_empty;
   int32_t offset;
@@ -2665,21 +2676,25 @@ HTMLEditRules::JoinBlocks(nsIContent& aL
     if (mergeLists) {
       // The idea here is to take all children in rightList that are past
       // offset, and pull them into leftlist.
       for (nsCOMPtr<nsIContent> child = rightList->GetChildAt(offset);
            child; child = rightList->GetChildAt(rightOffset)) {
         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);
-    }
-    if (brNode) {
-      htmlEditor->DeleteNode(brNode);
+      bool handled = false;
+      MoveBlock(*leftBlock, *rightBlock, leftOffset, rightOffset, &handled);
+      *aHandled |= handled;
+    }
+    if (brNode && NS_SUCCEEDED(htmlEditor->DeleteNode(brNode))) {
+      *aHandled = true;
     }
   // Offset below is where you find yourself in leftBlock when you traverse
   // upwards from rightBlock
   } else if (EditorUtils::IsDescendantOf(rightBlock, leftBlock, &leftOffset)) {
     // Tricky case.  Right block is inside left block.  Do ws adjustment.  This
     // just destroys non-visible ws at boundaries we will be joining.
     nsresult rv = WSRunObject::ScrubBlockBoundary(htmlEditor,
                                                   WSRunObject::kBlockStart,
@@ -2701,17 +2716,19 @@ HTMLEditRules::JoinBlocks(nsIContent& aL
         NS_ENSURE_STATE(trackingLeftBlock->GetParentElement());
         leftBlock = trackingLeftBlock->GetParentElement();
       }
     }
     // Do br adjustment.
     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
       // want to move.
 
       int32_t previousContentOffset;
       nsCOMPtr<nsINode> previousContentParent;
 
@@ -2757,22 +2774,24 @@ HTMLEditRules::JoinBlocks(nsIContent& aL
           previousContentParent = splittedPreviousContent->GetParentNode();
           previousContentOffset = previousContentParent ?
             previousContentParent->IndexOf(splittedPreviousContent) : -1;
         }
       }
 
       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
     // of the latter is <p>paragraph</p><ul><li>one<li>two<li>three</ul>.  The
     // first li and the p are not true siblings, but we still want to join them
     // if you backspace from li into p.
 
     // Adjust whitespace at block boundaries
@@ -2786,124 +2805,139 @@ HTMLEditRules::JoinBlocks(nsIContent& aL
                       rightBlock->NodeInfo()->NameAtom()) {
       // Nodes are same type.  merge them.
       EditorDOMPoint pt = JoinNodesSmart(*leftBlock, *rightBlock);
       if (pt.node && mergeLists) {
         nsCOMPtr<Element> newBlock;
         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),
                                   EditAction::makeList, arrayOfNodes,
                                   TouchContent::yes);
   NS_ENSURE_SUCCESS(rv, rv);
   for (uint32_t i = 0; i < arrayOfNodes.Length(); i++) {
     // 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;
     }
   }
 
   // XXX We're only checking return value of the last iteration
   NS_ENSURE_SUCCESS(rv, rv);
   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);
 
   // Check if this node can go into the destination node
   if (htmlEditor->CanContain(aDestElement, aNode)) {
     // If it can, move it there
     nsresult rv =
       htmlEditor->MoveNode(&aNode, &aDestElement, *aInOutDestOffset);
     NS_ENSURE_SUCCESS(rv, rv);
     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;
 }
 
 
 nsresult
 HTMLEditRules::DeleteNonTableElements(nsINode* aNode)
--- a/editor/libeditor/HTMLEditRules.h
+++ b/editor/libeditor/HTMLEditRules.h
@@ -158,24 +158,81 @@ protected:
                                nsIEditor::EStripWrappers aStripWrappers,
                                bool* aCancel, bool* aHandled);
   nsresult DidDeleteSelection(Selection* aSelection,
                               nsIEditor::EDirection aDir,
                               nsresult aResult);
   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,
                         bool aEntireList,
                         const nsAString* aBulletType,
                         bool* aCancel, bool* aHandled,
                         const nsAString* aItemType = nullptr);
   nsresult WillRemoveList(Selection* aSelection, bool aOrdered, bool* aCancel,