Bug 1304624 ContentEventHandler::GetFlatTextLengthInRange() shouldn't include a line break length caused by the end node's open tag when the given range ends before the open tag r=smaug
authorMasayuki Nakano <masayuki@d-toybox.com>
Thu, 29 Sep 2016 14:04:15 +0900
changeset 315749 9ec418dcac3f0611f612bdad7d2fc0f0b23de043
parent 315748 e4e13b10667c36eeb0d24ad09f37851504977208
child 315750 d4e7f3917d2fbe4fe82495dc3d2d3b34ab109f34
push id20634
push usercbook@mozilla.com
push dateFri, 30 Sep 2016 10:10:13 +0000
treeherderfx-team@afe79b010d13 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssmaug
bugs1304624, 1213589
milestone52.0a1
Bug 1304624 ContentEventHandler::GetFlatTextLengthInRange() shouldn't include a line break length caused by the end node's open tag when the given range ends before the open tag r=smaug This must be a regression of bug 1213589. When removing whole text from an editor, the editor removes moz-<br> after removing all nodes. Immediately before the remove, moz-<br> becomes <br>. So, IMEContentObserver::AttributeChanged() detects the native text length for the <br> element changing from 0 to 2 (on Windows) or 1 (on the other platforms). However, the call of ContentEventHandler::GetFlatTextLengthInRange() in IMEContentObserver::AttributeChanged() returns 2 (on Windows) or 1 (on the other platforms). Although, it tries to get the length from the start of the editor to before aElement, it always includes the length of the line breaker caused by the end node. The reason is, ContentEventHandler::GetFlatTextLengthInRange() always adds the line breaker's length for the end node without checking the range includes open tag of the end node. Therefore, this patch adds the check into ContentEventHandler::GetFlatTextLengthInRange(). So, ContentEventHandler::GetFlatTextLengthInRange() becomes to support NodePositionBefore() for the end of the range. Additionally, this patch fixes a bug at appending text node length. In the loop, |aEndPosition| shouldn't be referred because it may be hacked with |endPosition|. So, detecting the last text node should be compared with |endPosition|. For making the method code safer, this changes whole |aEndPosition| after defining |endPosition| is replaced with |endPosition|. MozReview-Commit-ID: FUp2nxuQHnO
dom/events/ContentEventHandler.cpp
dom/events/ContentEventHandler.h
--- a/dom/events/ContentEventHandler.cpp
+++ b/dom/events/ContentEventHandler.cpp
@@ -303,18 +303,17 @@ ContentEventHandler::Init(WidgetQueryCon
     if (composition) {
       uint32_t compositionStart = composition->NativeOffsetOfStartComposition();
       if (NS_WARN_IF(!aEvent->mInput.MakeOffsetAbsolute(compositionStart))) {
         return NS_ERROR_FAILURE;
       }
     } else {
       LineBreakType lineBreakType = GetLineBreakType(aEvent);
       uint32_t selectionStart = 0;
-      rv = GetFlatTextLengthBefore(mFirstSelectedRange,
-                                   &selectionStart, lineBreakType);
+      rv = GetStartOffset(mFirstSelectedRange, &selectionStart, lineBreakType);
       if (NS_WARN_IF(NS_FAILED(rv))) {
         return NS_ERROR_FAILURE;
       }
       if (NS_WARN_IF(!aEvent->mInput.MakeOffsetAbsolute(selectionStart))) {
         return NS_ERROR_FAILURE;
       }
     }
   }
@@ -1304,18 +1303,18 @@ ContentEventHandler::OnQuerySelectedText
       !nsContentUtils::ContentIsDescendantOf(endNode, mRootContent)) {
     return NS_ERROR_NOT_AVAILABLE;
   }
 
   NS_ASSERTION(aEvent->mReply.mString.IsEmpty(),
                "The reply string must be empty");
 
   LineBreakType lineBreakType = GetLineBreakType(aEvent);
