Bug 1316302 part.4 Refine HTMLEditRules::TryToJoinBlocks() and HTMLEditRules::MoveNodeSmart() with early return style r?smaug draft
authorMasayuki Nakano <masayuki@d-toybox.com>
Wed, 16 Nov 2016 11:13:40 +0900
changeset 439604 3f709be90884ffa67f20812e90bd9c1e83e5a5a3
parent 439603 e948cc22afe1be80cb4e0e1076d0dd6546f04f59
child 439605 e541e365f1426b5547fade323fd36c5f3cbf9984
push id36058
push usermasayuki@d-toybox.com
push dateWed, 16 Nov 2016 11:23:22 +0000
reviewerssmaug
bugs1316302
milestone53.0a1
Bug 1316302 part.4 Refine HTMLEditRules::TryToJoinBlocks() and HTMLEditRules::MoveNodeSmart() with early return style r?smaug MozReview-Commit-ID: 9vDoU9bUdVO
editor/libeditor/HTMLEditRules.cpp
--- a/editor/libeditor/HTMLEditRules.cpp
+++ b/editor/libeditor/HTMLEditRules.cpp
@@ -2692,30 +2692,36 @@ HTMLEditRules::TryToJoinBlocks(nsIConten
            child; child = rightList->GetChildAt(rightOffset)) {
         rv = htmlEditor->MoveNode(child, leftList, -1);
         if (NS_WARN_IF(NS_FAILED(rv))) {
           return EditActionIgnored(rv);
         }
       }
       // XXX Should this set to true only when above for loop moves the node?
       ret.MarkAsHandled();
-    } else {
-      // XXX Why do we ignore the result of MoveBlock()?
-      EditActionResult retMoveBlock =
-        MoveBlock(*leftBlock, *rightBlock, leftOffset, rightOffset);
-      if (retMoveBlock.Handled()) {
-        ret.MarkAsHandled();
-      }
+      if (brNode) {
+        htmlEditor->DeleteNode(brNode);
+      }
+      return ret;
+    }
+    // XXX Why do we ignore the result of MoveBlock()?
+    EditActionResult retMoveBlock =
+      MoveBlock(*leftBlock, *rightBlock, leftOffset, rightOffset);
+    if (retMoveBlock.Handled()) {
+      ret.MarkAsHandled();
     }
     if (brNode && NS_SUCCEEDED(htmlEditor->DeleteNode(brNode))) {
       ret.MarkAsHandled();
     }
+    return ret;
+  }
+
   // Offset below is where you find yourself in leftBlock when you traverse
   // upwards from rightBlock
-  } else if (EditorUtils::IsDescendantOf(rightBlock, leftBlock, &leftOffset)) {
+  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,
                                                   rightBlock);
     if (NS_WARN_IF(NS_FAILED(rv))) {
       return EditActionIgnored(rv);
     }
