Bug 5401 – [WebCoreBridge firstRectForDOMRange:] works incorrectly for the first character after a line wrap

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Alexey Proskuryakov]

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).

[Alexey Proskuryakov]

Created attachment 4380 [details]
a screenshot demonstrating the problem

[Alexey Proskuryakov]

Created attachment 4381 [details]
automated test

[Alexey Proskuryakov]

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.

[Maciej Stachowiak]

Comment on attachment 4383 [details]
proposed patch

r=me

[Justin Garcia]

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?

[Alexey Proskuryakov]

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.

[Justin Garcia]

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.

[Alexey Proskuryakov]

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?

[Justin Garcia]

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.

[Alexey Proskuryakov]

Comment on attachment 4383 [details]
proposed patch

Resetting the review flag.

[Maciej Stachowiak]

I don't think input methods ever want the "end of line before a line break" position returned here.

[Dave Hyatt]

Comment on attachment 4383 [details]
proposed patch

r=me

[Dave Hyatt]

Comment on attachment 4383 [details]
proposed patch

Actually sounds like there are some open questions, so I will hold off on+.

[Justin Garcia]

Hyatt, what were those open questions?

[Darin Adler]

Comment on attachment 4383 [details]
proposed patch

Justin, do you think that changing upstream to downstream here is correct? When
could it do harm?

[Justin Garcia]

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".

[Maciej Stachowiak]

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.

[Justin Garcia]

Comment on attachment 4383 [details]
proposed patch

r- until AP can show me that firstRectForDOMRange and firstRectForCharacter
range clients never want the upstream rect.

[Alexey Proskuryakov]

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.

[Justin Garcia]

Comment on attachment 4383 [details]
proposed patch

Ok, r=me