Applying the patches for bug 1257446 causes Google Japanese Input stops showing popup for input mode in empty line of HTML editor. STR: 1. Open a page which has a contenteditable editor, e.g., twitter.com, Gmail's composing window. 2. Set focus to it and turn on/off IME by "Kanji" key. I cannot reproduce this bug with <input> nor <textarea>.

Hmm, looks like that eQueryTextRectArray returns empty rect for <br> nodes...

Currently, ContentEventHandler::OnQueryTextRectArray() supports only text frames. That means that it doesn't support <br> nodes. This causes odd behavior in contentediable editor. It should support <br> frames in special path. Review commit: https://reviewboard.mozilla.org/r/64124/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/64124/

Non nsTextFrame returns error now. So we might have to return frame rect for it... I change reviewer to jfkthame because he reviews my previous code.

Rather than adding a special code path in OnQueryTextRectArray(), would it work to provide an override of GetCharacterRectsInRange() in BRFrame? That seems like it would be the most natural place to put this.

Comment on attachment 8770771 [details] Bug 1286464 part.0 Add eQueryTextRect tests for line breakers Hmm, I realized some bugs in ContentEventHandler::OnQueryTextRectArray(). First, it doesn't treat line breaks even in a text node. For example, it doesn't append *2* rects for a XP line breakers even on Windows. This causes a lot of odd behavior. Next, it and some other queries don't care line breaks which are caused by <br> *or* block level elements. I think that the latter problem really depends on ContentEventHandler's design. In other words, frames should not support GetCharacterRectsInRange() on the other frames. I'll retry to fix this bug with new approach.

I think that we need to fixing regressions of bug 1213589 and bug 1203871 in this bug.

I completely understand why does neither bug 1257446 nor bug 1286157 work well with normal size patches. There are two main reasons: 1. Bug 1286157 fix made ContentEventHandler generates line breaks before block level elements, like <p>. However, I didn't change querying text/caret rects code. Therefore, after such new line breaks, the offset computation may be wrong (not always, this is the reason why I didn't realize in bug 1286157). 2. Bug 1286157 added eQueryTextRectArray, but it's used only for composition string and composition string never has line breakers. Therefore, the event handler, ContentEventHandler::OnQueryTextRectArray() and nsTextFrame::GetCharacterRectsInRange() are not aware of line breaks, multiple text frames in a range case. With 18 patches, I rewrite ContentEventHandler's related stuff. The changes are big, but if it's possible, I'd like to land them in this cycle because this big fix is very important in Gmail.

Oops, the #2's bug# is bug 1203871, not bug 1286157.

Oh, and #1's bug# is bug 1213589, not bug 1286157 too! Sorry for the spam...

Just explain the patches well to ease reviewing (whoever will be review the patches) :)

Review commit: https://reviewboard.mozilla.org/r/68216/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/68216/

GetFirstFrameInRange() uses AdjustTextRectNode() which may return different node before retrieving the result frame. Therefore, the caller may need offset in the new node. Review commit: https://reviewboard.mozilla.org/r/68218/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/68218/

This patch makes the static methods members of ContentEventHandler because ContentEventHandler::NodePosition is useful for them. * nsINode* AdjustTextRectNode(nsINode*, int32_t&) -> NodePosition GetNodeHavingFlatText(nsINode* int32_t) * nsIFrame* GetFirstFrameInRange(nsRange*, int32_t&) -> FrameAndNodeOffset GetFirstFrameHavingFlatTextInRange(nsRange*) So, this patch avoids unclear in/out arguments of them. Review commit: https://reviewboard.mozilla.org/r/68220/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/68220/

Currently, ContentEventHandler::SetRangeFromFlatTextOffset() sets end point to before a line breaker when the end of queried range is between a set of native line breakers (i.e., "\r]\n" on Windows). This causes unexpected empty range (e.g., "[]\n") when it queries a text rect at [\r]\n. Therefore, it should select an XP line breaker in such case (i.e., the range should be "[\n]" when it queries "[\r]\n" or "\r[\n]"). Note that we don't need to do anything at setting selection start because it's always aligned to before the line breaker. Review commit: https://reviewboard.mozilla.org/r/68222/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/68222/

This can kill the duplicated code in a lot of places in ContentEventHandler. Review commit: https://reviewboard.mozilla.org/r/68224/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/68224/

ContentEventHandler::OnQueryTextRect() is now too complicated. So, we shouldn't duplicate similar code in OnQueryCaretRect(). When it needs to guess a caret rect from a character rect, we shouldn't compute character rect by itself. Review commit: https://reviewboard.mozilla.org/r/68226/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/68226/

When eQueryTextRect's query range length is 0 (this may be caused by native IME's bug), it should return caret rect. Therefore, it should redirect such query events to OnQueryCaretRect(). Such IME must want caret rect in such case. Review commit: https://reviewboard.mozilla.org/r/68228/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/68228/

Some elements causes a line break before itself. In such case, OnQueryTextRectArray() should append one or two rects for such line breakers. This patch also implements GetLineBreakerRectBefore(). It computes line breaker rect for the given frame. Even if it's a <br> frame, we cannot use its rect because <br> frame height is computed with line height but we need text's height (i.e., font height) and both height and width are 0 if it's in quirks mode and in non-empty line. Therefore, this patch computes it with frame's baseline, font's max-ascent and max-descent. Although, this patch computes a line breaker rect caused by a open tag of a block level element with block frame's rect. However, it shouldn't be used actually as far as possible. Following patches will kill the path to the hack. Review commit: https://reviewboard.mozilla.org/r/68230/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/68230/

If the query range starts middle of a line breaker, i.e., "\r[\n", ContentEventHandler::OnQueryTextRectArray() does not need to (in other words, should not) append same rect anymore. This patch checks if the range starts middle of a line breaker and in such case, skip to append same rect. Review commit: https://reviewboard.mozilla.org/r/68232/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/68232/

If line breakers in a text node works as is, e.g., |white-space: pre;| is specified, we need to handle line breakers in text frames too. This patch handles "\n" in text nodes same as line breakers caused by some elements. Review commit: https://reviewboard.mozilla.org/r/68234/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/68234/

nsTextFrame::GetCharacterRectsInRange() handles a character at the end offset of its content as in it. However, it causes odd result when the caller wants first text rect in the next nsTextFrame. E.g., if end of query range is at the next nsTextFrame's first character, currently, it returns the last character as in same line. So, it should stop handling next frame's first character as in it. Review commit: https://reviewboard.mozilla.org/r/68236/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/68236/

If it returns frames which don't cause text, the caller needs complicated loop. So, it should return only frames which causes some text. Review commit: https://reviewboard.mozilla.org/r/68238/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/68238/

This is hack for returning better rect for a line breaker caused by non-<br> element. E.g., if a block frame causes a line break, after the last visible character is the best position, i.e., there is |<p>ABC</p><p>DEF</p>| and the caret is after "C", querying next character's rect should be computed with "C"'s rect, rather than computed with next <p> element's border rect. Review commit: https://reviewboard.mozilla.org/r/68240/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/68240/

When the loop in ContentEventHandler::OnQueryTextRectArray() reaches at the end of contents, it should append a caret rect at the end of the contents. That helps deciding popup position of IME when caret is at the end of the contents. This patch guesses the caret rect with eQueryTextRect event (for avoiding the dependency of native caret position of eQueryCaretRect event handler). Review commit: https://reviewboard.mozilla.org/r/68242/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/68242/

Although, this patch is pretty big and complicated, but I have no idea to separate this since this rewrites all over the OnQueryTextRect() with new utility methods which are implemented for OnQueryTextRectArray(). Currently, OnQueryTextRect() doesn't support line breaks before block level elements. Therefore, in HTML editors, eQueryTextRect may not work fine if the range includes open tags of block level elements. This causes odd positioning of IME's candidate window in non-e10s mode. This implements ContentEventHandler::GetLastFrameHavingFlatTextInRange() which scans the given range from last to first. However, this hits wrong NS_ASSERTION() in nsContentIterator::Last(). mLast can be nullptr if there is no content but Init() returns NS_OK even in such case. I think that returning NS_OK is fine because it's not illegal case. Therefore this patch fixes the wrong assertion. Review commit: https://reviewboard.mozilla.org/r/68244/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/68244/

In plain text editor, moz-<br> element is appended for a placeholder of empty line when the text ends with a line breaker. Therefore, when we compute text rects, we need to handle it same as a normal <br> element even though it doesn't cause any text. Review commit: https://reviewboard.mozilla.org/r/68246/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/68246/

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. Review commit: https://reviewboard.mozilla.org/r/68248/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/68248/

Similar to OnQueryTextRect(), OnQueryTextRectArray() should compute a line breaker's rect with the last character when it's the first character in queried range and not caused by <br> frame. Review commit: https://reviewboard.mozilla.org/r/68250/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/68250/

First, when the first node causing text is invisible, OnQueryTextRect() still fails even with this patch. We shouldn't fix it in this bug because it's unusual case but this bug is very important especially for some web service using HTML editor like Gmail. This patch fixes all cases when the start offset of queried range reaches the end of mRootContent. 1. When the last node causing text is a <br> element (either content <br> element or moz-<br> element), its frame is a placeholder for empty line. Therefore, this patch sets the rect to the frame rect. 2. When the last node causing text is a text node, the last frame generated for it represents its line (including empty line). Therefore, this patch sets the rect to the result of GetLineBreakerRectAfter(). 3. When the last node causes a line breaker before it, the frame may be a placeholder for it (this is not usual case, when user types Enter key at the end of <p> element, <p><br></p> is generated by Gecko). In this case, this patch sets a possible caret rect which is guessed from the content box of the frame and its font height. 4. When there are no nodes causing text in mRootContent, this patch sets a possible caret rect like case #3. Review commit: https://reviewboard.mozilla.org/r/68252/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/68252/

Comment on attachment 8770771 [details] Bug 1286464 part.0 Add eQueryTextRect tests for line breakers Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64124/diff/1-2/

Comment on attachment 8776476 [details] Bug 1286464 part.11 nsTextFrame::GetCharacterRectsInRange() shouldn't compute character rect at the first character in next nsTextFrame https://reviewboard.mozilla.org/r/68236/#review65310

Comment on attachment 8776466 [details] Bug 1286464 part.1 Cleaning up ContentEventHandler::OnQueryTextRectArray() https://reviewboard.mozilla.org/r/68216/#review65554

