Bug 1460509 - part 46: Make HTMLEditRules::NormalizeSelection() return NS_ERROR_EDITOR_DESTROYED if it causes destroying the editor r?m_kato draft
authorMasayuki Nakano <masayuki@d-toybox.com>
Tue, 15 May 2018 18:59:27 +0900
changeset 798764 5f0c37e2f604b38cd2700fc881f00e982dfad088
parent 798763 721d4dddaa2a5a4af8898e08777f75b8b4dc2e44
child 798765 fbc062e17ec7663cf7ec23aa7c610af427b4d955
push id110840
push usermasayuki@d-toybox.com
push dateWed, 23 May 2018 13:41:58 +0000
reviewersm_kato
bugs1460509
milestone62.0a1
Bug 1460509 - part 46: Make HTMLEditRules::NormalizeSelection() return NS_ERROR_EDITOR_DESTROYED if it causes destroying the editor r?m_kato MozReview-Commit-ID: 5yf1ZJ5sQ1t
editor/libeditor/HTMLEditRules.cpp
editor/libeditor/HTMLEditRules.h
--- a/editor/libeditor/HTMLEditRules.cpp
+++ b/editor/libeditor/HTMLEditRules.cpp
@@ -6254,34 +6254,35 @@ HTMLEditRules::ExpandSelectionForDeletio
     if (NS_WARN_IF(NS_FAILED(rv))) {
       return rv;
     }
   }
 
   return NS_OK;
 }
 
