Bug 23178 - offsetTop is wrong in some cases
Summary: offsetTop is wrong in some cases
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Grace Kloba
URL: http://finance.google.com/finance?q=aapl
Keywords:
Depends on:
Blocks:
 
Reported: 2009-01-07 16:42 PST by Grace Kloba
Modified: 2009-02-03 15:53 PST (History)
0 users

See Also:


Attachments
A simple test case. (566 bytes, text/html)
2009-01-07 16:43 PST, Grace Kloba
no flags Details
The patch to fix the bug (1.25 KB, patch)
2009-01-08 15:16 PST, Grace Kloba
darin: review-
Details | Formatted Diff | Diff
patch with layout test file (2.99 KB, patch)
2009-01-09 13:01 PST, Grace Kloba
darin: review-
Details | Formatted Diff | Diff
Include layout test and expected result for the proposed patch (3.53 KB, patch)
2009-01-12 13:40 PST, Grace Kloba
darin: review+
Details | Formatted Diff | Diff
Remove tab in the text file. (3.56 KB, patch)
2009-01-12 18:14 PST, Grace Kloba
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Grace Kloba 2009-01-07 16:42:24 PST
Load the site, "http://finance.google.com/finance?q=aapl".

Click on the search box and type "aapl".

It should pop up an auto-complete list and the list is currently too high so it overlaps the text input box.

I have narrowed it down to a simple case with attached file.
Comment 1 Grace Kloba 2009-01-07 16:43:17 PST
Created attachment 26512 [details]
A simple test case.
Comment 2 Grace Kloba 2009-01-07 17:21:23 PST
This seems to be caused by http://trac.webkit.org/changeset/35551.

Instead of 

    int y = yPos() - borderTopExtra() - offsetPar->borderTop();

it should be

    int y = yPos() - borderTopExtra() + offsetPar->borderTopExtra() - offsetPar->borderTop();
Comment 3 Grace Kloba 2009-01-08 15:16:09 PST
Created attachment 26541 [details]
The patch to fix the bug
Comment 4 Darin Adler 2009-01-08 16:09:53 PST
Comment on attachment 26541 [details]
The patch to fix the bug

This fix looks great!

But we need a regression test for it. Please add a test to LayoutTests that demonstrates the bug is fixed.
Comment 5 Grace Kloba 2009-01-09 13:01:19 PST
Created attachment 26570 [details]
patch with layout test file
Comment 6 Darin Adler 2009-01-10 10:28:45 PST
Comment on attachment 26570 [details]
patch with layout test file

Looking good.

> +2009-01-09  Grace Kloba  <klobag@gmail.com>
> +
> +	https://bugs.webkit.org/show_bug.cgi?id=23178
> +	Added a case to test the offsetTop of table cell's children.
> +
> +        * fast/dom/Element/offsetTop-table-cell.html:

This change log entry has tabs in it. We need one that uses spaces instead.

> Index: LayoutTests/fast/dom/Element/offsetTop-table-cell.html
> ===================================================================
> --- LayoutTests/fast/dom/Element/offsetTop-table-cell.html	(revision 39716)
> +++ LayoutTests/fast/dom/Element/offsetTop-table-cell.html	(working copy)

Since this test has changed, then the expected results have also changed. We need a patch that includes the changes to the expected results files too.
Comment 7 Grace Kloba 2009-01-12 13:40:45 PST
Created attachment 26647 [details]
Include layout test and expected result for the proposed patch
Comment 8 Darin Adler 2009-01-12 14:23:50 PST
Comment on attachment 26647 [details]
Include layout test and expected result for the proposed patch

> +	https://bugs.webkit.org/show_bug.cgi?id=23178
> +	Added a case to test the offsetTop of table cell's children.

There are tabs here, which make it hard to land the patch.

> +			<div style="margin: 50pt 0pt 50pt 0pt;"></div>

You want px here, not pt. I'm surprised the test works if you're specifying pt, since they are not the same as px.

Did you try the test case in other browsers? Does Firefox give the same results?

I'm going to say r=me but I am slightly worried about the pt in the test case. And I want to be sure we're matching the other browsers here -- the fix looks clearly right, but I'd like to see the test to be sure.
Comment 9 Grace Kloba 2009-01-12 18:14:40 PST
Created attachment 26656 [details]
Remove tab in the text file.

I have changed it to use px instead of pt. For some reason, px and pt gave the same result on both Safari and FireFox.
Comment 10 Darin Adler 2009-01-12 18:16:34 PST
Comment on attachment 26656 [details]
Remove tab in the text file.

Looks fine.

LayoutTests/ChangeLog still has tabs in it, but I guess that’s OK.
Comment 11 Brent Fulgham 2009-02-03 15:43:58 PST
This bug was resolved in the recent RenderBox changes (http://trac.webkit.org/browser/trunk/WebCore/rendering/RenderObject.cpp?rev=40107#L189)
Comment 12 Brent Fulgham 2009-02-03 15:53:46 PST
Test cases committed in @r40552.