Comment on attachment 8770771 [details] Bug 1286464 part.0 Add eQueryTextRect tests for line breakers Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64124/diff/2-3/

Comment on attachment 8776466 [details] Bug 1286464 part.1 Cleaning up ContentEventHandler::OnQueryTextRectArray() Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68216/diff/1-2/

Comment on attachment 8776467 [details] Bug 1286464 part.2 GetFirstFrameInRange() should return node offset since it may return different node's frame Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68218/diff/1-2/

Comment on attachment 8776468 [details] Bug 1286464 part.3 Make static methods, AdjustTextRectNode() and GetFirstFrameInRange(), members of ContentEventHandler Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68220/diff/1-2/

Comment on attachment 8776469 [details] Bug 1286464 part.4 ContentEventHandler::SetRangeFromFlatTextOffset() should set end of the range to after a line break when the range is end between a set of native line breakers Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68222/diff/1-2/

Comment on attachment 8776470 [details] Bug 1286464 part.5 Create ContentEventHandler::EnsureNonEmptyRect() for query various rect event handlers Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68224/diff/1-2/

Comment on attachment 8776471 [details] Bug 1286464 part.6 ContentEventHandler::OnQueryCaretRect() should use eQueryTextRect event when it needs to guess the caret rect Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68226/diff/1-2/

Comment on attachment 8776472 [details] Bug 1286464 part.7 ContentEventHandler::OnQueryTextRect() should redirect the query event to OnQueryCaretRect() if its query range is empty Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68228/diff/1-2/

Comment on attachment 8776473 [details] Bug 1286464 part.8 ContentEventHandler::OnQueryTextRectArray() should handle line break before a node Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68230/diff/1-2/

Comment on attachment 8776474 [details] Bug 1286464 part.9 ContentEventHandler::OnQueryTextRectArray() shouldn't append same rect for following character of a lien breaker when the query range starts from middle of the line breaker Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68232/diff/1-2/

Comment on attachment 8776475 [details] Bug 1286464 part.10 ContentEventHandler::OnQueryTextRectArray() should append same rects for following characters of a line breaker in nsTextFrame Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68234/diff/1-2/

Comment on attachment 8776476 [details] Bug 1286464 part.11 nsTextFrame::GetCharacterRectsInRange() shouldn't compute character rect at the first character in next nsTextFrame Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68236/diff/1-2/

Comment on attachment 8776477 [details] Bug 1286464 part.12 ContentEventHandler::GetFirstFrameHavingFlatTextInRange() should return only frames whose content causes text Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68238/diff/1-2/

Comment on attachment 8776478 [details] Bug 1286464 part.13 ContentEventHandler::OnQueryTextRectArray() should guess a line breaker's rect with previous character's rect if it's available Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68240/diff/1-2/

Comment on attachment 8776479 [details] Bug 1286464 part.14 When ContentEventHandler::OnQueryTextRectArray() reaches the end of query range, it should append caret rect at the end of the content Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68242/diff/1-2/

Comment on attachment 8776480 [details] Bug 1286464 part.15 Rewrite ContentEventHandler::OnQueryTextRect() with new utilities like ContentEventHandler::OnQueryTextRectArray() Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68244/diff/1-2/

Comment on attachment 8776481 [details] Bug 1286464 part.16 Rename ContentEventHandler::Get*FrameHavingFlatTextInRange() to ContentEventHandler::Get*FrameInRangeForTextRect() and make them treat a moz-<br> element as a normal <br> element because it doesn't cause text but needs to compute text Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68246/diff/1-2/

Comment on attachment 8776482 [details] 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 Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68248/diff/1-2/

Comment on attachment 8776483 [details] Bug 1286464 part.18 ContentEventHandler::OnQueryTextRectArray() should compute line breaker's rect with the last character's node when the start of queried range is a line breaker and it's caused by a block frame Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68250/diff/1-2/

Comment on attachment 8776484 [details] Bug 1286464 part.19 ContentEventHandler::OnQueryTextRect() should handle the case when queried range starts from the end of mRootContent Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68252/diff/1-2/

Fixed some nits in part.14 and part.16.

Comment on attachment 8776467 [details] Bug 1286464 part.2 GetFirstFrameInRange() should return node offset since it may return different node's frame https://reviewboard.mozilla.org/r/68218/#review65580

Comment on attachment 8770771 [details] Bug 1286464 part.0 Add eQueryTextRect tests for line breakers https://reviewboard.mozilla.org/r/64124/#review65754

https://reviewboard.mozilla.org/r/68220/#review65758 ::: dom/events/ContentEventHandler.h:309 (Diff revision 2) > nsresult QueryTextRectByRange(nsRange* aRange, > LayoutDeviceIntRect& aRect, > WritingMode& aWritingMode); > + > + // Returns a node and position in the node for computing text rect. > + NodePosition GetNodeHavingFlatText(const NodePosition& aNodePosition); I don't quite understand what these methods are for. The name says they return a node, but they actually return node position

Comment on attachment 8776472 [details] Bug 1286464 part.7 ContentEventHandler::OnQueryTextRect() should redirect the query event to OnQueryCaretRect() if its query range is empty https://reviewboard.mozilla.org/r/68228/#review65760

Comment on attachment 8776469 [details] Bug 1286464 part.4 ContentEventHandler::SetRangeFromFlatTextOffset() should set end of the range to after a line break when the range is end between a set of native line breakers https://reviewboard.mozilla.org/r/68222/#review65762

https://reviewboard.mozilla.org/r/68220/#review65758 > I don't quite understand what these methods are for. The name says they return a node, but they actually return node position They return proper node and offset for computing text rect. For example: <p> <br> abc<br> </p> If the given node position is: {<p>, 0}, it returns {<br>, 0} {<p>, 1}, it returns {#text, 0} {<p>, 4}, it returns {<br>, 1} In the following patch, it may return {<p>, 0} for computing a line breaker caused by <p>. So, their purpose isn't returning a leaf node. Simply, GetNodePositionHavingFlatText() is better?

yes, the name talking about NodePosition would be better.

Comment on attachment 8776468 [details] Bug 1286464 part.3 Make static methods, AdjustTextRectNode() and GetFirstFrameInRange(), members of ContentEventHandler https://reviewboard.mozilla.org/r/68220/#review66386 ::: dom/events/ContentEventHandler.h:313 (Diff revision 2) > + // Returns a node and position in the node for computing text rect. > + NodePosition GetNodeHavingFlatText(const NodePosition& aNodePosition); > + NodePosition GetNodeHavingFlatText(nsINode* aNode, > + int32_t aNodeOffset); > + > + struct FrameAndNodeOffset final This should have the annotation that it is used only on stack, since having a raw pointer to mFrame isn't exactly safe. mFrame needs also some comment why it is safe. ::: dom/events/ContentEventHandler.cpp:1388 (Diff revision 2) > + > +ContentEventHandler::NodePosition > +ContentEventHandler::GetNodeHavingFlatText(nsINode* aNode, > + int32_t aNodeOffset) > +{ > + NodePosition result(aNode, aNodeOffset); I don't understand why result is initiated with aNode and aNodeOffset. Wouldn't nullptr and -1 make more sense? ::: dom/events/ContentEventHandler.cpp:1395 (Diff revision 2) > if (childCount) { > if (aNodeOffset < childCount) { > - node = aNode->GetChildAt(aNodeOffset); > - aNodeOffset = 0; > + result.mNode = aNode->GetChildAt(aNodeOffset); > + result.mOffset = 0; > } else if (aNodeOffset == childCount) { > - node = aNode->GetChildAt(childCount - 1); > + result.mNode = aNode->GetChildAt(childCount - 1); I know you just change the name of the method here, but why only last child is special cased? GetNodeHavingFlatText (or GetNodePositionHavingFlatText) needs some good documentation why if offset == childcount, it does something else than otherwise. ::: dom/events/ContentEventHandler.cpp:1414 (Diff revision 2) > > // get the starting frame > - aNodeOffset = aRange->StartOffset(); > - nsINode* node = iter->GetCurrentNode(); > - if (!node) { > - node = AdjustTextRectNode(aRange->GetStartParent(), aNodeOffset); > + NodePosition nodePosition(iter->GetCurrentNode(), aRange->StartOffset()); > + if (!nodePosition.mNode) { > + nodePosition = > + GetNodeHavingFlatText(aRange->GetStartParent(), nodePosition.mOffset); So if GetNodePositionHavingFlatText could return null node, we need then a null check here.

Comment on attachment 8776470 [details] Bug 1286464 part.5 Create ContentEventHandler::EnsureNonEmptyRect() for query various rect event handlers https://reviewboard.mozilla.org/r/68224/#review66394 ::: dom/events/ContentEventHandler.cpp (Diff revision 2) > rv = ConvertToRootRelativeOffset(firstFrame, rect); > NS_ENSURE_SUCCESS(rv, rv); > nsRect frameRect = rect; > nsPoint ptOffset; > firstFrame->GetPointFromOffset(startNodePosition.mOffset, &ptOffset); > - // minus 1 to avoid creating an empty rect Now I don't understand the old code, but new one looks ok :) ::: dom/events/ContentEventHandler.cpp:1588 (Diff revision 2) > } > } > frameRect.SetRect(nsPoint(0, 0), frame->GetRect().Size()); > rv = ConvertToRootRelativeOffset(frame, frameRect); > NS_ENSURE_SUCCESS(rv, rv); > + EnsureNonEmptyRect(frameRect); All these EnsureNonEmptyRect calls could use some comment that why they are needed.

Comment on attachment 8776471 [details] Bug 1286464 part.6 ContentEventHandler::OnQueryCaretRect() should use eQueryTextRect event when it needs to guess the caret rect https://reviewboard.mozilla.org/r/68226/#review66396 nice simplification

