Bug 1417492 - HTMLEditRules::TryToJoinBlocks() should move all list items in the right list node to the left list node when they should be merged r?m_kato draft
authorMasayuki Nakano <masayuki@d-toybox.com>
Sat, 18 Nov 2017 16:12:44 +0900
changeset 700158 7e6d1751001447ddfc30d808d8ffad85459af117
parent 700056 daa0dcd1616cbdf5541f548e44662d20d6d67d99
child 740792 a4605b364523a2eae1aae4868804c9adc203ea73
push id89747
push usermasayuki@d-toybox.com
push dateSat, 18 Nov 2017 15:09:22 +0000
reviewersm_kato
bugs1417492, 1414713, 1392867
milestone59.0a1
Bug 1417492 - HTMLEditRules::TryToJoinBlocks() should move all list items in the right list node to the left list node when they should be merged r?m_kato We hit assertion which were added by bug 1414713. That tells us regression of bug 1392867. Starting from bug 1392867, we receive ancestor node's child node at calling EditorUtils::IsDescendantOf(). However, it's never set when it returns false. HTMLEditRules::TryToJoinBlocks() thinks that it needs to merge right list and left list when they are not a descendant of each other. So, the result of EditorUtils::IsDescendantOf() has never been set. However, the code moving right list items to left list believes the point which is set by EditorUtils::IsDescendantOf() refers last child of the right list. So, we should move all children of the right list to the left node in this case. I think that we can use HTMLEditRules::MoveContents() simply in this case. It just moves all contents of right node to left node and returns error if it failed to move one of the children. MozReview-Commit-ID: 1r81q1m44oW
editor/libeditor/HTMLEditRules.cpp
--- a/editor/libeditor/HTMLEditRules.cpp
+++ b/editor/libeditor/HTMLEditRules.cpp
@@ -2920,119 +2920,120 @@ HTMLEditRules::TryToJoinBlocks(nsIConten
       rightBlock->GetParentNode() == leftBlock) {
     return EditActionHandled();
   }
 
   // Special rule here: if we are trying to join list items, and they are in
   // different lists, join the lists instead.
   bool mergeLists = false;
   nsAtom* existingList = nsGkAtoms::_empty;
-  EditorDOMPoint childInBlock;
+  EditorDOMPoint atChildInBlock;
   nsCOMPtr<Element> leftList, rightList;
   if (HTMLEditUtils::IsListItem(leftBlock) &&
       HTMLEditUtils::IsListItem(rightBlock)) {
     leftList = leftBlock->GetParentElement();
     rightList = rightBlock->GetParentElement();
     if (leftList && rightList && leftList != rightList &&
-        !EditorUtils::IsDescendantOf(*leftList, *rightBlock, &childInBlock) &&
-        !EditorUtils::IsDescendantOf(*rightList, *leftBlock, &childInBlock)) {
+        !EditorUtils::IsDescendantOf(*leftList, *rightBlock, &atChildInBlock) &&
+        !EditorUtils::IsDescendantOf(*rightList, *leftBlock, &atChildInBlock)) {
       // There are some special complications if the lists are descendants of
       // the other lists' items.  Note that it is okay for them to be
       // descendants of the other lists themselves, which is the usual case for
       // sublists in our implementation.
-      MOZ_DIAGNOSTIC_ASSERT(!childInBlock.IsSet());
+      MOZ_DIAGNOSTIC_ASSERT(!atChildInBlock.IsSet());
       leftBlock = leftList;
       rightBlock = rightList;
       mergeLists = true;
       existingList = leftList->NodeInfo()->NameAtom();
     }
   }
 
   AutoTransactionsConserveSelection dontChangeMySelection(htmlEditor);
 
   // offset below is where you find yourself in rightBlock when you traverse
   // upwards from leftBlock
