Bug 16844

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:

Version:

528+ (Nightly build)

Hardware:

OS:

Windows XP

Attachments:

Description	Flags
Patch for the bug	none
Patch taking Peter's comments into account	none
Patch for the boundingBox issue	darin: review-
Patch and changelog	eric: review-
Finally, I think I have the format of the patch correctly :)	eric: review+

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	Details Formatted Diff Diff
Patch taking Peter's comments into account (1.06 KB, patch) 2008-01-11 11:47 PST, Finnur Thorarinsson	no flags	Details Formatted Diff Diff
Patch for the boundingBox issue (1.09 KB, patch) 2008-01-14 11:07 PST, Finnur Thorarinsson	darin: review-	Details Formatted Diff Diff
Patch and changelog (1.90 KB, patch) 2008-01-14 11:29 PST, Finnur Thorarinsson	eric: review-	Details Formatted Diff Diff
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+	Details Formatted Diff Diff
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.