https://reviewboard.mozilla.org/r/68230/#review66402 ::: dom/events/ContentEventHandler.h:340 (Diff revision 2) > bool IsValid() const { return mFrame && mStartOffsetInNode >= 0; } > }; > // Get first frame in the given range for computing text rect. > FrameAndNodeOffset GetFirstFrameHavingFlatTextInRange(nsRange* aRange); > > + struct FrameRelativeRect final This needs to be marked to be stack only class ::: dom/events/ContentEventHandler.h:371 (Diff revision 2) > + }; > + > + // Returns a rect for line breaker before the node of aFrame. > + // Note that this doesn't check if aFrame should cause line break in > + // non-debug build. > + FrameRelativeRect GetLineBreakerRectBefore(nsIFrame* aFrame); It is still unclear what this is needed for. Some random line break before nsIFrame, but why? Could you improve the comment. ::: dom/events/ContentEventHandler.cpp:1466 (Diff revision 2) > + if (aFrame->GetType() != nsGkAtoms::brFrame && aFrame->GetParent()) { > + frameForFontMetrics = aFrame->GetParent(); > + } > + > + // Note that <br> element's rect is decided with line-height but we need > + // a rect without leading. Additionally, <br> frame's width and height what does "leading" mean here? ::: dom/events/ContentEventHandler.cpp:1492 (Diff revision 2) > + } else { > + result.mRect.y = baseline - fontMetrics->MaxAscent(); > + result.mRect.height = fontMetrics->MaxHeight(); > + } > + > + // If aFrame isn't a <br> frame, caret should be at outside of it because I don't understand this. Why should caret be outside, if we're dealing with textframe? There is the assertion for ShouldBreakLineBefore, but please add also some comment here that we're dealing with elements here. ::: dom/events/ContentEventHandler.cpp:1605 (Diff revision 2) > + break; > + } > + > + // TODO: If the query range is stated between a line breaker, i.e., \r[\n, > + // We shouldn't append a rect here. > + aEvent->mReply.mRectArray.AppendElement(rect); I don't understand this at all. We have already appended rect to the array. But ok, there is that todo. I assume some other patch will explain this.

Comment on attachment 8776473 [details] Bug 1286464 part.8 ContentEventHandler::OnQueryTextRectArray() should handle line break before a node https://reviewboard.mozilla.org/r/68230/#review66418

Comment on attachment 8776474 [details] Bug 1286464 part.9 ContentEventHandler::OnQueryTextRectArray() shouldn't append same rect for following character of a lien breaker when the query range starts from middle of the line breaker https://reviewboard.mozilla.org/r/68232/#review66424 So explain and ask review again. The code needs also some more comments. ::: dom/events/ContentEventHandler.cpp:1625 (Diff revision 2) > + if (startsBetweenLineBreaker) { > + continue; > + } > + > + // Let's append same rect for following character in a line breaker. > aEvent->mReply.mRectArray.AppendElement(rect); ok, I still don't understand why we add the same rect again.

Comment on attachment 8776475 [details] Bug 1286464 part.10 ContentEventHandler::OnQueryTextRectArray() should append same rects for following characters of a line breaker in nsTextFrame https://reviewboard.mozilla.org/r/68234/#review66426 In general I need some more explanation what is happening in this patch. ::: dom/events/ContentEventHandler.cpp:1590 (Diff revision 2) > if (NS_WARN_IF(NS_FAILED(rv)) || NS_WARN_IF(charRects.IsEmpty())) { > // XXX: If the node isn't a text node and does not cause a line break, > // we need to recompute with new range, but how? > return rv; > } > + AppendSubString(chars, firstContent, firstFrame.mStartOffsetInNode, This needs some comment. what are we appending here? ::: dom/events/ContentEventHandler.cpp:1597 (Diff revision 2) > + if (NS_WARN_IF(chars.Length() != charRects.Length())) { > + return NS_ERROR_UNEXPECTED; > + } > + if (kBRLength > 1 && chars[0] == '\n' && > + offset == aEvent->mInput.mOffset && offset) { > + // If start of range staring from previous offset of query range is staring?

Comment on attachment 8776477 [details] Bug 1286464 part.12 ContentEventHandler::GetFirstFrameHavingFlatTextInRange() should return only frames whose content causes text https://reviewboard.mozilla.org/r/68238/#review66428

Comment on attachment 8776478 [details] Bug 1286464 part.13 ContentEventHandler::OnQueryTextRectArray() should guess a line breaker's rect with previous character's rect if it's available https://reviewboard.mozilla.org/r/68240/#review66432 Please explain ::: dom/events/ContentEventHandler.cpp:1613 (Diff revision 2) > + // following for loop but we need the rect in the previous frame. > + // So, we need to avoid using current frame position. > + brRect = lastCharRect - frameRect.TopLeft(); > + if (!wasLineBreaker) { > + if (firstFrame->GetWritingMode().IsVertical()) { > + brRect.y = brRect.YMost() + 1; why +1? ::: dom/events/ContentEventHandler.cpp:1616 (Diff revision 2) > + if (!wasLineBreaker) { > + if (firstFrame->GetWritingMode().IsVertical()) { > + brRect.y = brRect.YMost() + 1; > + brRect.height = 0; > + } else { > + brRect.x = brRect.XMost() + 1; ditto ::: dom/events/ContentEventHandler.cpp:1617 (Diff revision 2) > + if (firstFrame->GetWritingMode().IsVertical()) { > + brRect.y = brRect.YMost() + 1; > + brRect.height = 0; > + } else { > + brRect.x = brRect.XMost() + 1; > + brRect.width = 0; why height or width is 0? Shouldn't they be 1, per that other stuff ensuring that rects aren't empty?

Comment on attachment 8776479 [details] Bug 1286464 part.14 When ContentEventHandler::OnQueryTextRectArray() reaches the end of query range, it should append caret rect at the end of the content https://reviewboard.mozilla.org/r/68242/#review66438 ::: dom/events/ContentEventHandler.cpp:1714 (Diff revision 2) > offset++; > } > } > + > + // If the query range is longer than actual content length, we should append > + // caret rect after the last character. Note that this method hasn't Why we should append caret rect in that case? Could you improve the comment a bit

I'd like to see also a patch containing all the changes, as an attachment here in bugzilla. It is hard to see the big picture from smaller patches (even though they ease reviewing)

Comment on attachment 8776482 [details] 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 https://reviewboard.mozilla.org/r/68248/#review66458 I don't quite understand this. ::: dom/events/ContentEventHandler.cpp:996 (Diff revision 2) > if (node == mRootContent || !node->IsContent()) { > continue; > } > nsIContent* content = node->AsContent(); > > + if (aLastTextNode && content->IsNodeOfType(nsINode::eTEXT)) { this manual addref/release isn't nice, but given that this method is super complicated, I don't see better way (except if the method is somehow split to smaller pieces). ::: dom/events/ContentEventHandler.cpp:1926 (Diff revision 2) > rect.x += ptOffset.x; > rect.width -= ptOffset.x; > } > - } else if (firstFrame->GetType() == nsGkAtoms::brFrame) { > - hasSetRect = true; > + } else if (firstFrame->GetType() != nsGkAtoms::brFrame && lastTextContent) { > + // If the node isn't <br> frame but there is a text node, we should > + // compute the line break rect with the last character. Why? Can we not have <span>some text</span> here, where last textContent points to that 'some text'. Please explain

Comment on attachment 8776481 [details] Bug 1286464 part.16 Rename ContentEventHandler::Get*FrameHavingFlatTextInRange() to ContentEventHandler::Get*FrameInRangeForTextRect() and make them treat a moz-<br> element as a normal <br> element because it doesn't cause text but needs to compute text https://reviewboard.mozilla.org/r/68246/#review66464 Please explain "Why IsMozBR isn't called in ShouldBreakLineBefore", with that, r+ ::: dom/events/ContentEventHandler.h:341 (Diff revision 2) > }; > // Get first frame after the start of the given range for computing text rect. > // This returns invalid FrameAndNodeOffset if there is no content which > - // causes text in the range. mOffsetInNode is start offset in the frame. > - FrameAndNodeOffset GetFirstFrameHavingFlatTextInRange(nsRange* aRange); > + // should affect to computing text rect in the range. mOffsetInNode is start > + // offset in the frame. > + FrameAndNodeOffset GetFirstFrameInRangeForTextRect(nsRange* aRange); I don't quite like *ForTextRect but I don't have better suggestions either. ::: dom/events/ContentEventHandler.cpp:1468 (Diff revision 2) > } > > // If the element node causes a line break before it, it's the first > // node causing text. > - if (ShouldBreakLineBefore(node->AsContent(), mRootContent)) { > + if (ShouldBreakLineBefore(node->AsContent(), mRootContent) || > + IsMozBR(node->AsContent())) { Why IsMozBR isn't called in ShouldBreakLineBefore?

Comment on attachment 8776483 [details] Bug 1286464 part.18 ContentEventHandler::OnQueryTextRectArray() should compute line breaker's rect with the last character's node when the start of queried range is a line breaker and it's caused by a block frame https://reviewboard.mozilla.org/r/68250/#review66468 Because of that, I don't think I understand this well enough. ::: dom/events/ContentEventHandler.cpp:1655 (Diff revision 2) > + int32_t length = static_cast<int32_t>(aTextContent->Length()); > + if (NS_WARN_IF(length < 0)) { > + return result; > + } > + nsIFrame* lastTextFrame = nullptr; > + nsresult rv = GetFrameForTextRect(aTextContent, length, true, &lastTextFrame); I don't understand why this returns lastTextFrame. Or is lastTextFrame just wrong name?

