Bug 1792387 - part 2-2: Make `HTMLEditor::DoJoinNodes()` handle both join nodes directions r=m_kato
authorMasayuki Nakano <masayuki@d-toybox.com>
Fri, 30 Sep 2022 22:20:20 +0000
changeset 636754 0df250e3eccca3fb6addf212ba186319f78b0e3a
parent 636753 fe6e3d412dd295b0ae8d640ee58b0aae881744de
child 636755 de9f2339aeb290e200ac33e737ae829de375d3c1
push id170484
push usermasayuki@d-toybox.com
push dateFri, 30 Sep 2022 22:23:25 +0000
treeherderautoland@f4c48015c119 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersm_kato
bugs1792387
milestone107.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1792387 - part 2-2: Make `HTMLEditor::DoJoinNodes()` handle both join nodes directions r=m_kato This changes the existing path's behavior a bit. However, it should not affect to web apps in the wild because it must be really rare case that web apps inserting new nodes into the removing node while moving its children. Differential Revision: https://phabricator.services.mozilla.com/D158099
editor/libeditor/HTMLEditor.cpp
editor/libeditor/SelectionState.cpp
editor/libeditor/SelectionState.h
--- a/editor/libeditor/HTMLEditor.cpp
+++ b/editor/libeditor/HTMLEditor.cpp
@@ -19,16 +19,17 @@
 #include "PendingStyles.h"
 #include "ReplaceTextTransaction.h"
 #include "SplitNodeTransaction.h"
 #include "WSRunObject.h"
 
 #include "mozilla/ComposerCommandsUpdater.h"
 #include "mozilla/ContentIterator.h"
 #include "mozilla/DebugOnly.h"
+#include "mozilla/EditorForwards.h"
 #include "mozilla/Encoding.h"      // for Encoding
 #include "mozilla/IntegerRange.h"  // for IntegerRange
 #include "mozilla/InternalMutationEvent.h"
 #include "mozilla/mozInlineSpellChecker.h"
 #include "mozilla/Preferences.h"
 #include "mozilla/PresShell.h"
 #include "mozilla/StaticPrefs_editor.h"
 #include "mozilla/StyleSheet.h"