-  rv = GetFlatTextLengthBefore(mFirstSelectedRange,
-                               &aEvent->mReply.mOffset, lineBreakType);
+  rv = GetStartOffset(mFirstSelectedRange,
+                      &aEvent->mReply.mOffset, lineBreakType);
   NS_ENSURE_SUCCESS(rv, rv);
 
   nsCOMPtr<nsINode> anchorNode, focusNode;
   int32_t anchorOffset, focusOffset;
   if (mSelection->RangeCount()) {
     // If there is only one selection range, the anchor/focus node and offset
     // are the information of the range.  Therefore, we have the direction
     // information.
@@ -2435,18 +2434,18 @@ ContentEventHandler::OnQueryCaretRect(Wi
 
   // When the selection is collapsed and the queried offset is current caret
   // position, we should return the "real" caret rect.
   if (mSelection->IsCollapsed()) {
     nsRect caretRect;
     nsIFrame* caretFrame = nsCaret::GetGeometry(mSelection, &caretRect);
     if (caretFrame) {
       uint32_t offset;
-      rv = GetFlatTextLengthBefore(mFirstSelectedRange,
-                                   &offset, GetLineBreakType(aEvent));
+      rv = GetStartOffset(mFirstSelectedRange,
+                          &offset, GetLineBreakType(aEvent));
       NS_ENSURE_SUCCESS(rv, rv);
       if (offset == aEvent->mInput.mOffset) {
         rv = ConvertToRootRelativeOffset(caretFrame, caretRect);
         NS_ENSURE_SUCCESS(rv, rv);
         nscoord appUnitsPerDevPixel =
           caretFrame->PresContext()->AppUnitsPerDevPixel();
         aEvent->mReply.mRect = LayoutDeviceIntRect::FromUnknownRect(
           caretRect.ToOutsidePixels(appUnitsPerDevPixel));
@@ -2693,71 +2692,72 @@ ContentEventHandler::GetFlatTextLengthIn
     *aLength = 0;
     return NS_OK;
   }
 
   // Don't create nsContentIterator instance until it's really necessary since
   // destroying without initializing causes unexpected NS_ASSERTION() call.
   nsCOMPtr<nsIContentIterator> iter;
 
+  // Working with ContentIterator, we may need to adjust the end position for
+  // including it forcibly.
+  NodePosition endPosition(aEndPosition);
+
   // This may be called for retrieving the text of removed nodes.  Even in this
   // case, the node thinks it's still in the tree because UnbindFromTree() will
   // be called after here.  However, the node was already removed from the
   // array of children of its parent.  So, be careful to handle this case.
   if (aIsRemovingNode) {
     DebugOnly<nsIContent*> parent = aStartPosition.mNode->GetParent();
     MOZ_ASSERT(parent && parent->IndexOf(aStartPosition.mNode) == -1,
       "At removing the node, the node shouldn't be in the array of children "
       "of its parent");
-    MOZ_ASSERT(aStartPosition.mNode == aEndPosition.mNode,
+    MOZ_ASSERT(aStartPosition.mNode == endPosition.mNode,
       "At removing the node, start and end node should be same");
     MOZ_ASSERT(aStartPosition.mOffset == 0,
       "When the node is being removed, the start offset should be 0");
-    MOZ_ASSERT(static_cast<uint32_t>(aEndPosition.mOffset) ==
-                 aEndPosition.mNode->GetChildCount(),
+    MOZ_ASSERT(static_cast<uint32_t>(endPosition.mOffset) ==
+                 endPosition.mNode->GetChildCount(),
       "When the node is being removed, the end offset should be child count");
     iter = NS_NewPreContentIterator();
     nsresult rv = iter->Init(aStartPosition.mNode);
     if (NS_WARN_IF(NS_FAILED(rv))) {
       return rv;
     }
   } else {
     RefPtr<nsRange> prev = new nsRange(aRootContent);
     nsresult rv = aStartPosition.SetToRangeStart(prev);
     if (NS_WARN_IF(NS_FAILED(rv))) {
       return rv;
     }
 
     // When the end position is immediately after non-root element's open tag,
     // we need to include a line break caused by the open tag.
-    NodePosition endPosition;
-    if (aEndPosition.mNode != aRootContent &&
-        aEndPosition.IsImmediatelyAfterOpenTag()) {
-      if (aEndPosition.mNode->HasChildren()) {
-        // When the end node has some children, move the end position to the
-        // start of its first child.
-        nsINode* firstChild = aEndPosition.mNode->GetFirstChild();
+    if (endPosition.mNode != aRootContent &&
+        endPosition.IsImmediatelyAfterOpenTag()) {
+      if (endPosition.mNode->HasChildren()) {
+        // When the end node has some children, move the end position to before
+        // the open tag of its first child.
+        nsINode* firstChild = endPosition.mNode->GetFirstChild();
         if (NS_WARN_IF(!firstChild)) {
           return NS_ERROR_FAILURE;
         }
-        endPosition = NodePosition(firstChild, 0);
+        endPosition = NodePositionBefore(firstChild, 0);
       } else {
         // When the end node is empty, move the end position after the node.
-        nsIContent* parentContent = aEndPosition.mNode->GetParent();
+        nsIContent* parentContent = endPosition.mNode->GetParent();
         if (NS_WARN_IF(!parentContent)) {
           return NS_ERROR_FAILURE;
         }
-        int32_t indexInParent = parentContent->IndexOf(aEndPosition.mNode);
+        int32_t indexInParent = parentContent->IndexOf(endPosition.mNode);
         if (NS_WARN_IF(indexInParent < 0)) {
           return NS_ERROR_FAILURE;
         }
-        endPosition = NodePosition(parentContent, indexInParent + 1);
+        endPosition = NodePositionBefore(parentContent, indexInParent + 1);
       }
-    } else {
-      endPosition = aEndPosition;
     }
 
     if (endPosition.OffsetIsValid()) {
       // Offset is within node's length; set end of range to that offset
       rv = endPosition.SetToRangeEnd(prev);
       if (NS_WARN_IF(NS_FAILED(rv))) {
         return rv;
       }
@@ -2795,43 +2795,48 @@ ContentEventHandler::GetFlatTextLengthIn
     }
     if (!node->IsContent()) {
       continue;
     }
     nsIContent* content = node->AsContent();
 
     if (node->IsNodeOfType(nsINode::eTEXT)) {
       // Note: our range always starts from offset 0
-      if (node == aEndPosition.mNode) {
+      if (node == endPosition.mNode) {
         *aLength += GetTextLength(content, aLineBreakType,
-                                  aEndPosition.mOffset);
+                                  endPosition.mOffset);
       } else {
         *aLength += GetTextLength(content, aLineBreakType);
       }
     } else if (ShouldBreakLineBefore(content, aRootContent)) {
       // If the start position is start of this node but doesn't include the
       // open tag, don't append the line break length.
       if (node == aStartPosition.mNode && !aStartPosition.IsBeforeOpenTag()) {
         continue;
       }
+      // If the end position is before the open tag, don't append the line
+      // break length.
+      if (node == endPosition.mNode && endPosition.IsBeforeOpenTag()) {
+        continue;
+      }
       *aLength += GetBRLength(aLineBreakType);
     }
   }
   return NS_OK;
 }
 
 nsresult
-ContentEventHandler::GetFlatTextLengthBefore(nsRange* aRange,
-                                             uint32_t* aOffset,
-                                             LineBreakType aLineBreakType)
+ContentEventHandler::GetStartOffset(nsRange* aRange,
+                                    uint32_t* aOffset,
+                                    LineBreakType aLineBreakType)
 {
   MOZ_ASSERT(aRange);
   return GetFlatTextLengthInRange(
            NodePosition(mRootContent, 0),
-           NodePositionBefore(aRange->GetStartParent(), aRange->StartOffset()),
+           NodePosition(aRange->GetStartParent(), aRange->StartOffset()),
            mRootContent, aOffset, aLineBreakType);
 }
 
 nsresult
 ContentEventHandler::AdjustCollapsedRangeMaybeIntoTextNode(nsRange* aRange)
 {
   MOZ_ASSERT(aRange);
   MOZ_ASSERT(aRange->Collapsed());
--- a/dom/events/ContentEventHandler.h
+++ b/dom/events/ContentEventHandler.h
@@ -247,20 +247,22 @@ protected:
   // elements causes "\n" (or "\r\n"), see also ShouldBreakLineBefore().
   nsresult GenerateFlatTextContent(nsIContent* aContent,
                                    nsAFlatString& aString,
                                    LineBreakType aLineBreakType);
   // Get the contents of aRange as plain text.
   nsresult GenerateFlatTextContent(nsRange* aRange,
                                    nsAFlatString& aString,
                                    LineBreakType aLineBreakType);
-  // Get the text length before the start position of aRange.
-  nsresult GetFlatTextLengthBefore(nsRange* aRange,
-                                   uint32_t* aOffset,
-                                   LineBreakType aLineBreakType);
+  // Get offset of start of aRange.  Note that the result includes the length
+  // of line breaker caused by the start of aContent because aRange never
+  // includes the line breaker caused by its start node.
+  nsresult GetStartOffset(nsRange* aRange,
+                          uint32_t* aOffset,
+                          LineBreakType aLineBreakType);
   // Check if we should insert a line break before aContent.
   // This should return false only when aContent is an html element which
   // is typically used in a paragraph like <em>.
   static bool ShouldBreakLineBefore(nsIContent* aContent,
                                     nsINode* aRootNode);
   // Get the line breaker length.
   static inline uint32_t GetBRLength(LineBreakType aLineBreakType);
   static LineBreakType GetLineBreakType(WidgetQueryContentEvent* aEvent);