Summary: | [WebCoreBridge firstRectForDOMRange:] works incorrectly for the first character after a line wrap | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alexey Proskuryakov <ap> | ||||||||||
Component: | HTML Editing | Assignee: | Justin Garcia <justin.garcia> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | evan, justin.garcia | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 420+ | ||||||||||||
Hardware: | Mac | ||||||||||||
OS: | OS X 10.4 | ||||||||||||
Attachments: |
|
Description
Alexey Proskuryakov
2005-10-17 09:47:16 PDT
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
r=me
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
|