Comment on attachment 8776484 [details] Bug 1286464 part.19 ContentEventHandler::OnQueryTextRect() should handle the case when queried range starts from the end of mRootContent https://reviewboard.mozilla.org/r/68252/#review66470 ::: dom/events/ContentEventHandler.h:243 (Diff revision 2) > // the given line break type. > static uint32_t GetTextLengthInRange(nsIContent* aContent, > uint32_t aXPStartOffset, > uint32_t aXPEndOffset, > LineBreakType aLineBreakType); > + // Get the contents in aContent (meaning all children of aContent) as plain Please explain how this is different to .textContent ::: dom/events/ContentEventHandler.h:246 (Diff revision 2) > uint32_t aXPEndOffset, > LineBreakType aLineBreakType); > + // Get the contents in aContent (meaning all children of aContent) as plain > + // text. E.g., specifying mRootContent gets whole text in it. > + nsresult GenerateFlatTextContent(nsIContent* aContent, > + nsAFlatString& aString, Why is this nsAFlatString&? Why not just normal nsAString&? I guess because the existing GenerateFlatTextContent uses that too. But why does it do that? ::: dom/events/ContentEventHandler.cpp:1697 (Diff revision 2) > + const WritingMode kWritingMode = aFrame->GetWritingMode(); > + RefPtr<nsFontMetrics> fontMetrics = > + nsLayoutUtils::GetInflatedFontMetricsForFrame(aFrame); > + const nscoord kMaxHeight = > + fontMetrics ? fontMetrics->MaxHeight() : > + 16 * mPresContext->AppUnitsPerDevPixel(); This magical 16 needs some explanation ::: dom/events/ContentEventHandler.cpp:1706 (Diff revision 2) > + caretRect.y = kContentRect.y; > + if (!kWritingMode.IsVertical()) { > + if (kWritingMode.IsBidiLTR()) { > + caretRect.x = kContentRect.x; > + } else { > + caretRect.x = kContentRect.XMost() - mPresContext->AppUnitsPerDevPixel(); I don't understand - mPresContext->AppUnitsPerDevPixel() ::: dom/events/ContentEventHandler.cpp:1765 (Diff revision 2) > - // So, in such case, we must have reached the end of the contents. > + // the end of contents. > if (!firstFrame.IsValid()) { > + nsAutoString allText; > + rv = GenerateFlatTextContent(mRootContent, allText, lineBreakType); > + // If we meet invisible node, make this query fail for now. > + if (NS_WARN_IF(NS_FAILED(rv)) || offset < allText.Length()) { Why offset < allText.Length() is error? ::: dom/events/ContentEventHandler.cpp:1995 (Diff revision 2) > if (!firstFrame.IsValid()) { > - // TODO: Handle this case later. > + nsAutoString allText; > + rv = GenerateFlatTextContent(mRootContent, allText, lineBreakType); > + // If there are no visible nodes in the range, make this query fail for now. > + if (NS_WARN_IF(NS_FAILED(rv)) || > + static_cast<uint32_t>(aEvent->mInput.mOffset) < allText.Length()) { Also here. I don't understand the <. I would have expected >= I'm clearly misunderstanding something here, which hints that the code needs more comments.

Comment on attachment 8776480 [details] Bug 1286464 part.15 Rewrite ContentEventHandler::OnQueryTextRect() with new utilities like ContentEventHandler::OnQueryTextRectArray() https://reviewboard.mozilla.org/r/68244/#review66492 This is so super complicated that this will need couple of iterations, even with just minor changes. Need to just re-read this so many times. ::: dom/events/ContentEventHandler.cpp:1488 (Diff revision 2) > + iter->Init(aRange); > + > + nsINode* endNode = aRange->GetEndParent(); > + uint32_t endOffset = static_cast<uint32_t>(aRange->EndOffset()); > + // If the end point is start of a text node or specified by its parent and > + // index, the node shouldn't be included into the range. "If the end point is start of a text node...the node shouldn't be included into the range " Why not? ::: dom/events/ContentEventHandler.cpp:1510 (Diff revision 2) > + continue; > + } > + > + if (node->IsNodeOfType(nsINode::eTEXT)) { > + if (node == aRange->GetEndParent()) { > + nodePosition.mNode = node; Move nodePosition.mNode = node; to be right after if (node->IsNodeOfType(nsINode::eTEXT)) { ::: dom/events/ContentEventHandler.cpp:1894 (Diff revision 2) > - NS_ENSURE_SUCCESS(rv, rv); > - nsRect frameRect = rect; > + bool hasSetRect = false; > + > + nsRect rect, frameRect; > nsPoint ptOffset; > - firstFrame->GetPointFromOffset(startNodePosition.mOffset, &ptOffset); > + > + // Get the starting frame rect if it's not a block frame. And what if we have block frame. What happens then? I don't understand the setup here. The old code doesn't care about block or not

https://reviewboard.mozilla.org/r/68220/#review66386 > This should have the annotation that it is used only on stack, since having a raw pointer to mFrame isn't exactly safe. mFrame needs also some comment why it is safe. Good point! > I don't understand why result is initiated with aNode and aNodeOffset. Wouldn't nullptr and -1 make more sense? No, it never fails to find a node. I guess that you misunderstand no child case here, but it's a case of text node. I think that I can rewrite the code more explicit. > I know you just change the name of the method here, but why only last child is special cased? > GetNodeHavingFlatText > (or GetNodePositionHavingFlatText) needs some good documentation why if offset == childcount, it does something else than otherwise. This method depends on the behavior of SetRangeFromFlatTextOffset(). For example, if a range is |<p>[<br>]</p>|, then, start point is {<br>, 0}, and the end point is {<p>, 1}. Therefore, index which is same as child count has special meaning. > So if GetNodePositionHavingFlatText could return null node, we need then a null check here. Indeed, but this code will be rewritten. So, if it's necessary in new code, I'll add to it (because I don't want to waste time to merge of removing code).

https://reviewboard.mozilla.org/r/68220/#review66386 > Indeed, but this code will be rewritten. So, if it's necessary in new code, I'll add to it (because I don't want to waste time to merge of removing code). Ah, but renaming the method, anyway, causes merge. Okay, I'll include the check into this patch.

https://reviewboard.mozilla.org/r/68230/#review66402 > It is still unclear what this is needed for. Some random line break before nsIFrame, but why? Could you improve the comment. > // Returns a rect for line breaker before the node of aFrame (If aFrame is > // a <br> frame or a block level frame, it causes a line break at its > // element's open tag, see also ShouldBreakLineBefore()). Note that this > // doesn't check if aFrame should cause line break in non-debug build. > FrameRelativeRect GetLineBreakerRectBefore(nsIFrame* aFrame); > what does "leading" mean here? Hmm, it's font metrics' leading... half leading is appended above and below text for making some space between lines... I'll rewrite this as: > a rect only with font height. > I don't understand this. Why should caret be outside, if we're dealing with textframe? > There is the assertion for ShouldBreakLineBefore, but please add also some comment here that we're dealing with elements here. New comment: > // If aFrame isn't a <br> frame, caret should be at outside of it because > // the line break is before its open tag. For example, case of > // |<div><p>some text</p></div>|, caret is before <p> element and in <div> > // element, the caret should be left of top-left corner of <p> element like: > // > // +-<div>------------------- <div>'s border box > // | I +-<p>----------------- <p>'s border box > // | I | > // | I | > // | | > // ^- caret > // > // However, this is a hack for unusual scenario. This hack shouldn't be > // used as far as possible. As I wrote in the comment, users shouldn't see this result. This is last resort for some cases which we have not found yet. > I don't understand this at all. We have already appended rect to the array. > > But ok, there is that todo. I assume some other patch will explain this. For example, if queried range is [2-5] of |abc\ndef|, then, on non-Windows, we need to return rects of 'c', '\n', 'd' and 'e' but on Windows, we need to return rects of 'c', '\r', '\n', 'd'. Without this patch, this method sets 3rd rect to d's, 4th rect to e's rect. This is one of the main causes of this bug.

https://reviewboard.mozilla.org/r/68232/#review66424 > ok, I still don't understand why we add the same rect again. Okay, I rewrote to following comments: > // If the first frame for the previous offset of the query range and > // the first frame for the start of query range are same, that means > // the start offset is between the first line breaker (i.e., the range > // starts between "\r" and "\n"). > rv = SetRangeFromFlatTextOffset(range, aEvent->mInput.mOffset - 1, 1, > lineBreakType, true, nullptr); and > // If the query range starts from between a line breaker, i.e., it starts > // between "\r" and "\n", the appended rect was for the "\n". Therefore, > // we don't need to append same rect anymore for current "\r\n". > if (startsBetweenLineBreaker) { > continue; > } > > // The appended rect was for "\r" of "\r\n". Therefore, we need to > // append same rect for "\n" too because querying rect of "\r" and "\n" > // should return same rect. E.g., IME may query previous character's > // rect of first character of a line. > aEvent->mReply.mRectArray.AppendElement(rect);

https://reviewboard.mozilla.org/r/68234/#review66426 > This needs some comment. what are we appending here? > // Assign the characters whose rects are computed by the call of > // nsTextFrame::GetCharacterRectsInRange(). > AppendSubString(chars, firstContent, firstFrame.mStartOffsetInNode, > charRects.Length());

https://reviewboard.mozilla.org/r/68234/#review66426 I guess that after you understand the previous patches, you will understand what this patch does.

https://reviewboard.mozilla.org/r/68240/#review66432 Not only for this, though, I wonder, my commit message shown below the commit summary isn't enough? > why +1? Because the rect should be under the last character's rect. Therefore, +1 is necessary for move the rect to outside of the previous character's rect. > ditto Similar to above, the rect should be at right of the last character's rect (note that currently, this isn't bidi-aware). > why height or width is 0? Shouldn't they be 1, per that other stuff ensuring that rects aren't empty? Hmm, I was thinking which is better (setting empty rect or non-empty rect). My conclusion was, the |for| loop can set arbitrary width or height if it's empty. But I have no idea to recommend current code. So, I'll modify them.

https://reviewboard.mozilla.org/r/68242/#review66438 > Why we should append caret rect in that case? > Could you improve the comment a bit > // If the query range is longer than actual content length, we should append > // caret rect at the end of contents as the last character rect because > // native IME may want to query character rect at the end of contents for > // deciding the position of a popup window (e.g., suggest window for next > // word). Note that when this method hasn't appended character rects, it > // means that the offset is too large or the query range is collapsed. > if (offset < kEndOffset || aEvent->mReply.mRectArray.IsEmpty()) {

https://reviewboard.mozilla.org/r/68244/#review66492 Sorry for the big patch, I've tried to separate the patches, but smaller patch doesn't make sense for understanding what this patch changes... > "If the end point is start of a text node...the node shouldn't be included into the range " > Why not? because if it includes a text frame's rect whose contents are not in the range actually, the result isn't what the query event dispatcher wants. I rewrote the comment as: > // If the end point is start of a text node or specified by its parent and > // index, the node shouldn't be included into the range. For example, > // with this case, |<p>abc[<br>]def</p>|, the range ends at 3rd children of > // <p> (see the range creation rules, "2.4. Cases: <element/>]"). This causes > // following frames: > // +----+-----+ > // | abc|[<br>| > // +----+-----+ > // +----+ > // |]def| > // +----+ > // So, if the caller includes the 2nd text frame's rect to its result, > // empty range in the 2nd line causes increasing the rect height. > // Therefore, we need to adjust the end node and offset for such cases. > Move nodePosition.mNode = node; to be right after > if (node->IsNodeOfType(nsINode::eTEXT)) { Good catch! > And what if we have block frame. What happens then? > I don't understand the setup here. The old code doesn't care about block or not If it's a block frame, only with this patch and previous patches, we need to fallback to the hack in GetLineBreakerRectBefore() (see part.8). However, I don't want to use it for now because we can compute better rect with the last text node. So, this issue will be fixed by part.17. Please ignore this for now (I should've write this comment as TODO).

