Bug 1286464 part.17 ContentEventHandler::OnQueryTextRect() should compute a line breaker's rect from the last text frame if the queried range starts with a block frame r?smaug draft
authorMasayuki Nakano <masayuki@d-toybox.com>
Mon, 08 Aug 2016 18:11:11 +0900
changeset 399846 6a560ccfb218d22959c537fc66300868c4b99a0a
parent 399845 efab9565c8fed755fbf253ee1d965fe9f2f20f52
child 399847 27a831c09d55c2a6f75c50e16977306c06fe7963
push id26007
push usermasayuki@d-toybox.com
push dateFri, 12 Aug 2016 06:30:02 +0000
reviewerssmaug
bugs1286464
milestone51.0a1
Bug 1286464 part.17 ContentEventHandler::OnQueryTextRect() should compute a line breaker's rect from the last text frame if the queried range starts with a block frame r?smaug When queried range starts with a block frame, we cannot compute proper line breaker's rect at its open tag only with the frame's rect because the proper rect should be at the end of previous text frame. Therefore, this patch makes SetRangeFromFlatTextOffset() return the last text node found at scanning the range. Then, OnQueryTextRect() can compute the first line breaker's rect with it (if there is at least one text node). However, if the last text node is hidden by CSS or something, this patch won't work. We need to do something in another bug for hidden frame's issue, though. But note that IME typically doesn't request hidden text's rect because neither selection nor composition string is in such range. MozReview-Commit-ID: 2FFzNoubJ1y
dom/events/ContentEventHandler.cpp
dom/events/ContentEventHandler.h
--- a/dom/events/ContentEventHandler.cpp
+++ b/dom/events/ContentEventHandler.cpp
@@ -948,21 +948,25 @@ ContentEventHandler::ExpandToClusterBoun
 }
 
 nsresult
 ContentEventHandler::SetRangeFromFlatTextOffset(nsRange* aRange,
                                                 uint32_t aOffset,
                                                 uint32_t aLength,
                                                 LineBreakType aLineBreakType,
                                                 bool aExpandToClusterBoundaries,
-                                                uint32_t* aNewOffset)
+                                                uint32_t* aNewOffset,
+                                                nsIContent** aLastTextNode)
 {
   if (aNewOffset) {
     *aNewOffset = aOffset;
   }
+  if (aLastTextNode) {
+    *aLastTextNode = nullptr;
+  }
 
   // Special case like <br contenteditable>
   if (!mRootContent->HasChildren()) {
     nsresult rv = aRange->SetStart(mRootContent, 0);
     if (NS_WARN_IF(NS_FAILED(rv))) {
       return rv;
     }
     rv = aRange->SetEnd(mRootContent, 0);
@@ -986,16 +990,21 @@ ContentEventHandler::SetRangeFromFlatTex
       break;
     }
     // FYI: mRootContent shouldn't cause any text. So, we can skip it simply.
     if (node == mRootContent || !node->IsContent()) {
       continue;
     }
     nsIContent* content = node->AsContent();
 
+    if (aLastTextNode && content->IsNodeOfType(nsINode::eTEXT)) {
+      NS_IF_RELEASE(*aLastTextNode);
+      NS_ADDREF(*aLastTextNode = content);
+    }
+
     uint32_t textLength =
       content->IsNodeOfType(nsINode::eTEXT) ?
         GetTextLength(content, aLineBreakType) :
         (ShouldBreakLineBefore(content, mRootContent) ?
            GetBRLength(aLineBreakType) : 0);
     if (!textLength) {
       continue;
     }
@@ -1925,19 +1934,21 @@ ContentEventHandler::OnQueryTextRect(Wid
   // If mLength is 0 (this may be caused by bug of native IME), we should
   // redirect this event to OnQueryCaretRect().
   if (!aEvent->mInput.mLength) {
     return OnQueryCaretRect(aEvent);
   }
 
   LineBreakType lineBreakType = GetLineBreakType(aEvent);
   RefPtr<nsRange> range = new nsRange(mRootContent);
+  nsCOMPtr<nsIContent> lastTextContent;
   rv = SetRangeFromFlatTextOffset(range, aEvent->mInput.mOffset,
                                   aEvent->mInput.mLength, lineBreakType, true,
-                                  &aEvent->mReply.mOffset);
+                                  &aEvent->mReply.mOffset,
+                                  getter_AddRefs(lastTextContent));
   NS_ENSURE_SUCCESS(rv, rv);
   rv = GenerateFlatTextContent(range, aEvent->mReply.mString, lineBreakType);
   NS_ENSURE_SUCCESS(rv, rv);
 
   // used to iterate over all contents and their frames
   nsCOMPtr<nsIContentIterator> iter = NS_NewContentIterator();
   iter->Init(range);
 
@@ -1947,43 +1958,99 @@ ContentEventHandler::OnQueryTextRect(Wid
   // If GetFirstFrameInRangeForTextRect() does not return valid frame,
   // that means that there is no remaining content which causes text.
   // So, in such case, we must have reached the end of the contents.
   if (!firstFrame.IsValid()) {
     // TODO: Handle this case later.
     return NS_ERROR_FAILURE;
   }
 
-  // For GetLineBreakerRectBefore() doesn't work well with block frame, so,
-  // this should fail if there is only block frame in the queried range for now.
-  // TODO: Fix later.
-  bool hasSetRect = false;
-
   nsRect rect, frameRect;
   nsPoint ptOffset;
 
-  // Get the starting frame rect if it's not a block frame.
+  // If the first frame is a text frame, the result should be computed with
+  // the frame's rect but not including the rect before start point of the
+  // queried range.
   if (firstFrame->GetType() == nsGkAtoms::textFrame) {
-    hasSetRect = true;
     rect.SetRect(nsPoint(0, 0), firstFrame->GetRect().Size());
     rv = ConvertToRootRelativeOffset(firstFrame, rect);
     if (NS_WARN_IF(NS_FAILED(rv))) {
       return rv;
     }
     frameRect = rect;
+    // Exclude the rect before start point of the queried range.
     firstFrame->GetPointFromOffset(firstFrame.mOffsetInNode, &ptOffset);
     if (firstFrame->GetWritingMode().IsVertical()) {
       rect.y += ptOffset.y;
       rect.height -= ptOffset.y;
     } else {
       rect.x += ptOffset.x;
       rect.width -= ptOffset.x;
     }
-  } else if (firstFrame->GetType() == nsGkAtoms::brFrame) {
-    hasSetRect = true;
+  }
+  // If first frame causes a line breaker but it's not a <br> frame, we cannot
+  // compute proper rect only with the frame because typically caret is at
+  // right of the last character of it.  For example, if caret is after "c" of
+  // |<p>abc</p><p>def</p>|, IME may query a line breaker's rect after "c".
+  // Then, if we compute it only with the 2nd <p>'s block frame, the result
+  // will be:
+  //  +-<p>--------------------------------+
+  //  |abc                                 |
+  //  +------------------------------------+
+  //
+  // I+-<p>--------------------------------+
+  //  |def                                 |
+  //  +------------------------------------+
+  // However, users expect popup windows of IME should be positioned at
+  // right-bottom of "c" like this:
+  //  +-<p>--------------------------------+
+  //  |abcI                                |
+  //  +------------------------------------+
+  //
+  //  +-<p>--------------------------------+
+  //  |def                                 |
+  //  +------------------------------------+
+  // Therefore, if the first frame isn't a <br> frame and there is a text
+  // node before the first node in the queried range, we should compute the
+  // first rect with the previous character's rect.
+  else if (firstFrame->GetType() != nsGkAtoms::brFrame && lastTextContent) {
+    int32_t length = static_cast<int32_t>(lastTextContent->Length());
+    if (NS_WARN_IF(length < 0)) {
+      return NS_ERROR_FAILURE;
+    }
+    nsIFrame* lastTextFrame = nullptr;
+    rv = GetFrameForTextRect(lastTextContent, length, true, &lastTextFrame);
+    if (NS_WARN_IF(NS_FAILED(rv)) || NS_WARN_IF(!lastTextFrame)) {
+      return NS_ERROR_FAILURE;
+    }
+    const nsRect kLastTextFrameRect = lastTextFrame->GetRect();
+    if (lastTextFrame->GetWritingMode().IsVertical()) {
+      // Below of the last text frame.
+      rect.SetRect(0, kLastTextFrameRect.height,
+                   kLastTextFrameRect.width, 0);
+    } else {
+      // Right of the last text frame (not bidi-aware).
+      rect.SetRect(kLastTextFrameRect.width, 0,
+                   0, kLastTextFrameRect.height);
+    }
+    rv = ConvertToRootRelativeOffset(lastTextFrame, rect);
+    if (NS_WARN_IF(NS_FAILED(rv))) {
+      return rv;
+    }
+    frameRect = rect;
+  }
+  // Otherwise, we need to compute the line breaker's rect only with the
+  // first frame's rect.  But this may be unexpected.  For example,
+  // |<div contenteditable>[<p>]abc</p></div>|.  In this case, caret is before
+  // "a", therefore, users expect the rect left of "a".  However, we don't
+  // have enough information about the next character here and this isn't
+  // usual case (e.g., IME typically tries to query the rect of "a" or caret
+  // rect for computing its popup position).  Therefore, we shouldn't do
+  // more complicated hack here unless we'll get some bug reports actually.
+  else {
     FrameRelativeRect relativeRect = GetLineBreakerRectBefore(firstFrame);
     if (NS_WARN_IF(!relativeRect.IsValid())) {
       return NS_ERROR_FAILURE;
     }
     rect = relativeRect.RectRelativeTo(firstFrame);
     rv = ConvertToRootRelativeOffset(firstFrame, rect);
     if (NS_WARN_IF(NS_FAILED(rv))) {
       return rv;
@@ -2027,17 +2094,16 @@ ContentEventHandler::OnQueryTextRect(Wid
             primaryFrame->GetType() == nsGkAtoms::brFrame) {
           frame = primaryFrame;
         }
       } while (!frame && !iter->IsDone());
       if (!frame) {
         break;
       }
     }
-    hasSetRect = true;
     if (frame->GetType() == nsGkAtoms::textFrame) {
       frameRect.SetRect(nsPoint(0, 0), frame->GetRect().Size());
     } else {
       MOZ_ASSERT(frame->GetType() == nsGkAtoms::brFrame);
       FrameRelativeRect relativeRect = GetLineBreakerRectBefore(frame);
       if (NS_WARN_IF(!relativeRect.IsValid())) {
         return NS_ERROR_FAILURE;
       }
@@ -2051,21 +2117,16 @@ ContentEventHandler::OnQueryTextRect(Wid
     // non-emtpy rect from the frame.
     EnsureNonEmptyRect(frameRect);
     if (frame != lastFrame) {
       // not last frame, so just add rect to previous result
       rect.UnionRect(rect, frameRect);
     }
   }
 
-  // TODO: We need to fix this case later.
-  if (NS_WARN_IF(!hasSetRect)) {
-    return NS_ERROR_FAILURE;
-  }
-
   // Get the ending frame rect.
   // FYI: If first frame and last frame are same, frameRect is already set
   //      to the rect excluding the text before the query range.
   if (firstFrame.mFrame != lastFrame.mFrame) {
     frameRect.SetRect(nsPoint(0, 0), lastFrame->GetRect().Size());
     rv = ConvertToRootRelativeOffset(lastFrame, frameRect);
     if (NS_WARN_IF(NS_FAILED(rv))) {
       return rv;
--- a/dom/events/ContentEventHandler.h
+++ b/dom/events/ContentEventHandler.h
@@ -268,17 +268,18 @@ protected:
   // Make the DOM range from the offset of FlatText and the text length.
   // If aExpandToClusterBoundaries is true, the start offset and the end one are
   // expanded to nearest cluster boundaries.
   nsresult SetRangeFromFlatTextOffset(nsRange* aRange,
                                       uint32_t aOffset,
                                       uint32_t aLength,
                                       LineBreakType aLineBreakType,
                                       bool aExpandToClusterBoundaries,
-                                      uint32_t* aNewOffset = nullptr);
+                                      uint32_t* aNewOffset = nullptr,
+                                      nsIContent** aLastTextNode = nullptr);
   // If the aRange isn't in text node but next to a text node, this method
   // modifies it in the text node.  Otherwise, not modified.
   nsresult AdjustCollapsedRangeMaybeIntoTextNode(nsRange* aCollapsedRange);
   // Find the first frame for the range and get the start offset in it.
   nsresult GetStartFrameAndOffset(const nsRange* aRange,
                                   nsIFrame*& aFrame,
                                   int32_t& aOffsetInFrame);
   // Convert the frame relative offset to be relative to the root frame of the