@@ -5185,18 +5186,20 @@ nsresult HTMLEditor::DoJoinNodes(nsICont
                                  nsIContent& aContentToRemove,
                                  JoinNodesDirection aDirection) {
   MOZ_ASSERT(IsEditActionDataAvailable());
   MOZ_DIAGNOSTIC_ASSERT(aContentToKeep.GetParentNode() ==
                         aContentToRemove.GetParentNode());
 
   const uint32_t keepingContentLength = aContentToKeep.Length();
   const uint32_t removingContentLength = aContentToRemove.Length();
-  const Maybe<uint32_t> keepingContentExIndex =
-      aContentToKeep.ComputeIndexInParentNode();
+  const Maybe<uint32_t> rightContentExIndex =
+      aDirection == JoinNodesDirection::LeftNodeIntoRightNode
+          ? aContentToKeep.ComputeIndexInParentNode()
+          : aContentToRemove.ComputeIndexInParentNode();
 
   // Remember all selection points.
   // XXX Do we need to restore all types of selections by ourselves?  Normal
   //     selection should be modified later as result of handling edit action.
   //     IME selections shouldn't be there when nodes are joined.  Spellcheck
   //     selections should be recreated with newer text.  URL selections
   //     shouldn't be there because of used only by the URL bar.
   AutoTArray<SavedRange, 10> savedRanges;
@@ -5267,72 +5270,90 @@ nsresult HTMLEditor::DoJoinNodes(nsICont
   }
 
   // OK, ready to do join now.
   nsresult rv = [&]() MOZ_CAN_RUN_SCRIPT {
     // If it's a text node, just shuffle around some text.
     if (aContentToKeep.IsText() && aContentToRemove.IsText()) {
       nsAutoString rightText;
       nsAutoString leftText;
-      aContentToKeep.AsText()->GetData(rightText);
-      aContentToRemove.AsText()->GetData(leftText);
+      const nsIContent& rightTextNode =
+          aDirection == JoinNodesDirection::LeftNodeIntoRightNode
+              ? aContentToKeep
+              : aContentToRemove;
+      const nsIContent& leftTextNode =
+          aDirection == JoinNodesDirection::LeftNodeIntoRightNode
+              ? aContentToRemove
+              : aContentToKeep;
+      rightTextNode.AsText()->GetData(rightText);
+      leftTextNode.AsText()->GetData(leftText);
       leftText += rightText;
       IgnoredErrorResult ignoredError;
       DoSetText(MOZ_KnownLive(*aContentToKeep.AsText()), leftText,
                 ignoredError);
       if (NS_WARN_IF(Destroyed())) {
         return NS_ERROR_EDITOR_DESTROYED;
       }
       NS_WARNING_ASSERTION(!ignoredError.Failed(),
                            "EditorBase::DoSetText() failed, but ignored");
       return NS_OK;
     }
     // Otherwise it's an interior node, so shuffle around the children.
-    nsCOMPtr<nsINodeList> childNodes = aContentToRemove.ChildNodes();
-    MOZ_ASSERT(childNodes);
-
-    // Remember the first child in aContentToKeep, we'll insert all the children
-    // of aContentToRemove in front of it GetFirstChild returns nullptr
-    // firstChild if aContentToKeep has no children, that's OK.
-    nsCOMPtr<nsIContent> firstChild = aContentToKeep.GetFirstChild();
-
-    // Have to go through the list backwards to keep deletes from interfering
-    // with iteration.
-    for (uint32_t i = childNodes->Length(); i; --i) {
-      nsCOMPtr<nsIContent> childNode = childNodes->Item(i - 1);
-      if (childNode) {
-        // prepend children of aContentToRemove
+    AutoTArray<OwningNonNull<nsIContent>, 64> arrayOfChildContents;
+    HTMLEditor::GetChildNodesOf(aContentToRemove, arrayOfChildContents);
+
+    if (aDirection == JoinNodesDirection::LeftNodeIntoRightNode) {
+      for (const OwningNonNull<nsIContent>& child :
+           Reversed(arrayOfChildContents)) {
+        // Note that it's safe to pass the reference node to insert the child
+        // without making it grabbed by nsINode::mNextSibling before touching
+        // the DOM tree.
         IgnoredErrorResult error;
-        aContentToKeep.InsertBefore(*childNode, firstChild, error);
+        aContentToKeep.InsertBefore(child, aContentToKeep.GetFirstChild(),
+                                    error);
         if (NS_WARN_IF(Destroyed())) {
           return NS_ERROR_EDITOR_DESTROYED;
         }
         if (error.Failed()) {
           NS_WARNING("nsINode::InsertBefore() failed");
           return error.StealNSResult();
         }
-        firstChild = std::move(childNode);
+      }
+    } else {
+      for (const OwningNonNull<nsIContent>& child : arrayOfChildContents) {
+        IgnoredErrorResult error;
+        aContentToKeep.AppendChild(child, error);
+        if (NS_WARN_IF(Destroyed())) {
+          return NS_ERROR_EDITOR_DESTROYED;
+        }
+        if (error.Failed()) {
+          NS_WARNING("nsINode::AppendChild() failed");
+          return error.StealNSResult();
+        }
       }
     }
     return NS_OK;
   }();
 
   // Delete the extra node.
   if (NS_SUCCEEDED(rv)) {
     aContentToRemove.Remove();
     if (NS_WARN_IF(Destroyed())) {
       return NS_ERROR_EDITOR_DESTROYED;
     }
   }
 
-  if (MOZ_LIKELY(keepingContentExIndex.isSome())) {
+  if (MOZ_LIKELY(rightContentExIndex.isSome())) {
     DebugOnly<nsresult> rvIgnored = RangeUpdaterRef().SelAdjJoinNodes(
-        EditorRawDOMPoint(&aContentToKeep, std::min(removingContentLength,
-                                                    aContentToKeep.Length())),
-        aContentToRemove, *keepingContentExIndex, aDirection);
+        EditorRawDOMPoint(
+            &aContentToKeep,
+            aDirection == JoinNodesDirection::LeftNodeIntoRightNode
+                ? std::min(removingContentLength, aContentToKeep.Length())
+                : std::min(keepingContentLength, aContentToKeep.Length())),
+        aContentToRemove, *rightContentExIndex, aDirection);
     NS_WARNING_ASSERTION(NS_SUCCEEDED(rvIgnored),
                          "RangeUpdater::SelAdjJoinNodes() failed, but ignored");
   }
   if (MOZ_UNLIKELY(NS_FAILED(rv))) {
     return rv;
   }
 
   const bool allowedTransactionsToChangeSelection =
@@ -5413,17 +5434,19 @@ nsresult HTMLEditor::DoJoinNodes(nsICont
     if (NS_WARN_IF(error.Failed())) {
       return error.StealNSResult();
     }
   }
 
   if (allowedTransactionsToChangeSelection) {
     // Editor wants us to set selection at join point.
     DebugOnly<nsresult> rvIgnored = CollapseSelectionTo(
-        EditorRawDOMPoint(&aContentToKeep, removingContentLength));
+        aDirection == JoinNodesDirection::LeftNodeIntoRightNode
+            ? EditorRawDOMPoint(&aContentToKeep, removingContentLength)
+            : EditorRawDOMPoint(&aContentToKeep, 0u));
     if (MOZ_UNLIKELY(rv == NS_ERROR_EDITOR_DESTROYED)) {
       NS_WARNING(
           "EditorBase::CollapseSelectionTo() caused destroying the editor");
       return NS_ERROR_EDITOR_DESTROYED;
     }
     NS_WARNING_ASSERTION(
         NS_SUCCEEDED(rv),
         "EditorBases::CollapseSelectionTos() failed, but ignored");
--- a/editor/libeditor/SelectionState.cpp
+++ b/editor/libeditor/SelectionState.cpp
@@ -345,40 +345,40 @@ nsresult RangeUpdater::SelAdjSplitNode(n
     AdjustDOMPoint(rangeItem->mStartContainer, rangeItem->mStartOffset);
     AdjustDOMPoint(rangeItem->mEndContainer, rangeItem->mEndOffset);
   }
   return NS_OK;
 }
 
 nsresult RangeUpdater::SelAdjJoinNodes(
     const EditorRawDOMPoint& aStartOfRightContent,
-    const nsIContent& aRemovedContent, uint32_t aOffsetOfJoinedContent,
+    const nsIContent& aRemovedContent, uint32_t aExOffsetOfRightContent,
     JoinNodesDirection aJoinNodesDirection) {
   MOZ_ASSERT(aStartOfRightContent.IsSetAndValid());
 
   if (mLocked) {
     // lock set by Will/DidReplaceParent, etc...
     return NS_OK;
   }
 
   if (mArray.IsEmpty()) {
     return NS_OK;
   }
 
   auto AdjustDOMPoint = [&](nsCOMPtr<nsINode>& aContainer,
                             uint32_t& aOffset) -> void {
     if (aContainer == aStartOfRightContent.GetContainerParent()) {
-      // If the point is in common parent of joined content nodes and it pointed
-      // after the right content node, decrease the offset.
-      if (aOffset > aOffsetOfJoinedContent) {
+      // If the point is in common parent of joined content nodes and it
+      // pointed after the right content node, decrease the offset.
+      if (aOffset > aExOffsetOfRightContent) {
         aOffset--;
       }
       // If it pointed the right content node, adjust it to point ex-first
       // content of the right node.
-      else if (aOffset == aOffsetOfJoinedContent) {
+      else if (aOffset == aExOffsetOfRightContent) {
         aContainer = aStartOfRightContent.GetContainer();
         aOffset = aStartOfRightContent.Offset();
       }
     } else if (aContainer == aStartOfRightContent.GetContainer()) {
       // If the point is in joined node, and removed content is moved to
       // start of the joined node, we need to adjust the offset.
       if (aJoinNodesDirection == JoinNodesDirection::LeftNodeIntoRightNode) {
         aOffset += aStartOfRightContent.Offset();
--- a/editor/libeditor/SelectionState.h
+++ b/editor/libeditor/SelectionState.h
@@ -234,22 +234,22 @@ class MOZ_STACK_CLASS RangeUpdater final
    * SelAdjJoinNodes() is called immediately after joining aRemovedContent and
    * the container of aStartOfRightContent.
    *
    * @param aStartOfRightContent    The container is joined content node which
    *                                now has all children or text data which were
    *                                in aRemovedContent.  And this points where
    *                                the joined position.
    * @param aRemovedContent         The removed content.
-   * @param aOffsetOfJoinedContent  The offset which the container of
+   * @param aExOffsetOfRightContent The offset which the container of
    *                                aStartOfRightContent was in its parent.
    */
   nsresult SelAdjJoinNodes(const EditorRawDOMPoint& aStartOfRightContent,
                            const nsIContent& aRemovedContent,
-                           uint32_t aOffsetOfJoinedContent,
+                           uint32_t aExOffsetOfRightContent,
                            JoinNodesDirection aJoinNodesDirection);
   void SelAdjInsertText(const dom::Text& aTextNode, uint32_t aOffset,
                         uint32_t aInsertedLength);
   void SelAdjDeleteText(const dom::Text& aTextNode, uint32_t aOffset,
                         uint32_t aDeletedLength);
   void SelAdjReplaceText(const dom::Text& aTextNode, uint32_t aOffset,
                          uint32_t aReplacedLength, uint32_t aInsertedLength);
   // the following gravity routines need will/did sandwiches, because the other