If the firstRectForDOMRange: is called on a range that starts after a line wrap, a single-pixel rectangle at the end of the previous line is returned instead. Steps to reproduce: 1. Type something in Blot, to fill the first line of text 2. Switch to Kotoeri Hiragana 3. Type something more 4. Press the down arrow to make a Kotoeri's popup appear. Results: it appears below the end of the first line. Expected results: it should appear below the beginning of the second line (under the characters being converted). The same issue causes problems with handling mouse clicks in input methods (see a remotely related bug 5230).
Created attachment 4380 [details] a screenshot demonstrating the problem
Created attachment 4381 [details] automated test
Created attachment 4383 [details] proposed patch Hmm, a fix seems to be as easy as setting a different affinity when calculating startCaretRect. No regression tests broken. Please note that my proposed patch to bug 5230 changes the next line, which calculates endCaretRect. These fixes are unrelated, and I have tested them both separately and together.
Comment on attachment 4383 [details] proposed patch r=me
Comment on attachment 4383 [details] proposed patch I'm not sure that this patch is right. Alexey, you write that the rects returned by firstRectForDOMRange for ranges after a line wrap are all positioned at the spot where the line wraps. However, in your test case, the sequence that you use doesn't wrap because it uses a hard hyphen "aaaaaaaaaa–aaaaaaaaaaaaaaaaaaaaaaa". The sequence just spills outside its width:100px containing block. The test case still illustrates a bug, I think, which is that the rects returned from firstRectForDOMRange should reflect text that has overflowed. The rects returned are probably only approrpriate for overflow:hidden/scroll/auto. I think that the bug is in RenderText::caretRect(...): RenderBlock *cb = containingBlock(); int availableWidth = cb->lineWidth(top); if (style()->whiteSpace() == NORMAL) left = kMin(left, absx + box->m_x + availableWidth - 1); Perhaps we should only do the kMin if overflow is not "none". What do you think?
Created attachment 4404 [details] DumpRenderTree pixel output I have slightly modified the test case to produce a pixel output, and here are the results. Running on ToT from about a week ago, but if the current one behaves differently, it's probably a regression :) Besides, the patch fixes the original problem with Blot and Kotoeri.
I talked to Alexey on IRC and it turns out I was using the wrong encoding, so my '-' was not rendering as a '-'. This is why my line wasn't breaking. Still, the problem that I noticed is a bug, it's just not the problem Alexey was encountering. For Alexey's problem, I don't think it's appropriate to always use DOWNSTREAM for startCaretRect in firstRectForDOMRange. We spoke on IRC and he's working on a new fix.
Created bug 5445 for overflowed text problem. Returning to this bug: I said that I understood why it's inappropriate to always use DOWNSTREAM for startCaretRect, but on a second thought, I do not, sorry... For a range starting where the wrap occurs: 1. If the range has non-zero length, then it covers a character at the next line, and returning a single- pixel rectangle from the previous line is inappropriate (and makes input methods unhappy). 2. If the range is zero-length, then it's an insertion point position - and the actual insertion point wraps to the next line, too, so it also should be DOWNSTREAM. What am I missing?
I had no idea that fistRectForCharacterRange was the only client of firstRectForDOMRange. Now some of the things that firstRectForDOMRange make more sense, its assumption that the start/ end containers have renderers for example. Now that I know that, I agree with (1). However, regarding (2): might some clients depend on getting the upstream rect when passing firstRectForCharacterRange a zero lengthed range at an offset where the line wraps? Review? this again and let someone else weigh in here.
Comment on attachment 4383 [details] proposed patch Resetting the review flag.
I don't think input methods ever want the "end of line before a line break" position returned here.
Comment on attachment 4383 [details] proposed patch Actually sounds like there are some open questions, so I will hold off on+.
Hyatt, what were those open questions?
Comment on attachment 4383 [details] proposed patch Justin, do you think that changing upstream to downstream here is correct? When could it do harm?
I'm thinking of the case where someone calls firstRectForCharacter range, passing a zero-lengthed range, at an offet where a line wraps. Do they want the zero-width UPSTREAM rect (it is arguably 'first'), or the zero-width DOWNSTREAM rect? Since this change (to always return the DOWNSTREAM rect) is inside firstRectForDOMRange, a similar question should be asked of its callers. AP and Maciej say that callers would never want the UPSTREAM rect. However hyatt said that there were still some "open questions".
I think hyatt just meant that given all the comments, he wasn't comfortable signing off on this patch himself. I think the questions you asked are the key ones.
Comment on attachment 4383 [details] proposed patch r- until AP can show me that firstRectForDOMRange and firstRectForCharacter range clients never want the upstream rect.
Comment on attachment 4383 [details] proposed patch firstRectForDOMRange is only used twice, for -[NSTextInput firstRectForCharacterRange] and for -WebHTMLView characterIndexForPoint]. For -[NSTextInput firstRectForCharacterRange], the documentation says "If the length of theRange is 0 (as it would be if there is nothing selected at the insertion point), the rectangle will coincide with the insertion point, and its width will be 0." When the insertion point is positioned at a line wrap, it is normally diplayed at the beginning of the next line. Yes, it is possible to position it at the end of the wrapped line with a mouse click, but it is less common IMO. For -[WebHTMLView characterIndexForPoint], downstream is certainly preferable: it asks for single-character ranges, and expects to get non-empty rects. Currently, characterIndexForPoint doesn't work for the first character after a line wrap.
Comment on attachment 4383 [details] proposed patch Ok, r=me