RESOLVED FIXED 16844
RenderText::addLineBoxRects erroneously includes last char for boundingBox 
https://bugs.webkit.org/show_bug.cgi?id=16844
Summary RenderText::addLineBoxRects erroneously includes last char for boundingBox
Finnur Thorarinsson
Reported 2008-01-11 11:05:11 PST
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);
Attachments
Patch for the bug (638 bytes, patch)
2008-01-11 11:31 PST, Finnur Thorarinsson 		no flags
DetailsFormatted DiffDiff
Patch taking Peter's comments into account (1.06 KB, patch)
2008-01-11 11:47 PST, Finnur Thorarinsson 		no flags
DetailsFormatted DiffDiff
Patch for the boundingBox issue (1.09 KB, patch)
2008-01-14 11:07 PST, Finnur Thorarinsson 		darin: review-
DetailsFormatted DiffDiff
Patch and changelog (1.90 KB, patch)
2008-01-14 11:29 PST, Finnur Thorarinsson 		eric: review-
DetailsFormatted DiffDiff
Finally, I think I have the format of the patch correctly :) (2.34 KB, patch)
2008-01-14 13:50 PST, Finnur Thorarinsson 		eric: review+
DetailsFormatted DiffDiff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.
Peter Kasting
Comment 1 2008-01-11 11:29:06 PST
How about just "&& box->end() < end)" , plus maybe a brief comment? That seems a bit clearer...
Mark Rowe (bdash)
Comment 2 2008-01-11 11:30:52 PST
*** This bug has been marked as a duplicate of 16843 ***
Finnur Thorarinsson
Comment 3 2008-01-11 11:31:00 PST
Created attachment 18396 [details] Patch for the bug Adding the patch as an attachment.
Eric Seidel (no email)
Comment 4 2008-01-11 11:34:10 PST
Duping the other way, since this one has a patch attached.
Eric Seidel (no email)
Comment 5 2008-01-11 11:34:36 PST
*** Bug 16843 has been marked as a duplicate of this bug. ***
Finnur Thorarinsson
Comment 6 2008-01-11 11:37:54 PST
Sure, I'll upload another patch with Peter's comments.
Finnur Thorarinsson
Comment 7 2008-01-11 11:47:16 PST
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).
Dave Hyatt
Comment 8 2008-01-11 11:52:04 PST
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.
Dave Hyatt
Comment 9 2008-01-11 11:53:44 PST
Comment on attachment 18397 [details] Patch taking Peter's comments into account Never mind, I get it.
Mark Rowe (bdash)
Comment 10 2008-01-13 02:31:57 PST
Is it possible to create a layout test for this bug?
Darin Adler
Comment 11 2008-01-13 11:30:14 PST
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.
Finnur Thorarinsson
Comment 12 2008-01-13 11:47:44 PST
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.
Darin Adler
Comment 13 2008-01-13 12:24:43 PST
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.
Eric Seidel (no email)
Comment 14 2008-01-13 14:48:00 PST
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.
Finnur Thorarinsson
Comment 15 2008-01-14 11:07:33 PST
Created attachment 18444 [details] Patch for the boundingBox issue
Darin Adler
Comment 16 2008-01-14 11:10:26 PST
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.
Finnur Thorarinsson
Comment 17 2008-01-14 11:22:00 PST
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. :)
Darin Adler
Comment 18 2008-01-14 11:24:08 PST
Comment on attachment 18445 ChangeLog, regression test, and patch should all be uploaded in a single patch, not separate patches.
Finnur Thorarinsson
Comment 19 2008-01-14 11:29:31 PST
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...
Finnur Thorarinsson
Comment 20 2008-01-14 11:32:12 PST
And, as you can see from the changelog Eric and I didn't see an easy way to test this (hence no layout test)...
Eric Seidel (no email)
Comment 21 2008-01-14 13:42:45 PST
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.
Finnur Thorarinsson
Comment 22 2008-01-14 13:50:11 PST
Created attachment 18448 [details] Finally, I think I have the format of the patch correctly :)
John Sullivan
Comment 23 2008-01-14 14:19:21 PST
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.
Eric Seidel (no email)
Comment 24 2008-01-14 16:51:30 PST
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
Eric Seidel (no email)
Comment 25 2008-01-14 17:32:33 PST
Landed in r29483.
Note You need to log in before you can comment on or make changes to this bug.