@@ -2747,125 +2753,130 @@ HTMLEditRules::TryToJoinBlocks(nsIConten
       CheckForInvisibleBR(*leftBlock, BRLocation::beforeBlock, leftOffset);
     if (mergeLists) {
       // XXX Why do we ignore the result of MoveContents()?
       EditActionResult retMoveContents =
         MoveContents(*rightList, *leftList, &leftOffset);
       if (retMoveContents.Handled()) {
         ret.MarkAsHandled();
       }
+      if (brNode && NS_SUCCEEDED(htmlEditor->DeleteNode(brNode))) {
+        ret.MarkAsHandled();
+      }
+      return ret;
+    }
+
+    // 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;
+
+    if (&aLeftNode == leftBlock) {
+      // We are working with valid HTML, aLeftNode is a block node, and is
+      // therefore allowed to contain rightBlock.  This is the simple case,
+      // we will simply move the content in rightBlock out of its block.
+      previousContentParent = leftBlock;
+      previousContentOffset = leftOffset;
     } 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;
-
-      if (&aLeftNode == leftBlock) {
-        // We are working with valid HTML, aLeftNode is a block node, and is
-        // therefore allowed to contain rightBlock.  This is the simple case,
-        // we will simply move the content in rightBlock out of its block.
-        previousContentParent = leftBlock;
-        previousContentOffset = leftOffset;
-      } else {
-        // We try to work as well as possible with HTML that's already invalid.
-        // Although "right block" is a block, and a block must not be contained
-        // in inline elements, reality is that broken documents do exist.  The
-        // DIRECT parent of "left NODE" might be an inline element.  Previous
-        // versions of this code skipped inline parents until the first block
-        // parent was found (and used "left block" as the destination).
-        // However, in some situations this strategy moves the content to an
-        // unexpected position.  (see bug 200416) The new idea is to make the
-        // moving content a sibling, next to the previous visible content.
-
-        previousContentParent = aLeftNode.GetParentNode();
+      // We try to work as well as possible with HTML that's already invalid.
+      // Although "right block" is a block, and a block must not be contained
+      // in inline elements, reality is that broken documents do exist.  The
+      // DIRECT parent of "left NODE" might be an inline element.  Previous
+      // versions of this code skipped inline parents until the first block
+      // parent was found (and used "left block" as the destination).
+      // However, in some situations this strategy moves the content to an
+      // unexpected position.  (see bug 200416) The new idea is to make the
+      // moving content a sibling, next to the previous visible content.
+
+      previousContentParent = aLeftNode.GetParentNode();
+      previousContentOffset = previousContentParent ?
+        previousContentParent->IndexOf(&aLeftNode) : -1;
+
+      // We want to move our content just after the previous visible node.
+      previousContentOffset++;
+    }
+
+    // Because we don't want the moving content to receive the style of the
+    // previous content, we split the previous content's style.
+
+    nsCOMPtr<Element> editorRoot = htmlEditor->GetEditorRoot();
+    if (!editorRoot || &aLeftNode != editorRoot) {
+      nsCOMPtr<nsIContent> splittedPreviousContent;
+      rv = htmlEditor->SplitStyleAbovePoint(
+                         address_of(previousContentParent),
+                         &previousContentOffset,
+                         nullptr, nullptr, nullptr,
+                         getter_AddRefs(splittedPreviousContent));
+      if (NS_WARN_IF(NS_FAILED(rv))) {
+        return EditActionIgnored(rv);
+      }
+
+      if (splittedPreviousContent) {
+        previousContentParent = splittedPreviousContent->GetParentNode();
         previousContentOffset = previousContentParent ?
-          previousContentParent->IndexOf(&aLeftNode) : -1;
-
-        // We want to move our content just after the previous visible node.
-        previousContentOffset++;
-      }
-
-      // Because we don't want the moving content to receive the style of the
-      // previous content, we split the previous content's style.
-
-      nsCOMPtr<Element> editorRoot = htmlEditor->GetEditorRoot();
-      if (!editorRoot || &aLeftNode != editorRoot) {
-        nsCOMPtr<nsIContent> splittedPreviousContent;
-        rv = htmlEditor->SplitStyleAbovePoint(
-                           address_of(previousContentParent),
-                           &previousContentOffset,
-                           nullptr, nullptr, nullptr,
-                           getter_AddRefs(splittedPreviousContent));
-        if (NS_WARN_IF(NS_FAILED(rv))) {
-          return EditActionIgnored(rv);
-        }
-
-        if (splittedPreviousContent) {
-          previousContentParent = splittedPreviousContent->GetParentNode();
-          previousContentOffset = previousContentParent ?
-            previousContentParent->IndexOf(splittedPreviousContent) : -1;
-        }
-      }
-
-      if (NS_WARN_IF(!previousContentParent)) {
-        return EditActionIgnored(NS_ERROR_NULL_POINTER);
-      }
-
-      ret |= MoveBlock(*previousContentParent->AsElement(), *rightBlock,
-                       previousContentOffset, rightOffset);
-      if (NS_WARN_IF(ret.Failed())) {
-        return ret;
-      }
+          previousContentParent->IndexOf(splittedPreviousContent) : -1;
+      }
+    }
+
+    if (NS_WARN_IF(!previousContentParent)) {
+      return EditActionIgnored(NS_ERROR_NULL_POINTER);
+    }
+
+    ret |= MoveBlock(*previousContentParent->AsElement(), *rightBlock,
+                     previousContentOffset, rightOffset);
+    if (NS_WARN_IF(ret.Failed())) {
+      return ret;
     }
     if (brNode && NS_SUCCEEDED(htmlEditor->DeleteNode(brNode))) {
       ret.MarkAsHandled();
     }