https://reviewboard.mozilla.org/r/68248/#review66458 What's unclear to you of the commit message? > this manual addref/release isn't nice, but > given that this method is super complicated, I don't see better way > (except if the method is somehow split to smaller pieces). Yeah, but recreating the method is risky. We shouldn't do that in this bug. > Why? Can we not have <span>some text</span> here, where last textContent points to that > 'some text'. Please explain I don't understand the example, |<span>some text</span>|... For example, if there is, |<p>abc</p><p>def</p>|, the second <p>'s line break rect should be at right of "c" in the 1st <p> because when caret is at the end of 1st <p>, IME may try to query a rect for next character of 'c'. Then, we should return a rect right of 'c' because user wants windows related to caret positioned around caret, not around the start of next line.

https://reviewboard.mozilla.org/r/68250/#review66468 > I don't understand why this returns lastTextFrame. > Or is lastTextFrame just wrong name? I assumed that text node's primary frame is always nsTextFrame, no?? Additonally, nsTextFrame::GetChildFrameContainingOffset() returns always nsTextFrame. https://dxr.mozilla.org/mozilla-central/rev/0ba72e8027cfcbcbf3426770ac264a7ade2af090/layout/generic/nsTextFrame.cpp#7511,7547,7563

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #97) > I don't understand the example, |<span>some text</span>|... I was thinking non-block level element, when the code seems to deal with block level only

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #98) > https://reviewboard.mozilla.org/r/68250/#review66468 > > > I don't understand why this returns lastTextFrame. > > Or is lastTextFrame just wrong name? > > I assumed that text node's primary frame is always nsTextFrame, no?? 'last' was the question. Why it is 'last'?