-/**
- * NormalizeSelection() tweaks non-collapsed selections to be more "natural".
- * Idea here is to adjust selection endpoint so that they do not cross breaks
- * or block boundaries unless something editable beyond that boundary is also
- * selected.  This adjustment makes it much easier for the various block
- * operations to determine what nodes to act on.
- */
 nsresult
 HTMLEditRules::NormalizeSelection()
 {
   MOZ_ASSERT(IsEditorDataAvailable());
 
+  // NormalizeSelection() tweaks non-collapsed selections to be more "natural".
+  // Idea here is to adjust selection endpoint so that they do not cross breaks
+  // or block boundaries unless something editable beyond that boundary is also
+  // selected.  This adjustment makes it much easier for the various block
+  // operations to determine what nodes to act on.
+
   // don't need to touch collapsed selections
   if (SelectionRef().IsCollapsed()) {
     return NS_OK;
   }
 
-  // we don't need to mess with cell selections, and we assume multirange selections are those.
+  // We don't need to mess with cell selections, and we assume multirange
+  // selections are those.
+  // XXX Why?  Even in <input>, user can select 2 or more ranges.
   if (SelectionRef().RangeCount() != 1) {
     return NS_OK;
   }
 
   RefPtr<nsRange> range = SelectionRef().GetRangeAt(0);
   if (NS_WARN_IF(!range)) {
     return NS_ERROR_FAILURE;
   }
@@ -6308,22 +6309,22 @@ HTMLEditRules::NormalizeSelection()
   // some locals we need for whitespace code
   nsCOMPtr<nsINode> unused;
   int32_t offset = -1;
   WSType wsType;
 
   // let the whitespace code do the heavy lifting
   WSRunObject wsEndObj(&HTMLEditorRef(), endNode,
                        static_cast<int32_t>(endOffset));
-  // is there any intervening visible whitespace?  if so we can't push selection past that,
-  // it would visibly change maening of users selection
+  // Is there any intervening visible whitespace?  If so we can't push
+  // selection past that, it would visibly change meaning of users selection.
   wsEndObj.PriorVisibleNode(EditorRawDOMPoint(endNode, endOffset),
                             address_of(unused), &offset, &wsType);
   if (wsType != WSType::text && wsType != WSType::normalWS) {
-    // eThisBlock and eOtherBlock conveniently distinquish cases
+    // eThisBlock and eOtherBlock conveniently distinguish cases
     // of going "down" into a block and "up" out of a block.
     if (wsEndObj.mStartReason == WSType::otherBlock) {
       // endpoint is just after the close of a block.
       nsINode* child =
         HTMLEditorRef().GetRightmostChild(wsEndObj.mStartReasonNode, true);
       if (child) {
         int32_t offset = -1;
         newEndNode = EditorBase::GetNodeLocation(child, &offset);
@@ -6350,22 +6351,22 @@ HTMLEditRules::NormalizeSelection()
       newEndOffset = static_cast<uint32_t>(offset);;
     }
   }
 
 
   // similar dealio for start of range
   WSRunObject wsStartObj(&HTMLEditorRef(), startNode,
                          static_cast<int32_t>(startOffset));
-  // is there any intervening visible whitespace?  if so we can't push selection past that,
-  // it would visibly change maening of users selection
+  // Is there any intervening visible whitespace?  If so we can't push
+  // selection past that, it would visibly change meaning of users selection.
   wsStartObj.NextVisibleNode(EditorRawDOMPoint(startNode, startOffset),
                              address_of(unused), &offset, &wsType);
   if (wsType != WSType::text && wsType != WSType::normalWS) {
-    // eThisBlock and eOtherBlock conveniently distinquish cases
+    // eThisBlock and eOtherBlock conveniently distinguish cases
     // of going "down" into a block and "up" out of a block.
     if (wsStartObj.mEndReason == WSType::otherBlock) {
       // startpoint is just before the start of a block.
       nsINode* child =
         HTMLEditorRef().GetLeftmostChild(wsStartObj.mEndReasonNode, true);
       if (child) {
         int32_t offset = -1;
         newStartNode = EditorBase::GetNodeLocation(child, &offset);
@@ -6389,23 +6390,24 @@ HTMLEditRules::NormalizeSelection()
       int32_t offset = -1;
       newStartNode =
         EditorBase::GetNodeLocation(wsStartObj.mEndReasonNode, &offset);
       // offset *after* break
       newStartOffset = static_cast<uint32_t>(offset + 1);
     }
   }
 
-  // there is a demented possiblity we have to check for.  We might have a very strange selection
-  // that is not collapsed and yet does not contain any editable content, and satisfies some of the
-  // above conditions that cause tweaking.  In this case we don't want to tweak the selection into
-  // a block it was never in, etc.  There are a variety of strategies one might use to try to
-  // detect these cases, but I think the most straightforward is to see if the adjusted locations
-  // "cross" the old values: ie, new end before old start, or new start after old end.  If so
-  // then just leave things alone.
+  // There is a demented possiblity we have to check for.  We might have a very
+  // strange selection that is not collapsed and yet does not contain any
+  // editable content, and satisfies some of the above conditions that cause
+  // tweaking.  In this case we don't want to tweak the selection into a block
+  // it was never in, etc.  There are a variety of strategies one might use to
+  // try to detect these cases, but I think the most straightforward is to see
+  // if the adjusted locations "cross" the old values: i.e., new end before old
+  // start, or new start after old end.  If so then just leave things alone.
 
   int16_t comp;
   comp = nsContentUtils::ComparePoints(startNode, startOffset,
                                        newEndNode, newEndOffset);
   if (comp == 1) {
     return NS_OK;  // New end before old start.
   }
   comp = nsContentUtils::ComparePoints(newStartNode, newStartOffset,
@@ -6413,19 +6415,25 @@ HTMLEditRules::NormalizeSelection()
   if (comp == 1) {
     return NS_OK;  // New start after old end.
   }
 
   // otherwise set selection to new values.
   // XXX Why don't we use SetBaseAndExtent()?
   DebugOnly<nsresult> rv =
     SelectionRef().Collapse(newStartNode, newStartOffset);
+  if (NS_WARN_IF(!CanHandleEditAction())) {
+    return NS_ERROR_EDITOR_DESTROYED;
+  }
   NS_WARNING_ASSERTION(NS_SUCCEEDED(rv),
     "Failed to collapse selection");
   rv = SelectionRef().Extend(newEndNode, newEndOffset);
+  if (NS_WARN_IF(!CanHandleEditAction())) {
+    return NS_ERROR_EDITOR_DESTROYED;
+  }
   NS_WARNING_ASSERTION(NS_SUCCEEDED(rv),
     "Failed to extend selection");
   return NS_OK;
 }
 
 /**
  * GetPromotedPoint() figures out where a start or end point for a block
  * operation really is.
--- a/editor/libeditor/HTMLEditRules.h
+++ b/editor/libeditor/HTMLEditRules.h
@@ -465,17 +465,25 @@ protected:
                            IgnoreSingleBR aIgnoreSingleBR);
 
   nsresult CheckForEmptyBlock(nsINode* aStartNode, Element* aBodyNode,
                               nsIEditor::EDirection aAction, bool* aHandled);
   enum class BRLocation { beforeBlock, blockEnd };
   Element* CheckForInvisibleBR(Element& aBlock, BRLocation aWhere,
                                int32_t aOffset = 0);
   nsresult ExpandSelectionForDeletion();
-  nsresult NormalizeSelection();
+
+  /**
+   * NormalizeSelection() adjust Selection if it's not collapsed and there is
+   * only one range.  If range start and/or end point is <br> node or something
+   * non-editable point, they should be moved to nearest text node or something
+   * where the other methods easier to handle edit action.
+   */
+  MOZ_MUST_USE nsresult NormalizeSelection();
+
   EditorDOMPoint GetPromotedPoint(RulesEndpoint aWhere, nsINode& aNode,
                                   int32_t aOffset, EditAction actionID);
   void GetPromotedRanges(nsTArray<RefPtr<nsRange>>& outArrayOfRanges,
                          EditAction inOperationType);
   void PromoteRange(nsRange& aRange, EditAction inOperationType);
   enum class TouchContent { no, yes };
   MOZ_MUST_USE nsresult
   GetNodesForOperation(nsTArray<RefPtr<nsRange>>& aArrayOfRanges,