Bug 1436295 - HTMLEditRules::WillInsertBreak() should cancel resetting EditorDOMPoint when HTMLEditRules::ReturnInParagraph() splits DOM node around the point r=m_kato
authorMasayuki Nakano <masayuki@d-toybox.com>
Wed, 07 Feb 2018 19:04:52 +0900
changeset 752943 e05678d156d9a3cbb2ad3234ff76e5032994f325
parent 752942 ac157b31db6e487e7421cf8ee1fd8a0b3aa9b3b3
child 752944 752a687900e2b02dd5df2ff22f9dc23131bcd0b3
push id98429
push usermak77@bonardo.net
push dateFri, 09 Feb 2018 10:14:12 +0000
reviewersm_kato
bugs1436295
milestone60.0a1
Bug 1436295 - HTMLEditRules::WillInsertBreak() should cancel resetting EditorDOMPoint when HTMLEditRules::ReturnInParagraph() splits DOM node around the point r=m_kato This patch only fixes warning, not changing actual behavior of editor. HTMLEditRules::ReturnInParagraph() splits paragraph *around* given point. Therefore, from point of view of caller, offset of setting point may be invalid after HTMLEditRules::ReturnInParagraph() handled the edit action. In this case, invalidating stored child of the point may cause warning since offset may be larger than length of its container. So, if HTMLEditRules::ReturnInParagraph() handles the edit action, the caller, HTMLEditRules::WillInsertBreak(), should cancel invalidating the stored child for avoiding unnecessary warning. MozReview-Commit-ID: DKJlr0Awwlo
editor/libeditor/EditorDOMPoint.h
editor/libeditor/HTMLEditRules.cpp
--- a/editor/libeditor/EditorDOMPoint.h
+++ b/editor/libeditor/EditorDOMPoint.h
@@ -825,48 +825,61 @@ ImplCycleCollectionTraverse(nsCycleColle
  * destroyed.  Additionally, users of this class can invalidate the offset
  * manually when they need.
  */
 class MOZ_STACK_CLASS AutoEditorDOMPointOffsetInvalidator final
 {
 public:
   explicit AutoEditorDOMPointOffsetInvalidator(EditorDOMPoint& aPoint)
     : mPoint(aPoint)
+    , mCanceled(false)
   {
     MOZ_ASSERT(aPoint.IsSetAndValid());
     MOZ_ASSERT(mPoint.CanContainerHaveChildren());
     mChild = mPoint.GetChild();
   }
 
   ~AutoEditorDOMPointOffsetInvalidator()
   {
-    InvalidateOffset();
+    if (!mCanceled) {
+      InvalidateOffset();
+    }
   }
 
   /**
    * Manually, invalidate offset of the given point.
    */
   void InvalidateOffset()
   {
     if (mChild) {
       mPoint.Set(mChild);
     } else {
       // If the point referred after the last child, let's keep referring
       // after current last node of the old container.
       mPoint.SetToEndOf(mPoint.GetContainer());
     }
   }
 
+  /**
+   * After calling Cancel(), mPoint won't be modified by the destructor.
+   */
+  void Cancel()
+  {
+    mCanceled = true;
+  }
+
 private:
   EditorDOMPoint& mPoint;
   // Needs to store child node by ourselves because EditorDOMPoint stores
   // child node with mRef which is previous sibling of current child node.
   // Therefore, we cannot keep referring it if it's first child.
   nsCOMPtr<nsIContent> mChild;
 
+  bool mCanceled;
+
   AutoEditorDOMPointOffsetInvalidator() = delete;
   AutoEditorDOMPointOffsetInvalidator(
     const AutoEditorDOMPointOffsetInvalidator& aOther) = delete;
   const AutoEditorDOMPointOffsetInvalidator& operator=(
     const AutoEditorDOMPointOffsetInvalidator& aOther) = delete;
 };
 
 /**
@@ -879,37 +892,50 @@ private:
  * Additionally, users of this class can invalidate the child manually when
  * they need.
  */
 class MOZ_STACK_CLASS AutoEditorDOMPointChildInvalidator final
 {
 public:
   explicit AutoEditorDOMPointChildInvalidator(EditorDOMPoint& aPoint)
     : mPoint(aPoint)
+    , mCanceled(false)
   {
     MOZ_ASSERT(aPoint.IsSetAndValid());
     Unused << mPoint.Offset();
   }
 
   ~AutoEditorDOMPointChildInvalidator()
   {
-    InvalidateChild();
+    if (!mCanceled) {
+      InvalidateChild();
+    }
   }
 
   /**
    * Manually, invalidate child of the given point.
    */
   void InvalidateChild()
   {
     mPoint.Set(mPoint.GetContainer(), mPoint.Offset());
   }
 
+  /**
+   * After calling Cancel(), mPoint won't be modified by the destructor.
+   */
+  void Cancel()
+  {
+    mCanceled = true;
+  }
+
 private:
   EditorDOMPoint& mPoint;
 
+  bool mCanceled;
+
   AutoEditorDOMPointChildInvalidator() = delete;
   AutoEditorDOMPointChildInvalidator(
     const AutoEditorDOMPointChildInvalidator& aOther) = delete;
   const AutoEditorDOMPointChildInvalidator& operator=(
     const AutoEditorDOMPointChildInvalidator& aOther) = delete;
 };
 
 } // namespace mozilla
--- a/editor/libeditor/HTMLEditRules.cpp
+++ b/editor/libeditor/HTMLEditRules.cpp
@@ -1796,23 +1796,35 @@ HTMLEditRules::WillInsertBreak(Selection
     AutoEditorDOMPointChildInvalidator lockOffset(atStartOfSelection);
     // Paragraphs: special rules to look for <br>s
     EditActionResult result = ReturnInParagraph(aSelection, *blockParent);
     if (NS_WARN_IF(result.Failed())) {
       return result.Rv();
     }
     *aHandled = result.Handled();
     *aCancel = result.Canceled();
-    // Fall through, we may not have handled it in ReturnInParagraph()
-  }
-
-  // If not already handled then do the standard thing
-  if (!(*aHandled)) {
-    *aHandled = true;
-    return InsertBRElement(aSelection, atStartOfSelection);
+    if (result.Handled()) {
+      // Now, atStartOfSelection may be invalid because the left paragraph
+      // may have less children than its offset.  For avoiding warnings of
+      // validation of EditorDOMPoint, we should not touch it anymore.
+      lockOffset.Cancel();
+      return NS_OK;
+    }
+    // Fall through, if ReturnInParagraph() didn't handle it.
+    MOZ_ASSERT(!*aCancel, "ReturnInParagraph canceled this edit action, "
+                          "WillInsertBreak() needs to handle such case");
+  }
+
+  // If nobody handles this edit action, let's insert new <br> at the selection.
+  MOZ_ASSERT(!*aHandled, "Reached last resort of WillInsertBreak() "
+                         "after the edit action is handled");
+  nsresult rv = InsertBRElement(aSelection, atStartOfSelection);
+  *aHandled = true;
+  if (NS_WARN_IF(NS_FAILED(rv))) {
+    return rv;
   }
   return NS_OK;
 }
 
 nsresult
 HTMLEditRules::InsertBRElement(Selection& aSelection,
                                const EditorDOMPoint& aPointToBreak)
 {