(In reply to Olli Pettay [:smaug] from comment #99) > (In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #97) > > > I don't understand the example, |<span>some text</span>|... > I was thinking non-block level element, when the code seems to deal with > block level only <span> doesn't cause line breaks. Basically, <br> and block level element should cause line breaks (but CSS may break this, though). https://dxr.mozilla.org/mozilla-central/source/dom/events/ContentEventHandler.cpp#592,611,619-648 And yes, if after |<span>some text</span>|, there is a block level element's open tag, the line breaker rect should be computed with the last "t"'s rect.

(In reply to Olli Pettay [:smaug] from comment #100) > (In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #98) > > https://reviewboard.mozilla.org/r/68250/#review66468 > > > > > I don't understand why this returns lastTextFrame. > > > Or is lastTextFrame just wrong name? > > > > I assumed that text node's primary frame is always nsTextFrame, no?? > > > 'last' was the question. Why it is 'last'? A text node may cause multiple nsTextFrame instances. For example, the text is too long and wrapped by parent block or have some line breakers in it and the white-space property is specified as they are treated as line breakers (like "pre"). This method returns the last frame which is generated for the given text node.

https://reviewboard.mozilla.org/r/68250/#review66468 > I assumed that text node's primary frame is always nsTextFrame, no?? > > Additonally, nsTextFrame::GetChildFrameContainingOffset() returns always nsTextFrame. > https://dxr.mozilla.org/mozilla-central/rev/0ba72e8027cfcbcbf3426770ac264a7ade2af090/layout/generic/nsTextFrame.cpp#7511,7547,7563 Okay, I added some comments: > // Returns a line breaker rect after aTextContent. This is useful when > // following block element causes a line break before it and it needs to > // compute the line breaker's rect. For example, if there is > // |<p>abc</p><p>def</p>|, the rect of 2nd <p>'s line breaker should be > // at right of "c" in the first <p>, not the start of 2nd <p>. > // The result is relative to the last text frame which represents the > // last character of aTextContent. > FrameRelativeRect GetLineBreakerRectAfter(nsIContent* aTextContent); > ContentEventHandler::GetLineBreakerRectAfter(nsIContent* aTextContent) > { > // aTextContent should be a text node. > MOZ_ASSERT(aTextContent->IsNodeOfType(nsINode::eTEXT)); > > FrameRelativeRect result; > int32_t length = static_cast<int32_t>(aTextContent->Length()); > if (NS_WARN_IF(length < 0)) { > return result; > } > // Get the last nsTextFrame which is caused by aTextContent. Note that > // a text node can cause multiple text frames, e.g., the text is too long > // and wrapped by its parent block or the text has line breakers and its > // white-space property respects the line breakers (e.g., |pre|). > nsIFrame* lastTextFrame = nullptr; > nsresult rv = GetFrameForTextRect(aTextContent, length, true, &lastTextFrame);

https://reviewboard.mozilla.org/r/68252/#review66470 > Please explain how this is different to .textContent Improved the comment: > // Get the contents in aContent (meaning all children of aContent) as plain > // text. E.g., specifying mRootContent gets whole text in it. > // Note that the result is not same as .textContent. The result is > // optimized for native IMEs. For example, <br> element and some block > // elements causes "\n" (or "\r\n"), see also ShouldBreakLineBefore(). > nsresult GenerateFlatTextContent(nsIContent* aContent, > nsAFlatString& aString, > LineBreakType aLineBreakType); > Why is this nsAFlatString&? Why not just normal nsAString&? > I guess because the existing GenerateFlatTextContent uses that too. But why does it do that? No reason, I just used same as the other "GenerateFlatTextContent()". I don't remember why I did it. Okay, I replace them with nsAString. > This magical 16 needs some explanation Added comment: > // Computes the font height, but if it's not available, we should use > // default font size of Firefox. The default font size in default settings > // is 16px. > RefPtr<nsFontMetrics> fontMetrics = > nsLayoutUtils::GetInflatedFontMetricsForFrame(aFrame); > const nscoord kMaxHeight = > fontMetrics ? fontMetrics->MaxHeight() : > 16 * mPresContext->AppUnitsPerDevPixel(); > I don't understand > - mPresContext->AppUnitsPerDevPixel() Okay, I use const for clearing the its meaning and add some comments: > if (!kWritingMode.IsVertical()) { > if (kWritingMode.IsBidiLTR()) { > caretRect.x = kContentRect.x; > } else { > // Move 1px left for the space of caret itself. > const nscoord kOnePixel = mPresContext->AppUnitsPerDevPixel() > caretRect.x = kContentRect.XMost() - kOnePixel; > } > caretRect.height = kMaxHeight; > // However, don't add kOnePixel here because it may cause 2px width at > // aligning the edge to device pixels. > caretRect.width = 1; > } else { > if (kWritingMode.IsVerticalLR()) { > caretRect.x = kContentRect.x; > } else { > caretRect.x = kContentRect.XMost() - kMaxHeight; > } > caretRect.width = kMaxHeight; > // Don't add app units for a device pixel because it may cause 2px height > // at aligning the edge to device pixels. > caretRect.height = 1; > } > Why offset < allText.Length() is error? Ideally, we should handle invisible nodes but currently, we need a lot of work for supporting invisible nodes. Therefore, I don't touch it now. Currently, ContentEventHandler returns error at a lot of points. Therefore, here returns error for consistency behavior. Note that if this just breaks the loop, it may add odd caret rect and returns NS_OK as it misunderstands as reaching the end of contents. > Also here. I don't understand the <. > I would have expected >= > I'm clearly misunderstanding something here, which hints that the code needs more comments. Same, the range doesn't have first frame means that the range is invisible. So, we should return error for now. Note that if the offset reaches the end of contents, we can append a caret rect at the end of contents. Therefore, it uses |<|. # I'll file follow up bug for invisible node handling, but it doesn't important because I don't know the actual websites which specify complicated style in HTML editors.

https://reviewboard.mozilla.org/r/68252/#review66470 > No reason, I just used same as the other "GenerateFlatTextContent()". I don't remember why I did it. > > Okay, I replace them with nsAString. Hmm, no, they need to be nsAFlatString because the method uses ConvertToNativeNewlines() which uses nsAFlatString::ReplaceSubstring(). The method isn't defined in nsAString. # Although, I'm not sure why nsAString is so poor since all string classes are assumed they have characters as continuous array.

Comment on attachment 8770771 [details] Bug 1286464 part.0 Add eQueryTextRect tests for line breakers Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64124/diff/3-4/

Comment on attachment 8776466 [details] Bug 1286464 part.1 Cleaning up ContentEventHandler::OnQueryTextRectArray() Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68216/diff/2-3/

Comment on attachment 8776467 [details] Bug 1286464 part.2 GetFirstFrameInRange() should return node offset since it may return different node's frame Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68218/diff/2-3/

Comment on attachment 8776468 [details] Bug 1286464 part.3 Make static methods, AdjustTextRectNode() and GetFirstFrameInRange(), members of ContentEventHandler Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68220/diff/2-3/

Comment on attachment 8776469 [details] Bug 1286464 part.4 ContentEventHandler::SetRangeFromFlatTextOffset() should set end of the range to after a line break when the range is end between a set of native line breakers Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68222/diff/2-3/

Comment on attachment 8776470 [details] Bug 1286464 part.5 Create ContentEventHandler::EnsureNonEmptyRect() for query various rect event handlers Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68224/diff/2-3/

Comment on attachment 8776471 [details] Bug 1286464 part.6 ContentEventHandler::OnQueryCaretRect() should use eQueryTextRect event when it needs to guess the caret rect Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68226/diff/2-3/

Comment on attachment 8776472 [details] Bug 1286464 part.7 ContentEventHandler::OnQueryTextRect() should redirect the query event to OnQueryCaretRect() if its query range is empty Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68228/diff/2-3/

Comment on attachment 8776473 [details] Bug 1286464 part.8 ContentEventHandler::OnQueryTextRectArray() should handle line break before a node Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68230/diff/2-3/

Comment on attachment 8776474 [details] Bug 1286464 part.9 ContentEventHandler::OnQueryTextRectArray() shouldn't append same rect for following character of a lien breaker when the query range starts from middle of the line breaker Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68232/diff/2-3/

Comment on attachment 8776475 [details] Bug 1286464 part.10 ContentEventHandler::OnQueryTextRectArray() should append same rects for following characters of a line breaker in nsTextFrame Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68234/diff/2-3/

Comment on attachment 8776476 [details] Bug 1286464 part.11 nsTextFrame::GetCharacterRectsInRange() shouldn't compute character rect at the first character in next nsTextFrame Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68236/diff/2-3/

Comment on attachment 8776477 [details] Bug 1286464 part.12 ContentEventHandler::GetFirstFrameHavingFlatTextInRange() should return only frames whose content causes text Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68238/diff/2-3/

Comment on attachment 8776478 [details] Bug 1286464 part.13 ContentEventHandler::OnQueryTextRectArray() should guess a line breaker's rect with previous character's rect if it's available Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68240/diff/2-3/

Comment on attachment 8776479 [details] Bug 1286464 part.14 When ContentEventHandler::OnQueryTextRectArray() reaches the end of query range, it should append caret rect at the end of the content Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68242/diff/2-3/

Comment on attachment 8776480 [details] Bug 1286464 part.15 Rewrite ContentEventHandler::OnQueryTextRect() with new utilities like ContentEventHandler::OnQueryTextRectArray() Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68244/diff/2-3/

Comment on attachment 8776481 [details] Bug 1286464 part.16 Rename ContentEventHandler::Get*FrameHavingFlatTextInRange() to ContentEventHandler::Get*FrameInRangeForTextRect() and make them treat a moz-<br> element as a normal <br> element because it doesn't cause text but needs to compute text Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68246/diff/2-3/

Comment on attachment 8776482 [details] 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 Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68248/diff/2-3/

Comment on attachment 8776483 [details] Bug 1286464 part.18 ContentEventHandler::OnQueryTextRectArray() should compute line breaker's rect with the last character's node when the start of queried range is a line breaker and it's caused by a block frame Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68250/diff/2-3/

Comment on attachment 8776484 [details] Bug 1286464 part.19 ContentEventHandler::OnQueryTextRect() should handle the case when queried range starts from the end of mRootContent Review request updated; see interdiff: https://reviewboard.mozilla.org/r/68252/diff/2-3/

FYI, I'll try to review these tomorrow.

(In reply to Olli Pettay [:smaug] from comment #82) > I'd like to see also a patch containing all the changes, as an attachment > here in bugzilla. It is hard to see the big picture from smaller patches > (even though they ease reviewing) This would be nice. Or does mozreview somehow provide this?

Comment on attachment 8776468 [details] Bug 1286464 part.3 Make static methods, AdjustTextRectNode() and GetFirstFrameInRange(), members of ContentEventHandler https://reviewboard.mozilla.org/r/68220/#review66942

Comment on attachment 8776474 [details] Bug 1286464 part.9 ContentEventHandler::OnQueryTextRectArray() shouldn't append same rect for following character of a lien breaker when the query range starts from middle of the line breaker https://reviewboard.mozilla.org/r/68232/#review66944 ok, thanks for the comments.

Comment on attachment 8776475 [details] Bug 1286464 part.10 ContentEventHandler::OnQueryTextRectArray() should append same rects for following characters of a line breaker in nsTextFrame https://reviewboard.mozilla.org/r/68234/#review66946 ::: dom/events/ContentEventHandler.cpp:1648 (Diff revision 3) > + aEvent->mInput.mOffset - 1, 1, > + lineBreakType, true, nullptr); > + if (NS_WARN_IF(NS_FAILED(rv))) { > + return rv; > + } > + startsBetweenLineBreaker = Took a bit time to understand... ok.

Curious, do you have some good manual tests for this, like how to test gmail or such. Could we get devs using IME regularly to test this stuff one this lands?

Comment on attachment 8776478 [details] Bug 1286464 part.13 ContentEventHandler::OnQueryTextRectArray() should guess a line breaker's rect with previous character's rect if it's available https://reviewboard.mozilla.org/r/68240/#review66948 ::: dom/events/ContentEventHandler.cpp:1644 (Diff revision 3) > + } else { > + // The frame position in the root widget will be added in the > + // following for loop but we need the rect in the previous frame. > + // So, we need to avoid using current frame position. > + brRect = lastCharRect - frameRect.TopLeft(); > + if (!wasLineBreaker) { So wasLineBreaker is always false here. Why we need wasLineBreaker? I assume it is used in some other patch, but don't know yet. If it is not used, please remove it.

Have you tested this all both (1) with long text which wraps (implicit line breaks) and (2) with explicit <br> (explicit line breaks). And also with content editable containing multiple text nodes in an element. Something like data:text/html,<script>onload = function() { var d = document.createElement("div"); d.contentEditable = true; for (var i = 0; i < 100; ++i) { d.appendChild(document.createTextNode(i + " ")); if (i % 10 == 0) { d.appendChild(document.createElement("br"));d.appendChild(document.createElement("br"));} } document.body.appendChild(d); }</script>

Comment on attachment 8776482 [details] 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 https://reviewboard.mozilla.org/r/68248/#review66950 So I don't still understand what case this patch is trying to capture. ::: dom/events/ContentEventHandler.cpp:1982 (Diff revision 3) > rect.height -= ptOffset.y; > } else { > rect.x += ptOffset.x; > rect.width -= ptOffset.x; > } > - } else if (firstFrame->GetType() == nsGkAtoms::brFrame) { > + } else if (firstFrame->GetType() != nsGkAtoms::brFrame && lastTextContent) { I don't understand this. br is about the first frame but lastTextContent can be about something in tree way later, no? (This is hard to review since GetFirstFrameInRangeForTextRect isn't in mozilla-central and there isn't a patch containing all the changes. So I know GetFirstFrameInRangeForTextRect is defined in some patch, but which...)

Comment on attachment 8776483 [details] Bug 1286464 part.18 ContentEventHandler::OnQueryTextRectArray() should compute line breaker's rect with the last character's node when the start of queried range is a line breaker and it's caused by a block frame https://reviewboard.mozilla.org/r/68250/#review66952 (please explain my review questions) ::: dom/events/ContentEventHandler.cpp:1690 (Diff revision 3) > } > return result; > } > > +ContentEventHandler::FrameRelativeRect > +ContentEventHandler::GetLineBreakerRectAfter(nsIContent* aTextContent) So how do we know we have line break after the textnode? I mean, what guarantees we aren't dealing with text node which is inside a <span> and the next element is also <span> so there isn't a line break- ::: dom/events/ContentEventHandler.cpp:1794 (Diff revision 3) > - // causes a line break at first time of this loop. > - if (firstFrame->GetType() == nsGkAtoms::brFrame || > - aEvent->mInput.mOffset == offset) { > FrameRelativeRect relativeBRRect = GetLineBreakerRectBefore(firstFrame); > brRect = relativeBRRect.RectRelativeTo(firstFrame); > + } else if (aEvent->mInput.mOffset == offset) { Why does this mean we want to add brRect?

Comment on attachment 8776484 [details] Bug 1286464 part.19 ContentEventHandler::OnQueryTextRect() should handle the case when queried range starts from the end of mRootContent https://reviewboard.mozilla.org/r/68252/#review66954 ::: dom/events/ContentEventHandler.h:247 (Diff revision 3) > LineBreakType aLineBreakType); > + // Get the contents in aContent (meaning all children of aContent) as plain > + // text. E.g., specifying mRootContent gets whole text in it. > + // Note that the result is not same as .textContent. The result is > + // optimized for native IMEs. For example, <br> element and some block > + // elements causes "\n" (or "\r\n"), see also ShouldBreakLineBefore(). But implicit line breaking doesn't cause \n, right? I mean long text which layout then wraps to multiple lines. ::: dom/events/ContentEventHandler.cpp:1825 (Diff revision 3) > - // So, in such case, we must have reached the end of the contents. > + // the end of contents. > if (!firstFrame.IsValid()) { > + nsAutoString allText; > + rv = GenerateFlatTextContent(mRootContent, allText, lineBreakType); > + // If we meet invisible node, make this query fail for now. > + if (NS_WARN_IF(NS_FAILED(rv)) || offset < allText.Length()) { Why does offset < allText.Length() mean that there are invisible nodes? What guarantees offset is normally at least allText.Length() ? Please explain. ::: dom/events/ContentEventHandler.cpp:2066 (Diff revision 3) > - // that means that there is no remaining content which causes text. > - // So, in such case, we must have reached the end of the contents. > + // means that there are no visible frames having text or the offset reached > + // the end of contents. > if (!firstFrame.IsValid()) { > - // TODO: Handle this case later. > + nsAutoString allText; > + rv = GenerateFlatTextContent(mRootContent, allText, lineBreakType); > + // If there are no visible nodes in the range, make this query fail for now. Same here. ::: dom/events/ContentEventHandler.cpp:2080 (Diff revision 3) > + if (NS_WARN_IF(erv.Failed())) { > + return NS_ERROR_UNEXPECTED; > + } > + nsRect rect; > + FrameAndNodeOffset lastFrame = GetLastFrameInRangeForTextRect(range); > + if (lastFrame) { Ok, here we do something with lastFrame, but it isn't clear what and why. This needs a comment.

Comment on attachment 8776480 [details] Bug 1286464 part.15 Rewrite ContentEventHandler::OnQueryTextRect() with new utilities like ContentEventHandler::OnQueryTextRectArray() https://reviewboard.mozilla.org/r/68244/#review66956 (I will need to re-read this still couple of times, but please address the review comments) ::: dom/events/ContentEventHandler.h:349 (Diff revision 3) > - // causes text after the start of the range. > + // causes text in the range. mOffsetInNode is start offset in the frame. > FrameAndNodeOffset GetFirstFrameHavingFlatTextInRange(nsRange* aRange); > > + // Get last frame before the end of the given range for computing text rect. > + // This returns invalid FrameAndNodeOffset if there is no content which > + // causes text in the range. mOffsetInOde is end offset in the frame. mOffsetInNode ::: dom/events/ContentEventHandler.cpp:1519 (Diff revision 3) > + // +----+-----+ > + // +----+ > + // |]def| > + // +----+ > + // So, if the caller includes the 2nd text frame's rect to its result, > + // empty range in the 2nd line causes increasing the rect height. what empty range in the 2nd line? I'm not sure what we're trying to adjust here. Is ] the "empty range"?) ::: dom/events/ContentEventHandler.cpp:1571 (Diff revision 3) > + } > + > + // If the last frame is a text frame, we need to check if the range actually > + // includes at least one character in the range. Therefore, if it's not a > + // text frame, we do nothing anymore. > + if (lastFrame->GetType() != nsGkAtoms::textFrame) { How can lastFrame not be text frame here?

(In reply to Olli Pettay [:smaug] from comment #128) > (In reply to Olli Pettay [:smaug] from comment #82) > > I'd like to see also a patch containing all the changes, as an attachment > > here in bugzilla. It is hard to see the big picture from smaller patches > > (even though they ease reviewing) > > This would be nice. Or does mozreview somehow provide this? Oh, sorry, I forgot to do that. And I checked the MozReview, then, I found "Squashed Diff" below the Summary. (In reply to Olli Pettay [:smaug] from comment #132) > Curious, do you have some good manual tests for this, like how to test gmail > or such. > Could we get devs using IME regularly to test this stuff one this lands? Yes. As far as possible, I wrote the tests of simple API behavior. However, we still don't have emulation API to test native IME behavior. Therefore, when I write big changes, I usually use tryserver build for a couple of days. However, even with such manual test, I get some regressions with untested IME :-( (In reply to Olli Pettay [:smaug] from comment #134) > Have you tested this all both (1) with long text which wraps (implicit line > breaks) and (2) with explicit <br> (explicit line breaks). The latter is tested by both manually and automated tests of part.0. However, the former is good point, I'll check it and if there is a bug, I'll file a new bug for that (these bugs are too big, though). > And also with content editable containing multiple text nodes in an element. > Something like > data:text/html,<script>onload = function() { var d = > document.createElement("div"); d.contentEditable = true; for (var i = 0; i < > 100; ++i) { d.appendChild(document.createTextNode(i + " ")); if (i % 10 == > 0) { > d.appendChild(document.createElement("br"));d.appendChild(document. > createElement("br"));} } document.body.appendChild(d); }</script> Good point too. I have not tested this yet due to unusual case. But we should include this into the tests. I'll file a followup bug for this case. # I'll check the review results latter, thank you for your review (especially on a holiday)!

Comment on attachment 8776478 [details] Bug 1286464 part.13 ContentEventHandler::OnQueryTextRectArray() should guess a line breaker's rect with previous character's rect if it's available https://reviewboard.mozilla.org/r/68240/#review66948 > So wasLineBreaker is always false here. Why we need wasLineBreaker? I assume it is used in some other patch, but don't know yet. > If it is not used, please remove it. No, it can be true if previous |while| loop handles \n. > // If it's not a line breaker or the line breaker length is same as > // XP line breaker's, we need to do nothing for current character. > wasLineBreaker = chars[i] == '\n'; > if (!wasLineBreaker || kBRLength == 1) { > continue; > }

Comment on attachment 8776480 [details] Bug 1286464 part.15 Rewrite ContentEventHandler::OnQueryTextRect() with new utilities like ContentEventHandler::OnQueryTextRectArray() https://reviewboard.mozilla.org/r/68244/#review66956 > what empty range in the 2nd line? > I'm not sure what we're trying to adjust here. > Is ] the "empty range"?) Yes, it is. The end point is decided to set to it for including whole of its previous frame. Therefore, the range isn't expected as including a text frame's contents if its start is the end. Is this comment is clearer? > // If the end point is start of a text node or specified by its parent and > // index, the node shouldn't be included into the range. For example, > // with this case, |<p>abc[<br>]def</p>|, the range ends at 3rd children of > // <p> (see the range creation rules, "2.4. Cases: <element/>]"). This causes > // following frames: > // +----+-----+ > // | abc|[<br>| > // +----+-----+ > // +----+ > // |]def| > // +----+ > // So, if this method includes the 2nd text frame's rect to its result, the > // caller will return too tall rect which includes 2 lines in this case isn't > // expected by native IME (e.g., popup of IME will be positioned at bottom > // of "d" instead of right-bottom of "c"). Therefore, this method shouldn't > // include the last frame when its content isn't in aRange actually. > How can lastFrame not be text frame here? The frame my be a <br> frame or a block frame, in usual cases. I.e., when the |for| loops ended here: > if (ShouldBreakLineBefore(node->AsContent(), mRootContent)) { > nodePosition.mNode = node; > nodePosition.mOffset = 0; > break; > }

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #139) > Oh, sorry, I forgot to do that. And I checked the MozReview, then, I found > "Squashed Diff" below the Summary. ah, thanks. > > (In reply to Olli Pettay [:smaug] from comment #132) > > Curious, do you have some good manual tests for this, like how to test gmail > > or such. > > Could we get devs using IME regularly to test this stuff one this lands? > > Yes. As far as possible, I wrote the tests of simple API behavior. However, > we still don't have emulation API to test native IME behavior. Therefore, > when I write big changes, I usually use tryserver build for a couple of > days. However, even with such manual test, I get some regressions with > untested IME :-( Hsin-Yi, do you and others in TP use IMEs? Would anyone be willing to use some tryserver build for couple of days? Assuming masayuki thinks that would be useful. > However, the former is good point, I'll check it and if there is a bug, I'll > file a new bug for that (these bugs are too big, though). Just need to make sure that new bug and this bug lands to the same release. (Is this all something we need to get to Aurora? )

Comment on attachment 8776482 [details] 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 https://reviewboard.mozilla.org/r/68248/#review66950 E.g., <p>abc[</p><p>]def</p> In this case, a rect for the line breaker caused by 2nd <p> should be next to "c" rather than "before" of <p>. This example should be rendered as: +-<p>--------------------------------+ |abc | +------------------------------------+ +-<p>--------------------------------+ |def | +------------------------------------+ Then, when caret is at the end of the first <p> (i.e., after "c"), IME tries to query a rect for next of "c" (i.e., line breaker's rect for 2nd <p>). Without this patch, its computed as: +-<p>--------------------------------+ |abc | +------------------------------------+ |+-<p>--------------------------------+ ||def | +------------------------------------+ ^- computed caret rect for 2nd <p> by GetLineBreakerRectBefore(). However, this isn't expected, it should be: +-<p>--------------------------------+ |abc[] | +------------------------------------+ +-<p>--------------------------------+ |def | +------------------------------------+ So, the rect can be computed with the previous character's rect of the range start. > I don't understand this. br is about the first frame but lastTextContent can be about something in tree way later, no? > > (This is hard to review since GetFirstFrameInRangeForTextRect isn't in mozilla-central and there isn't a patch containing all the changes. So I know GetFirstFrameInRangeForTextRect is defined in some patch, but which...) GetFirstFrameInRangeForTextRect() at this patch is here: > ContentEventHandler::FrameAndNodeOffset > ContentEventHandler::GetFirstFrameInRangeForTextRect(nsRange* aRange) > { > NodePosition nodePosition; > nsCOMPtr<nsIContentIterator> iter = NS_NewPreContentIterator(); > for (iter->Init(aRange); !iter->IsDone(); iter->Next()) { > nsINode* node = iter->GetCurrentNode(); > if (NS_WARN_IF(!node)) { > break; > } > > if (!node->IsContent()) { > continue; > } > > if (node->IsNodeOfType(nsINode::eTEXT)) { > // If the range starts at the end of a text node, we need to find > // next node which causes text. > int32_t offsetInNode = > node == aRange->GetStartParent() ? aRange->StartOffset() : 0; > if (static_cast<uint32_t>(offsetInNode) < node->Length()) { > nodePosition.mNode = node; > nodePosition.mOffset = offsetInNode; > break; > } > continue; > } > > // If the element node causes a line break before it, it's the first > // node causing text. > if (ShouldBreakLineBefore(node->AsContent(), mRootContent) || > IsMozBR(node->AsContent())) { > nodePosition.mNode = node; > nodePosition.mOffset = 0; > } > } > > if (!nodePosition.IsValid()) { > return FrameAndNodeOffset(); > } > > nsIFrame* firstFrame = nullptr; > GetFrameForTextRect(nodePosition.mNode, nodePosition.mOffset, > true, &firstFrame); > return FrameAndNodeOffset(firstFrame, nodePosition.mOffset); > } And for making clearer what's handled by each condition in OnQueryTextRect(), I improve the comments in it: > // 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) { <snip> > } > // 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) { <snip> > } > // 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 { I hope these comments help you and other developers!

(In reply to Olli Pettay [:smaug] from comment #142) > > However, the former is good point, I'll check it and if there is a bug, I'll > > file a new bug for that (these bugs are too big, though). > Just need to make sure that new bug and this bug lands to the same release. > (Is this all something we need to get to Aurora? ) No, this should be landed only Nightly due to too risky and current release build already have this bug (but the pending patches of bug 1257446 and bug 1286157 make the symptom worse). I'd like to land the patches for bug 1257446, bug 1286157 and this bug as soon as possible because the patches really improve the UX enough in daily use and the patches are too many for managing only with my local environment. And of course, yes, I should fix regressions in 51.

Comment on attachment 8776483 [details] Bug 1286464 part.18 ContentEventHandler::OnQueryTextRectArray() should compute line breaker's rect with the last character's node when the start of queried range is a line breaker and it's caused by a block frame https://reviewboard.mozilla.org/r/68250/#review66952 > So how do we know we have line break after the textnode? I mean, what guarantees we aren't dealing with text node which is inside a <span> and the next element is also <span> so there isn't a line break- Hmm, this is just a helper method for preventing to duplicate the same code. GuessLineBreakerRectAfter() might be better, but I'm not sure... The important thing is, this is just computing the line breakers rect *as it exists after the text node*. But including such words into the method name causes too long name... Any ideas? > Why does this mean we want to add brRect? Because there is no cached character's rect due to the first time of the loop. Okay, for consistency with OnQueryTextRect(), I'll reorder each if/else if/else blocks similar to OnQueryTextRect()'s. And improve the comments, please check the coming patch.

Comment on attachment 8776484 [details] Bug 1286464 part.19 ContentEventHandler::OnQueryTextRect() should handle the case when queried range starts from the end of mRootContent https://reviewboard.mozilla.org/r/68252/#review66954 > But implicit line breaking doesn't cause \n, right? > I mean long text which layout then wraps to multiple lines. Yes. In such case, we don't need to include line breaker into queried character rects nor text contents. > Why does offset < allText.Length() mean that there are invisible nodes? > What guarantees offset is normally at least allText.Length() ? > Please explain. Okay, I improve the comment: > rv = GenerateFlatTextContent(mRootContent, allText, lineBreakType); > // If the offset doesn't reach the end of contents yet but there is no > // frames for the node, that means that current offset's node is hidden > // by CSS or something. Ideally, we should handle it with the last > // visible text node's last character's rect, but it's not usual cases > // in actual web services. Therefore, currently, we should make this > // case fail. > if (NS_WARN_IF(NS_FAILED(rv)) || offset < allText.Length()) { > return NS_ERROR_FAILURE; > } > Same here. Using same comment here. > Ok, here we do something with lastFrame, but it isn't clear what and why. > This needs a comment. Okay, I add comments: > // If there is at least one frame which can be used for computing a rect > // for a character or a line breaker, we should use it for guessing the > // caret rect at the end of the contents. > if (lastFrame) { <snip> > } > // Otherwise, if there are no contents in mRootContent, guess caret rect in > // its frame (with its font height and content box). > else {

Some of this comment is off-topic, though: I tested with wrapped text in <textarea>, then, at least, ContentEventHandler works as expected but perhaps not so for some IMEs. Gecko can put caret both end of wrapped line and start of next line. Therefore, if IME uses a character rect at same offset of caret, Gecko returns first character of new line. Therefore, e.g., Google Japanese Input shows input mode popup at start of new line even when caret is at the end of the wrapped line. We might be able to say, this is a bug of IME, but interestingly, Microsoft products doesn't put caret both the wrapped points. As far as I've tested, Edge can puts caret only at the end of wrapped line. IE 11, Word, Wordpad can put caret only at the start of new line. The latter case matches caret position and IME popup window's position. However, IE has same problem as Gecko. So, I don't think that we don't need to touch this issue at least for short period since most IME popups its window after putting first character of composition string and may be move it to the first character. Therefore, the position is always expected. This issue is a problem only when IME shows popup without composition string and caret is at the end of wrapped line.

FYI: Google Chrome can put caret both end of wrapped line and start of new line like Gecko. However, they return caret rect at this case. I guess that we can do same hack in IMEContentObserver and ContentCacheInParent when a character rect is queried and it matches selection offset. However, I'm afraid that it may cause odd behavior with some IMEs which want actual character's rect for other purpose. So, anyway, we should not hack this issue without whitelist of IMEs. (I think that native IME handler should request the hack if active IME is Google Japanese Input or something.)

Comment on attachment 8776483 [details] Bug 1286464 part.18 ContentEventHandler::OnQueryTextRectArray() should compute line breaker's rect with the last character's node when the start of queried range is a line breaker and it's caused by a block frame https://reviewboard.mozilla.org/r/68250/#review66952 > Hmm, this is just a helper method for preventing to duplicate the same code. > > GuessLineBreakerRectAfter() might be better, but I'm not sure... The important thing is, this is just computing the line breakers rect *as it exists after the text node*. But including such words into the method name causes too long name... Any ideas? I renamed it as GuessLineBreakerRectAfter(), but I don't like it. If you have better idea, let me know...

smaug: ping...

yeah, sorry, had other stuff to review.

Comment on attachment 8776480 [details] Bug 1286464 part.15 Rewrite ContentEventHandler::OnQueryTextRect() with new utilities like ContentEventHandler::OnQueryTextRectArray() https://reviewboard.mozilla.org/r/68244/#review68454 ::: dom/events/ContentEventHandler.cpp:1522 (Diff revision 4) > + // +----+ > + // So, if this method includes the 2nd text frame's rect to its result, the > + // caller will return too tall rect which includes 2 lines in this case isn't > + // expected by native IME (e.g., popup of IME will be positioned at bottom > + // of "d" instead of right-bottom of "c"). Therefore, this method shouldn't > + // include the last frame when its content isn't in aRange actually. "isn't really in aRange" or something like that. ::: dom/events/ContentEventHandler.cpp:2011 (Diff revision 4) > - frame = node->AsContent()->GetPrimaryFrame(); > + nsIFrame* primaryFrame = node->AsContent()->GetPrimaryFrame(); > + // The node may be hidden by CSS. > + if (!primaryFrame) { > + continue; > + } > + // Ignore block frames for avoiding GetLineBreakerRectBefore() issue. I don't understand this comment. What is being ignored? You check only for text and brFrames but there are tons of other frames too than those and block frames. inline frames, table stuff etc. Should the comment be that deal with br and text frames only because ... (something).

Comment on attachment 8776482 [details] 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 https://reviewboard.mozilla.org/r/68248/#review66950 So what should happen for example in <p>ab</p><span>c[</span<p>]def</p>? or in <p>abc[</p><span>]</span<p>def</p> ?

Comment on attachment 8776482 [details] 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 https://reviewboard.mozilla.org/r/68248/#review68472

Comment on attachment 8776483 [details] Bug 1286464 part.18 ContentEventHandler::OnQueryTextRectArray() should compute line breaker's rect with the last character's node when the start of queried range is a line breaker and it's caused by a block frame https://reviewboard.mozilla.org/r/68250/#review68478

Comment on attachment 8776484 [details] Bug 1286464 part.19 ContentEventHandler::OnQueryTextRect() should handle the case when queried range starts from the end of mRootContent https://reviewboard.mozilla.org/r/68252/#review68486 Explain those ::: dom/events/ContentEventHandler.h:406 (Diff revision 4) > // rect of 2nd <p>'s line breaker should be at right of "c" in the first > // <p>, not the start of 2nd <p>. The result is relative to the last text > // frame which represents the last character of aTextContent. > FrameRelativeRect GuessLineBreakerRectAfter(nsIContent* aTextContent); > > + // Returns a guessed first rect. I.e., it may be different from actual The comment should have some example what the rect is when selection is collapsed. ::: dom/events/ContentEventHandler.cpp:1753 (Diff revision 4) > + // is 16px. > + RefPtr<nsFontMetrics> fontMetrics = > + nsLayoutUtils::GetInflatedFontMetricsForFrame(aFrame); > + const nscoord kMaxHeight = > + fontMetrics ? fontMetrics->MaxHeight() : > + 16 * mPresContext->AppUnitsPerDevPixel(); Curious, is there no easy way to read some pref here and not hard code 16? ::: dom/events/ContentEventHandler.cpp:2161 (Diff revision 4) > + } > + // If there is a text frame at the end, use its information. > + else if (lastFrame->GetType() == nsGkAtoms::textFrame) { > + relativeRect = GuessLineBreakerRectAfter(lastFrame->GetContent()); > + } > + // If there is a empty block frame at the end, guess caret rect in it. How do you know this is block frame? Why can't lastFrame point to inline frame?

Have you tested how this all works with css transformations? How do other browsers work, and how does Gecko behave?

(In reply to Olli Pettay [:smaug] from comment #178) > Have you tested how this all works with css transformations? How do other > browsers work, and how does Gecko behave? Complicated cases like rotate isn't supported because we've not met such website and only TSF on Windows supports such complicated transformations (but I'm not sure if major IMEs support such complicated function actually). trancelate and scale are supported in limited situations such as the queried range isn't across different transformed frames. That was done in bug 1109410.

Ah, but only scale is broken only in e10s mode. I think that it's a bug of eQueryTextRectArray's new bug. I'll file it.

Comment on attachment 8776480 [details] Bug 1286464 part.15 Rewrite ContentEventHandler::OnQueryTextRect() with new utilities like ContentEventHandler::OnQueryTextRectArray() https://reviewboard.mozilla.org/r/68244/#review68454 > "isn't really in aRange" or something like that. Fixed. > I don't understand this comment. What is being ignored? You check only for text and brFrames but there are tons of other frames too than those and block frames. inline frames, table stuff etc. > Should the comment be that deal with br and text frames only because ... (something). I rewrote the comment as: > // We should take only text frame's rect and br frame's rect. We can > // always use frame rect of text frame and GetLineBreakerRectBefore() > // can return exactly correct rect only for <br> frame for now. On the > // other hand, GetLineBreakRectBefore() returns guessed caret rect for > // the other frames. We shouldn't include such odd rect to the result.

(In reply to Olli Pettay [:smaug] from comment #174) > Comment on attachment 8776482 [details] > 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 > > https://reviewboard.mozilla.org/r/68248/#review66950 > > So what should happen for example in <p>ab</p><span>c[</span<p>]def</p>? In this case, range starts from {#text ("c"), 1} - {#text ("def"), 0}. And <span> content is ignored by any loop with ContentIterator. So, ContentEventHandler works as there is no <span> element in this case. So, when the line breaker caused by 2nd <p>'s rect is queried, it's computed with "c"'s rect. > or in <p>abc[</p><span>]</span<p>def</p> ? This is unrealistic case because users cannot set caret in the empty <span>. But when I make such situation forcibly with JS <https://jsfiddle.net/t2x5r6xx/>, the line breaker for 2nd <p> is computed at 0 height font in <span>. That's unexpected behavior, though. We should do something if it's necessary after we get a bug report because I have no idea of this situation (basically, the <span> should be in a <p> in such struct document).

Comment on attachment 8776484 [details] Bug 1286464 part.19 ContentEventHandler::OnQueryTextRect() should handle the case when queried range starts from the end of mRootContent https://reviewboard.mozilla.org/r/68252/#review68486 > The comment should have some example what the rect is when selection is collapsed. New comment: > // Returns a guessed first rect. I.e., it may be different from actual > // caret when selection is collapsed at start of aFrame. For example, this > // guess the caret rect only with the content box of aFrame and its font > // height like: > // +-aFrame----------------- (border box) > // | > // | +--------------------- (content box) > // | | I > // ^ guessed caret rect > // However, actual caret is computed with more information like line-height, > // child frames of aFrame etc. But this does not emulate actual caret > // behavior exactly for simpler and faster code because it's difficult and > // we're not sure it's worthwhile to do it with complicated implementation. > FrameRelativeRect GuessFirstCaretRectIn(nsIFrame* aFrame); > Curious, is there no easy way to read some pref here and not hard code 16? Of course, we can do it, but not easy because our font preference has a lot of font sizes for every locale and every generic font family... (primary font family depending on the locale, e.g., Western is "serif", but Japanese is "san-serif".) So, I don't think we should do that only in such illegal cases. > How do you know this is block frame? > Why can't lastFrame point to inline frame? Oh, right. This could be any frame. I just rewrote the commend as: > // If there is an empty frame which is neither a text frame nor a <br> > // frame at the end, guess caret rect in it.

Smaug: Thank you for such big patches' review as I know you are always busy! I'll land them now because my local queue is too long and my new patches depend on the series of these bugs (this and the bugs blocked by this). If you have some points which I should fix in this cycle, please let me know or file bugs for each issue.