-  EditorDOMPoint rightBlockChild;
-  if (EditorUtils::IsDescendantOf(*leftBlock, *rightBlock, &rightBlockChild)) {
+  EditorDOMPoint atRightBlockChild;
+  if (EditorUtils::IsDescendantOf(*leftBlock, *rightBlock,
+                                  &atRightBlockChild)) {
     // Tricky case.  Left block is inside right block.  Do ws adjustment.  This
     // just destroys non-visible ws at boundaries we will be joining.
-    rightBlockChild.AdvanceOffset();
+    DebugOnly<bool> advanced = atRightBlockChild.AdvanceOffset();
+    NS_WARNING_ASSERTION(advanced,
+      "Failed to advance offset to after child of rightBlock, "
+      "leftBlock is a descendant of the child");
     nsresult rv = WSRunObject::ScrubBlockBoundary(htmlEditor,
                                                   WSRunObject::kBlockEnd,
                                                   leftBlock);
     if (NS_WARN_IF(NS_FAILED(rv))) {
       return EditActionIgnored(rv);
     }
 
     {
       // We can't just track rightBlock because it's an Element.
-      AutoTrackDOMPoint tracker(htmlEditor->mRangeUpdater, &rightBlockChild);
+      AutoTrackDOMPoint tracker(htmlEditor->mRangeUpdater, &atRightBlockChild);
       rv = WSRunObject::ScrubBlockBoundary(htmlEditor,
                                            WSRunObject::kAfterBlock,
-                                           rightBlock,
-                                           rightBlockChild.Offset());
+                                           atRightBlockChild.Container(),
+                                           atRightBlockChild.Offset());
       if (NS_WARN_IF(NS_FAILED(rv))) {
         return EditActionIgnored(rv);
       }
 
       // XXX AutoTrackDOMPoint instance, tracker, hasn't been destroyed here.
       //     Do we really need to do update rightBlock here??
-      MOZ_ASSERT(rightBlock == rightBlockChild.Container());
-      if (rightBlockChild.Container()->IsElement()) {
-        rightBlock = rightBlockChild.Container()->AsElement();
+      MOZ_ASSERT(rightBlock == atRightBlockChild.Container());
+      if (atRightBlockChild.Container()->IsElement()) {
+        rightBlock = atRightBlockChild.Container()->AsElement();
       } else {
-        if (NS_WARN_IF(!rightBlockChild.Container()->GetParentElement())) {
+        if (NS_WARN_IF(!atRightBlockChild.Container()->GetParentElement())) {
           return EditActionIgnored(NS_ERROR_UNEXPECTED);
         }
-        rightBlock = rightBlockChild.Container()->GetParentElement();
-      }
-    }
+        rightBlock = atRightBlockChild.Container()->GetParentElement();
+      }
+    }
+
     // Do br adjustment.
     nsCOMPtr<Element> brNode =
       CheckForInvisibleBR(*leftBlock, BRLocation::blockEnd);
     EditActionResult ret(NS_OK);
     if (mergeLists) {
+      MOZ_DIAGNOSTIC_ASSERT(!atChildInBlock.IsSet());
       // The idea here is to take all children in rightList that are past
       // offset, and pull them into leftlist.
-      // XXX Looks like that when mergeLists is true, childInBlock has never
-      //     been set.  So, this block must be dead code.
-      MOZ_DIAGNOSTIC_ASSERT(childInBlock.IsSet());
-      uint32_t offset = rightBlockChild.Offset();
-      for (nsCOMPtr<nsIContent> child = childInBlock.GetChildAtOffset();
-           child; child = rightList->GetChildAt(offset)) {
-        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?
+      int32_t append = -1;
+      ret = MoveContents(*rightList, *leftList, &append);
+      if (NS_WARN_IF(ret.Failed())) {
+        return ret;
+      }
+      // XXX Should this set to true only when above MoveContents moves some
+      //     list items actually?
       ret.MarkAsHandled();
-      // childInBlock and rightBlockChild were moved to leftList.  So, they
+      // atChildInBlock and atRightBlockChild were moved to leftList.  So, they
       // are now invalid.
-      rightBlockChild.Clear();
-      childInBlock.Clear();
+      atRightBlockChild.Clear();
+      atChildInBlock.Clear();
     } else {
       // XXX Why do we ignore the result of MoveBlock()?
       EditActionResult retMoveBlock =
         MoveBlock(*leftBlock, *rightBlock,
-                  -1, rightBlockChild.Offset());
+                  -1, atRightBlockChild.Offset());
       if (retMoveBlock.Handled()) {
         ret.MarkAsHandled();
       }
       // Now, all children of rightBlock were moved to leftBlock.  So,
-      // rightBlockChild is now invalid.
-      rightBlockChild.Clear();
+      // atRightBlockChild is now invalid.
+      atRightBlockChild.Clear();
     }
     if (brNode && NS_SUCCEEDED(htmlEditor->DeleteNode(brNode))) {
       ret.MarkAsHandled();
     }
     return ret;
   }
 
-  MOZ_DIAGNOSTIC_ASSERT(!rightBlockChild.IsSet());
+  MOZ_DIAGNOSTIC_ASSERT(!atRightBlockChild.IsSet());
 
   // Offset below is where you find yourself in leftBlock when you traverse
   // upwards from rightBlock
   EditorDOMPoint leftBlockChild;
   if (EditorUtils::IsDescendantOf(*rightBlock, *leftBlock, &leftBlockChild)) {
     // 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,
@@ -3141,17 +3142,17 @@ HTMLEditRules::TryToJoinBlocks(nsIConten
       }
     }
     if (brNode && NS_SUCCEEDED(htmlEditor->DeleteNode(brNode))) {
       ret.MarkAsHandled();
     }
     return ret;
   }
 
-  MOZ_DIAGNOSTIC_ASSERT(!rightBlockChild.IsSet());
+  MOZ_DIAGNOSTIC_ASSERT(!atRightBlockChild.IsSet());
   MOZ_DIAGNOSTIC_ASSERT(!leftBlockChild.IsSet());
 
   // 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