Bug 1408227 - part 1: TextEditor::CreateBRImpl() should take |const EditorRawDOMPoint&| for insertion point of new <br> element r?m_kato draft
authorMasayuki Nakano <masayuki@d-toybox.com>
Tue, 21 Nov 2017 14:38:19 +0900
changeset 705789 93e80ca698d592f7dcd55b951a3836615eb3638a
parent 705524 02eebc3277dd7a707722c5a9ecfe42fdace8eaec
child 705790 fda543e07df3422bb3e7c81c75b93963852d3cab
child 706046 14c9611fa71a4145720a1ac0f2148d88b6725e60
push id91582
push usermasayuki@d-toybox.com
push dateThu, 30 Nov 2017 17:12:13 +0000
reviewersm_kato
bugs1408227
milestone59.0a1
Bug 1408227 - part 1: TextEditor::CreateBRImpl() should take |const EditorRawDOMPoint&| for insertion point of new <br> element r?m_kato TextEditor::CreateBRImpl() should take |const EditorRawDOMPoint&| as insertion point of new <br> element. Additionally, it doesn't need to have out argument for the point of after <br> element because callers can get it with EditorRawDOMPoint(nsINode*) and the new <br> node, and calling AdvanceOffset(). This cost must be enough cheap. MozReview-Commit-ID: Hxawz3D2dCd
editor/libeditor/HTMLAbsPositionEditor.cpp
editor/libeditor/HTMLEditRules.cpp
editor/libeditor/HTMLEditor.cpp
editor/libeditor/HTMLEditor.h
editor/libeditor/TextEditor.cpp
editor/libeditor/TextEditor.h
editor/libeditor/WSRunObject.cpp
--- a/editor/libeditor/HTMLAbsPositionEditor.cpp
+++ b/editor/libeditor/HTMLAbsPositionEditor.cpp
@@ -529,19 +529,25 @@ HTMLEditor::AbsolutelyPositionElement(ns
 
     // we may need to create a br if the positioned element is alone in its
     // container
     nsCOMPtr<nsINode> element = do_QueryInterface(aElement);
     NS_ENSURE_STATE(element);
 
     nsINode* parentNode = element->GetParentNode();
     if (parentNode->GetChildCount() == 1) {
-      nsCOMPtr<nsIDOMNode> brNode;
-      nsresult rv = CreateBR(parentNode->AsDOMNode(), 0, address_of(brNode));
-      NS_ENSURE_SUCCESS(rv, rv);
+      RefPtr<Selection> selection = GetSelection();
+      if (NS_WARN_IF(!selection)) {
+        return NS_ERROR_FAILURE;
+      }
+      RefPtr<Element> newBRElement =
+        CreateBRImpl(*selection, EditorRawDOMPoint(parentNode, 0), eNone);
+      if (NS_WARN_IF(!newBRElement)) {
+        return NS_ERROR_FAILURE;
+      }
     }
   }
   else {
     mCSSEditUtils->RemoveCSSProperty(*element, *nsGkAtoms::position,
                                      EmptyString());
     mCSSEditUtils->RemoveCSSProperty(*element, *nsGkAtoms::top,
                                      EmptyString());
     mCSSEditUtils->RemoveCSSProperty(*element, *nsGkAtoms::left,
--- a/editor/libeditor/HTMLEditRules.cpp
+++ b/editor/libeditor/HTMLEditRules.cpp
@@ -1433,30 +1433,37 @@ HTMLEditRules::WillInsertText(EditAction
           pos = tString.Length();
         }
 
         nsDependentSubstring subStr(tString, oldPos, subStrLen);
 
         // is it a return?
         if (subStr.Equals(newlineStr)) {
           NS_ENSURE_STATE(mHTMLEditor);
-          nsCOMPtr<nsINode> curNode = currentPoint.Container();
-          int32_t curOffset = currentPoint.Offset();
           nsCOMPtr<Element> br =
-            mHTMLEditor->CreateBRImpl(address_of(curNode), &curOffset,
+            mHTMLEditor->CreateBRImpl(*aSelection, currentPoint.AsRaw(),
                                       nsIEditor::eNone);
           NS_ENSURE_STATE(br);
           pos++;
           if (br->GetNextSibling()) {
             pointToInsert.Set(br->GetNextSibling());
           } else {
-            pointToInsert.Set(curNode, curNode->Length());
+            pointToInsert.Set(currentPoint.Container(),
+                              currentPoint.Container()->Length());
           }
-          currentPoint.Set(curNode, curOffset);
-          MOZ_ASSERT(currentPoint == pointToInsert);
+          // XXX In most cases, pointToInsert and currentPoint are same here.
+          //     But if the <br> element has been moved to different point by
+          //     mutation observer, those points become different.
+          currentPoint.Set(br);
+          DebugOnly<bool> advanced = currentPoint.AdvanceOffset();
+          NS_WARNING_ASSERTION(advanced,
+            "Failed to advance offset after the new <br> element");
+          NS_WARNING_ASSERTION(currentPoint == pointToInsert,
+            "Perhaps, <br> element position has been moved to different point "
+            "by mutation observer");
         } else {
           NS_ENSURE_STATE(mHTMLEditor);
           EditorRawDOMPoint pointAfterInsertedString;
           rv = mHTMLEditor->InsertTextImpl(*doc, subStr, currentPoint.AsRaw(),
                                            &pointAfterInsertedString);
           NS_ENSURE_SUCCESS(rv, rv);
           currentPoint = pointAfterInsertedString;
           pointToInsert = pointAfterInsertedString;
--- a/editor/libeditor/HTMLEditor.cpp
+++ b/editor/libeditor/HTMLEditor.cpp
@@ -1044,25 +1044,25 @@ HTMLEditor::InsertBR()
   RefPtr<Selection> selection = GetSelection();
   NS_ENSURE_STATE(selection);
 
   if (!selection->Collapsed()) {
     nsresult rv = DeleteSelection(nsIEditor::eNone, nsIEditor::eStrip);
     NS_ENSURE_SUCCESS(rv, rv);
   }
 
-  nsCOMPtr<nsINode> selNode;
-  int32_t selOffset;
-  nsresult rv =
-    GetStartNodeAndOffset(selection, getter_AddRefs(selNode), &selOffset);
-  NS_ENSURE_SUCCESS(rv, rv);
-
-  // position selection after br
-  RefPtr<Element> br = CreateBR(selNode, selOffset, nsIEditor::eNext);
-  if (NS_WARN_IF(!br)) {
+  EditorRawDOMPoint atStartOfSelection(GetStartPoint(selection));
+  if (NS_WARN_IF(!atStartOfSelection.IsSet())) {
+    return NS_ERROR_FAILURE;
+  }
+
+  // CreateBRImpl() will set selection after the new <br> element.
+  RefPtr<Element> newBRElement =
+    CreateBRImpl(*selection, atStartOfSelection, nsIEditor::eNext);
+  if (NS_WARN_IF(!newBRElement)) {
     return NS_ERROR_FAILURE;
   }
   return NS_OK;
 }
 
 void
 HTMLEditor::CollapseSelectionToDeepestNonTableFirstChild(Selection* aSelection,
                                                          nsINode* aNode)
@@ -1503,23 +1503,26 @@ HTMLEditor::InsertElementAtSelection(nsI
       // Set caret after element, but check for special case
       //  of inserting table-related elements: set in first cell instead
       if (!SetCaretInTableCell(aElement)) {
         rv = SetCaretAfterElement(aElement);
         NS_ENSURE_SUCCESS(rv, rv);
       }
       // check for inserting a whole table at the end of a block. If so insert
       // a br after it.
-      if (HTMLEditUtils::IsTable(node)) {
-        if (IsLastEditableChild(element)) {
-          nsCOMPtr<nsIDOMNode> brNode;
-          rv = CreateBR(parentSelectedDOMNode, offsetForInsert + 1,
-                        address_of(brNode));
-          NS_ENSURE_SUCCESS(rv, rv);
-          selection->Collapse(parentSelectedDOMNode, offsetForInsert+1);
+      if (HTMLEditUtils::IsTable(node) &&
+          IsLastEditableChild(element)) {
+        // Collapse selection to the new <br> element node after creating it.
+        RefPtr<Element> newBRElement =
+          CreateBRImpl(*selection,
+                       EditorRawDOMPoint(parentSelectedDOMNode,
+                                         offsetForInsert + 1),
+                       ePrevious);
+        if (NS_WARN_IF(!newBRElement)) {
+          return NS_ERROR_FAILURE;
         }
       }
     }
   }
   rv = rules->DidDoAction(selection, &ruleInfo, rv);
   return rv;
 }
 
--- a/editor/libeditor/HTMLEditor.h
+++ b/editor/libeditor/HTMLEditor.h
@@ -465,16 +465,20 @@ protected:
 
   /**
    * Return TRUE if aElement is a table-related elemet and caret was set.
    */
   bool SetCaretInTableCell(nsIDOMElement* aElement);
 
   nsresult TabInTable(bool inIsShift, bool* outHandled);
 
+  /**
+   * InsertBR() inserts a new <br> element at selection.  If there is
+   * non-collapsed selection ranges, the selected ranges is deleted first.
+   */
   nsresult InsertBR();
 
   // Table Editing (implemented in nsTableEditor.cpp)
 
   /**
    * Insert a new cell after or before supplied aCell.
    * Optional: If aNewCell supplied, returns the newly-created cell (addref'd,
    * of course)
--- a/editor/libeditor/TextEditor.cpp
+++ b/editor/libeditor/TextEditor.cpp
@@ -412,129 +412,122 @@ TextEditor::TypedText(const nsAString& a
       return InsertLineBreak();
     default:
       // eTypedBR is only for HTML
       return NS_ERROR_FAILURE;
   }
 }
 
 already_AddRefed<Element>
-TextEditor::CreateBRImpl(nsCOMPtr<nsINode>* aInOutParent,
-                         int32_t* aInOutOffset,
-                         EDirection aSelect)
+TextEditor::CreateBR(nsINode* aNode,
+                     int32_t aOffset,
+                     EDirection aSelect /* = eNone */)
 {
-  nsCOMPtr<nsIDOMNode> parent(GetAsDOMNode(*aInOutParent));
-  nsCOMPtr<nsIDOMNode> br;
-  // We ignore the retval, and assume it's fine if the br is non-null
-  CreateBRImpl(address_of(parent), aInOutOffset, address_of(br), aSelect);
-  *aInOutParent = do_QueryInterface(parent);
-  nsCOMPtr<Element> ret(do_QueryInterface(br));
-  return ret.forget();
+  RefPtr<Selection> selection = GetSelection();
+  if (NS_WARN_IF(!selection)) {
+    return nullptr;
+  }
+  // We assume everything is fine if newBRElement is not null.
+  return CreateBRImpl(*selection, EditorRawDOMPoint(aNode, aOffset), aSelect);
 }
 
-nsresult
-TextEditor::CreateBRImpl(nsCOMPtr<nsIDOMNode>* aInOutParent,
-                         int32_t* aInOutOffset,
-                         nsCOMPtr<nsIDOMNode>* outBRNode,
+already_AddRefed<Element>
+TextEditor::CreateBRImpl(Selection& aSelection,
+                         const EditorRawDOMPoint& aPointToInsert,
                          EDirection aSelect)
 {
-  NS_ENSURE_TRUE(aInOutParent && *aInOutParent && aInOutOffset && outBRNode, NS_ERROR_NULL_POINTER);
-  *outBRNode = nullptr;
-
-  // we need to insert a br.  unfortunately, we may have to split a text node to do it.
-  nsCOMPtr<nsINode> node = do_QueryInterface(*aInOutParent);
-  int32_t theOffset = *aInOutOffset;
-  RefPtr<Element> brNode;
-  if (IsTextNode(node)) {
-    EditorRawDOMPoint pointToInsertBrNode(node);
-    if (NS_WARN_IF(!pointToInsertBrNode.IsSetAndValid())) {
-      return NS_ERROR_FAILURE;
-    }
-    if (!theOffset) {
-      // we are already set to go
-    } else if (theOffset == static_cast<int32_t>(node->Length())) {
-      // update offset to point AFTER the text node
-      pointToInsertBrNode.AdvanceOffset();
-    } else {
-      MOZ_DIAGNOSTIC_ASSERT(theOffset < static_cast<int32_t>(node->Length()));
-      // split the text node
-      EditorRawDOMPoint atStartOfNewLine(node, theOffset);
-      ErrorResult error;
-      nsCOMPtr<nsIContent> newLeftNode = SplitNode(atStartOfNewLine, error);
-      if (NS_WARN_IF(error.Failed())) {
-        return error.StealNSResult();
-      }
-      // The right node offset in the parent is now changed.  Recompute it.
-      pointToInsertBrNode.Set(node);
-      Unused << newLeftNode;
-    }
-    // Lock the offset of pointToInsertBrNode because it'll be referred after
-    // inserting a new <br> node before it.
-    Unused << pointToInsertBrNode.Offset();
-    // create br
-    brNode = CreateNode(nsGkAtoms::br, pointToInsertBrNode);
-    if (NS_WARN_IF(!brNode)) {
-      return NS_ERROR_FAILURE;
-    }
-    *aInOutParent = GetAsDOMNode(pointToInsertBrNode.Container());
-    *aInOutOffset = pointToInsertBrNode.Offset() + 1;
-  } else {
-    EditorRawDOMPoint pointToInsertBrNode(node, theOffset);
-    brNode = CreateNode(nsGkAtoms::br, pointToInsertBrNode);
-    if (NS_WARN_IF(!brNode)) {
-      return NS_ERROR_FAILURE;
-    }
-    (*aInOutOffset)++;
+  if (NS_WARN_IF(!aPointToInsert.IsSet())) {
+    return nullptr;
   }
 
-  *outBRNode = GetAsDOMNode(brNode);
-  if (*outBRNode && (aSelect != eNone)) {
-    RefPtr<Selection> selection = GetSelection();
-    NS_ENSURE_STATE(selection);
-    if (aSelect == eNext) {
-      selection->SetInterlinePosition(true);
-      // position selection after br
-      EditorRawDOMPoint afterBrNode(brNode);
-      if (NS_WARN_IF(!afterBrNode.AdvanceOffset())) {
-        return NS_OK;
+  // We need to insert a <br> node.
+  RefPtr<Element> newBRElement;
+  if (IsTextNode(aPointToInsert.Container())) {
+    EditorDOMPoint pointInContainer;
+    if (aPointToInsert.IsStartOfContainer()) {
+      // Insert before the text node.
+      pointInContainer.Set(aPointToInsert.Container());
+      if (NS_WARN_IF(!pointInContainer.IsSet())) {
+        return nullptr;
+      }
+    } else if (aPointToInsert.IsEndOfContainer()) {
+      // Insert after the text node.
+      pointInContainer.Set(aPointToInsert.Container());
+      if (NS_WARN_IF(!pointInContainer.IsSet())) {
+        return nullptr;
       }
-      selection->Collapse(afterBrNode);
-    } else if (aSelect == ePrevious) {
-      selection->SetInterlinePosition(true);
-      // position selection before br
-      EditorRawDOMPoint atBrNode(brNode);
-      if (NS_WARN_IF(!atBrNode.IsSetAndValid())) {
-        return NS_OK;
+      DebugOnly<bool> advanced = pointInContainer.AdvanceOffset();
+      NS_WARNING_ASSERTION(advanced,
+        "Failed to advance offset to after the text node");
+    } else {
+      MOZ_DIAGNOSTIC_ASSERT(aPointToInsert.IsSetAndValid());
+      // Unfortunately, we need to split the text node at the offset.
+      ErrorResult error;
+      nsCOMPtr<nsIContent> newLeftNode = SplitNode(aPointToInsert, error);
+      if (NS_WARN_IF(error.Failed())) {
+        error.SuppressException();
+        return nullptr;
       }
-      selection->Collapse(atBrNode);
+      Unused << newLeftNode;
+      // Insert new <br> before the right node.
+      pointInContainer.Set(aPointToInsert.Container());
+    }
+    // Create a <br> node.
+    newBRElement = CreateNode(nsGkAtoms::br, pointInContainer.AsRaw());
+    if (NS_WARN_IF(!newBRElement)) {
+      return nullptr;
+    }
+  } else {
+    newBRElement = CreateNode(nsGkAtoms::br, aPointToInsert);
+    if (NS_WARN_IF(!newBRElement)) {
+      return nullptr;
     }
   }
-  return NS_OK;
-}
-
 
-already_AddRefed<Element>
-TextEditor::CreateBR(nsINode* aNode,
-                     int32_t aOffset,
-                     EDirection aSelect)
-{
-  nsCOMPtr<nsINode> parent = aNode;
-  int32_t offset = aOffset;
-  return CreateBRImpl(address_of(parent), &offset, aSelect);
-}
+  switch (aSelect) {
+    case eNone:
+      break;
+    case eNext: {
+      aSelection.SetInterlinePosition(true);
+      // Collapse selection after the <br> node.
+      EditorRawDOMPoint afterBRElement(newBRElement);
+      if (afterBRElement.IsSet()) {
+        DebugOnly<bool> advanced = afterBRElement.AdvanceOffset();
+        NS_WARNING_ASSERTION(advanced,
+          "Failed to advance offset after the <br> element");
+        ErrorResult error;
+        aSelection.Collapse(afterBRElement, error);
+        NS_WARNING_ASSERTION(!error.Failed(),
+          "Failed to collapse selection after the <br> element");
+      } else {
+        NS_WARNING("The <br> node is not in the DOM tree?");
+      }
+      break;
+    }
+    case ePrevious: {
+      aSelection.SetInterlinePosition(true);
+      // Collapse selection at the <br> node.
+      EditorRawDOMPoint atBRElement(newBRElement);
+      if (atBRElement.IsSet()) {
+        ErrorResult error;
+        aSelection.Collapse(atBRElement, error);
+        NS_WARNING_ASSERTION(!error.Failed(),
+          "Failed to collapse selection at the <br> element");
+      } else {
+        NS_WARNING("The <br> node is not in the DOM tree?");
+      }
+      break;
+    }
+    default:
+      NS_WARNING("aSelect has invalid value, the caller need to set selection "
+                 "by itself");
+      break;
+  }
 
-nsresult
-TextEditor::CreateBR(nsIDOMNode* aNode,
-                     int32_t aOffset,
-                     nsCOMPtr<nsIDOMNode>* outBRNode,
-                     EDirection aSelect)
-{
-  nsCOMPtr<nsIDOMNode> parent = aNode;
-  int32_t offset = aOffset;
-  return CreateBRImpl(address_of(parent), &offset, outBRNode, aSelect);
+  return newBRElement.forget();
 }
 
 nsresult
 TextEditor::ExtendSelectionForDelete(Selection* aSelection,
                                      nsIEditor::EDirection* aAction)
 {
   bool bCollapsed = aSelection->Collapsed();
 
--- a/editor/libeditor/TextEditor.h
+++ b/editor/libeditor/TextEditor.h
@@ -182,28 +182,54 @@ protected:
   void BeginEditorInit();
   nsresult EndEditorInit();
 
   already_AddRefed<nsIDocumentEncoder> GetAndInitDocEncoder(
                                          const nsAString& aFormatType,
                                          uint32_t aFlags,
                                          const nsACString& aCharset);
 
+  /**
+   * CreateBR() creates new <br> element and inserts it before the point,
+   * aNode - aOffset, and collapse selection if it's necessary.
+   *
+   * @param aNode       The container node to insert new <br> element.
+   * @param aOffset     The offset in aNode to insert new <br> element.
+   * @param aSelect     If eNone, this won't change selection.
+   *                    If eNext, selection will be collapsed after the <br>
+   *                    element.
+   *                    If ePrevious, selection will be collapsed at the <br>
+   *                    element.
+   * @return            The new <br> node.  If failed to create new <br> node,
+   *                    returns nullptr.
+   */
   already_AddRefed<Element> CreateBR(nsINode* aNode, int32_t aOffset,
                                      EDirection aSelect = eNone);
-  nsresult CreateBR(nsIDOMNode* aNode, int32_t aOffset,
-                    nsCOMPtr<nsIDOMNode>* outBRNode,
-                    EDirection aSelect = eNone);
-  already_AddRefed<Element> CreateBRImpl(nsCOMPtr<nsINode>* aInOutParent,
-                                         int32_t* aInOutOffset,
-                                         EDirection aSelect);
-  nsresult CreateBRImpl(nsCOMPtr<nsIDOMNode>* aInOutParent,
-                        int32_t* aInOutOffset,
-                        nsCOMPtr<nsIDOMNode>* outBRNode,
-                        EDirection aSelect);
+
+  /**
+   * CreateBRImpl() creates a <br> element and inserts it before aPointToInsert.
+   * Then, tries to collapse selection at or after the new <br> node if
+   * aSelect is not eNone.
+   * XXX Perhaps, this should be merged with CreateBR().
+   *
+   * @param aSelection          The selection of this editor.
+   * @param aPointToInsert      The DOM point where should be <br> node inserted
+   *                            before.
+   * @param aSelect             If eNone, this won't change selection.
+   *                            If eNext, selection will be collapsed after
+   *                            the <br> element.
+   *                            If ePrevious, selection will be collapsed at
+   *                            the <br> element.
+   * @return                    The new <br> node.  If failed to create new
+   *                            <br> node, returns nullptr.
+   */
+  already_AddRefed<Element>
+  CreateBRImpl(Selection& aSelection,
+               const EditorRawDOMPoint& aPointToInsert,
+               EDirection aSelect);
 
   /**
    * Factored methods for handling insertion of data from transferables
    * (drag&drop or clipboard).
    */
   NS_IMETHOD PrepareTransferable(nsITransferable** transferable);
   nsresult InsertTextFromTransferable(nsITransferable* transferable,
                                       nsIDOMNode* aDestinationNode,
--- a/editor/libeditor/WSRunObject.cpp
+++ b/editor/libeditor/WSRunObject.cpp
@@ -223,18 +223,34 @@ WSRunObject::InsertBreak(nsCOMPtr<nsINod
       NS_ENSURE_SUCCESS(rv, nullptr);
     } else if (beforeRun->mType == WSType::normalWS) {
       // Try to change an nbsp to a space, just to prevent nbsp proliferation
       nsresult rv = CheckTrailingNBSP(beforeRun, *aInOutParent, *aInOutOffset);
       NS_ENSURE_SUCCESS(rv, nullptr);
     }
   }
 
-  // ready, aim, fire!
-  return mHTMLEditor->CreateBRImpl(aInOutParent, aInOutOffset, aSelect);
+  RefPtr<Selection> selection = mHTMLEditor->GetSelection();
+  if (NS_WARN_IF(!selection)) {
+    return nullptr;
+  }
+  RefPtr<Element> newBRElement =
+    mHTMLEditor->CreateBRImpl(*selection,
+                              EditorRawDOMPoint(*aInOutParent, *aInOutOffset),
+                              aSelect);
+  if (NS_WARN_IF(!newBRElement)) {
+    return nullptr;
+  }
+  EditorRawDOMPoint atNewBRElement(newBRElement);
+  DebugOnly<bool> advanced = atNewBRElement.AdvanceOffset();
+  NS_WARNING_ASSERTION(advanced,
+    "Failed to advance offset to after the new <br> element");
+  *aInOutParent = atNewBRElement.Container();
+  *aInOutOffset = atNewBRElement.Offset();
+  return newBRElement.forget();
 }
 
 nsresult
 WSRunObject::InsertText(nsIDocument& aDocument,
                         const nsAString& aStringToInsert,
                         const EditorRawDOMPoint& aPointToInsert,
                         EditorRawDOMPoint* aPointAfterInsertedString)
 {