| Summary: | RenderText::addLineBoxRects erroneously includes last char for boundingBox | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Finnur Thorarinsson <finnur.webkit> | ||||||||||||
| Component: | WebCore Misc. | Assignee: | Eric Seidel (no email) <eric> | ||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||
| Severity: | Normal | CC: | darin, eric, sullivan | ||||||||||||
| Priority: | P2 | ||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||
| Hardware: | PC | ||||||||||||||
| OS: | Windows XP | ||||||||||||||
| Attachments: |
|
||||||||||||||
How about just "&& box->end() < end)" , plus maybe a brief comment? That seems a bit clearer... *** This bug has been marked as a duplicate of 16843 *** Created attachment 18396 [details]
Patch for the bug
Adding the patch as an attachment.
Duping the other way, since this one has a patch attached. *** Bug 16843 has been marked as a duplicate of this bug. *** Sure, I'll upload another patch with Peter's comments. Created attachment 18397 [details]
Patch taking Peter's comments into account
I changed:
box->end() + 1 <= end to
box->end() < end
... and moved the comment in the else clause above the if (since it applies to both the if clause and the else clause now).
Comment on attachment 18397 [details]
Patch taking Peter's comments into account
end() points to the last character in a run and not to the character after it.
The implementation of end():
unsigned end() const { return m_len ? m_start + m_len - 1 : m_start; }
I do not know why your particular test case is failing, but end() does point to the last character.
Comment on attachment 18397 [details]
Patch taking Peter's comments into account
Never mind, I get it.
Is it possible to create a layout test for this bug? Comment on attachment 18397 [details]
Patch taking Peter's comments into account
Why are we taking the comment out in the second half of the patch? The comment looks correct.
Also, there's no ChangeLog here and no test case! So why is this review+?
I also think that the terminology "points at" is confusing for an integer. I would not have used that terminology in comments. Instead I'd say "is the index of the last character, not the index past it" or something like that.
The comment is not being taken out, is being moved higher up since it applies to both the if and the else clause now (not just the else). I can change the wording of the comment, as you suggested, but do you really want the comment duplicated? I don't know why it was marked as review+, I think Dave Hyatt meant to put it back to review:? when he realized this is actually a bug (he changed it from review:? to review:- before realizing what the problem was). As for a layout test and a changelog: I am working with Eric Seidel on creating those. Comment on attachment 18397 [details] Patch taking Peter's comments into account (In reply to comment #12) > As for a layout test and a changelog: I am working with Eric Seidel on creating > those. Excellent! I'll clear the review flag, then. You can set to review? again once you have a regression test and a change log. Yeah, this is Finnur's first WebKit patch, and I told him on Friday I'd help him make a ChangeLog and test case. I got distracted by my own patches however. Created attachment 18444 [details]
Patch for the boundingBox issue
Comment on attachment 18444 [details]
Patch for the boundingBox issue
Patch looks good. It needs a change log and layout test, which is why I marked it review- before.
Created attachment 18445 I probably should not have changed the 'review' flag until I had finished uploading, as the first file got rejected while I was uploading the changelog. :) Comment on attachment 18445 ChangeLog, regression test, and patch should all be uploaded in a single patch, not separate patches. Created attachment 18446 [details]
Patch and changelog
Oh, sorry about that (I'm still learning how this works) :)
Let's see if this is more to your liking...
And, as you can see from the changelog Eric and I didn't see an easy way to test this (hence no layout test)... Comment on attachment 18446 [details]
Patch and changelog
The change is fine. The patch seems malformed. You said you'd post a new one later today, so we'll review that.
Created attachment 18448 [details]
Finally, I think I have the format of the patch correctly :)
The example in the steps to reproduce does show the problem in the Find highlights in Safari 3, which is good. I also do not know how to make a test case for this code since it isn't used in any code that affects the appearance of a web page. Comment on attachment 18448 [details] Finally, I think I have the format of the patch correctly :) Looks good. I plan to land this once the WebKit buildbots are green again: http://build.webkit.org/waterfall Landed in r29483. |
Version: local build, downloaded from tarball on Jan 10th, 2008. Platform and OS: Reproduced with Safari (which I hooked up to use my local build of WebKit) on Windows XP (didn't try Mac) To reproduce, start with this super simple web page: <html> <body> Ctrl-T<br> asdf<br> </body> </html> Then add the following code line to Frame::findString at the bottom: + IntRect bounds = resultRange->boundingBox(); return true; ... and set a breakpoint on the return statement. Now, in Safari, when you incrementally search for 'asdf', you will see the following boundingBox as the breakpoint hits: search (x, y) (w, h) -------------------------- a (8, 26) (14, 42) as (8, 26) (20, 42) asd (8, 26) (33, 42) <- clearly wrong, this is showing the boundingBox for 'asdf' instead of 'asd' asdf (8, 26) (33, 42) The reason for this error is that box->end() is assumed to point beyond the last character, when in fact it points *to* the last character (as specified in a comment in this function). WebKit uses box->end() in two locations in this function but only adds +1 to it in one location. The fix is therefore to add the missing +1. With the fix we get: search (x, y) (w, h) -------------------------- a (8, 26) (14, 42) as (8, 26) (20, 42) asd (8, 26) (28, 42) <- correct asdf (8, 26) (33, 42) I've been in touch with Eric Seidel and will work with him on what else I need to supply for this bug (if anything). In the meantime, here is the fix: =================================================================== --- RenderText.cpp (revision 29378) +++ RenderText.cpp (working copy) @@ -211,7 +211,7 @@ absolutePositionForContent(x, y); for (InlineTextBox* box = firstTextBox(); box; box = box->nextTextBox()) { - if (start <= box->start() && box->end() <= end) { + if (start <= box->start() && box->end() + 1 <= end) { IntRect r = IntRect(x + box->xPos(), y + box->yPos(), box->width(), box->height()); if (useSelectionHeight) { IntRect selectionRect = box->selectionRect(x, y, start, end);