Bug 1417492 - HTMLEditRules::TryToJoinBlocks() should no do nothing when left list node and right list node are not descendant of each other r=m_kato a=gchang
authorMasayuki Nakano <masayuki@d-toybox.com>
Sat, 18 Nov 2017 16:12:44 +0900
changeset 444893 3412b2ad3c3dbeb3f42958e5642edee86a66ac73
parent 444892 6036ae55ded07157b4d1f89759fcc72e8f8cc793
child 444894 fcc68a359d705653058244c2b1dd318e8b1d2f09
push id1618
push userCallek@gmail.com
push dateThu, 11 Jan 2018 17:45:48 +0000
treeherdermozilla-release@882ca853e05a [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersm_kato, gchang
bugs1417492, 1414713
milestone58.0
Bug 1417492 - HTMLEditRules::TryToJoinBlocks() should no do nothing when left list node and right list node are not descendant of each other r=m_kato a=gchang We hit assertion which were added by bug 1414713. That tells us an ancient bug. There is comment which claims that we should move all list items in the right list node to left list node if the list nodes should be merged. However, this has never been done actually since 2002. The code tried to move *some* list items in the right list node to the list list node. However, retrieving first list item at an offset almost always failed because the offset variable has never been initialized. If we believe the comment, we should move all children of the right list node to the left list node. However, until we get a testcase to reach this case, we should keep current behavior, i.e., do nothing, for unexpected regression. MozReview-Commit-ID: 1r81q1m44oW
editor/libeditor/HTMLEditRules.cpp
--- a/editor/libeditor/HTMLEditRules.cpp
+++ b/editor/libeditor/HTMLEditRules.cpp
@@ -2903,119 +2903,121 @@ 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) {
-      // 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?
+    if (NS_WARN_IF(mergeLists)) {
+      // Since 2002, here was the following comment:
+      // > The idea here is to take all children in rightList that are past
+      // > offset, and pull them into leftlist.
+      // However, this has never been performed because we are here only when
+      // neither left list nor right list is a descendant of the other but
+      // in such case, getting a list item in the right list node almost
+      // always failed since a variable for offset of rightList->GetChildAt()
+      // was not initialized.  So, it might be a bug, but we should keep this
+      // traditional behavior for now.  If you find when we get here, please
+      // remove this comment if we don't need to do it.  Otherwise, please
+      // move children of the right list node to the end of the left list node.
+      MOZ_DIAGNOSTIC_ASSERT(!atChildInBlock.IsSet());
+
+      // XXX Although, we don't do nothing here, but for keeping traditional
+      //     behavior, we should mark as handled.
       ret.MarkAsHandled();
-      // childInBlock and rightBlockChild were moved to leftList.  So, they
-      // are now invalid.
-      rightBlockChild.Clear();
-      childInBlock.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,
@@ -3124,17 +3126,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