+    return ret;
+  }
+
+  // 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
+  nsresult rv =
+    WSRunObject::PrepareToJoinBlocks(htmlEditor, leftBlock, rightBlock);
+  if (NS_WARN_IF(NS_FAILED(rv))) {
+    return EditActionIgnored(rv);
+  }
+  // Do br adjustment.
+  nsCOMPtr<Element> brNode =
+    CheckForInvisibleBR(*leftBlock, BRLocation::blockEnd);
+  if (mergeLists || leftBlock->NodeInfo()->NameAtom() ==
+                    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);
+    }
+    ret.MarkAsHandled();
   } 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
-    nsresult rv =
-      WSRunObject::PrepareToJoinBlocks(htmlEditor, leftBlock, rightBlock);
+    // Nodes are dissimilar types.
+    ret |= MoveBlock(*leftBlock, *rightBlock, leftOffset, rightOffset);
+    if (NS_WARN_IF(ret.Failed())) {
+      return ret;
+    }
+  }
+  if (brNode) {
+    rv = htmlEditor->DeleteNode(brNode);
+    // XXX In other top level if blocks, the result of DeleteNode()
+    //     is ignored.  Why does only this result is respected?
     if (NS_WARN_IF(NS_FAILED(rv))) {
-      return EditActionIgnored(rv);
-    }
-    // Do br adjustment.
-    nsCOMPtr<Element> brNode =
-      CheckForInvisibleBR(*leftBlock, BRLocation::blockEnd);
-    if (mergeLists || leftBlock->NodeInfo()->NameAtom() ==
-                      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);
-      }
-      ret.MarkAsHandled();
-    } else {
-      // Nodes are dissimilar types.
-      ret |= MoveBlock(*leftBlock, *rightBlock, leftOffset, rightOffset);
-      if (NS_WARN_IF(ret.Failed())) {
-        return ret;
-      }
-    }
-    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?
-      if (NS_WARN_IF(NS_FAILED(rv))) {
-        return ret.SetResult(rv);
-      }
-      ret.MarkAsHandled();
-    }
+      return ret.SetResult(rv);
+    }
+    ret.MarkAsHandled();
   }
   return ret;
 }
 
 EditActionResult
 HTMLEditRules::MoveBlock(Element& aLeftBlock,
                          Element& aRightBlock,
                          int32_t aLeftOffset,
@@ -2931,32 +2942,32 @@ HTMLEditRules::MoveNodeSmart(nsIContent&
     if (NS_WARN_IF(NS_FAILED(rv))) {
       return EditActionIgnored(rv);
     }
     if (*aInOutDestOffset != -1) {
       (*aInOutDestOffset)++;
     }
     // XXX Should we check if the node is actually moved in this case?
     return EditActionHandled();
-  } else {
-    // If it can't, move its children (if any), and then delete it.
-    EditActionResult ret(NS_OK);
-    if (aNode.IsElement()) {
-      ret = MoveContents(*aNode.AsElement(), aDestElement, aInOutDestOffset);
-      if (NS_WARN_IF(ret.Failed())) {
-        return ret;
-      }
-    }
-
-    nsresult rv = htmlEditor->DeleteNode(&aNode);
-    if (NS_WARN_IF(NS_FAILED(rv))) {
-      return ret.SetResult(rv);
-    }
-    return ret.MarkAsHandled();
-  }
+  }
+
+  // If it can't, move its children (if any), and then delete it.
+  EditActionResult ret(NS_OK);
+  if (aNode.IsElement()) {
+    ret = MoveContents(*aNode.AsElement(), aDestElement, aInOutDestOffset);
+    if (NS_WARN_IF(ret.Failed())) {
+      return ret;
+    }
+  }
+
+  nsresult rv = htmlEditor->DeleteNode(&aNode);
+  if (NS_WARN_IF(NS_FAILED(rv))) {
+    return ret.SetResult(rv);
+  }
+  return ret.MarkAsHandled();
 }
 
 EditActionResult
 HTMLEditRules::MoveContents(Element& aElement,
                             Element& aDestElement,
                             int32_t* aInOutDestOffset)
 {
   MOZ_ASSERT(aInOutDestOffset);