Bug 23178 – offsetTop is wrong in some cases

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Grace Kloba]

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.

[Grace Kloba]

Created attachment 26512 [details]
A simple test case.

[Grace Kloba]

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();

[Grace Kloba]

Created attachment 26541 [details]
The patch to fix the bug

[Darin Adler]

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.

[Grace Kloba]

Created attachment 26570 [details]
patch with layout test file

[Darin Adler]

Comment on attachment 26570 [details]
patch with layout test file

Looking good.

> [phone]  Grace Kloba  <[e-mail]>
> +
> +	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.

[Grace Kloba]

Created attachment 26647 [details]
Include layout test and expected result for the proposed patch

[Darin Adler]

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.

[Grace Kloba]

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.

[Darin Adler]

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.

[Brent Fulgham]

This bug was resolved in the recent RenderBox changes (http://trac.webkit.org/browser/trunk/WebCore/rendering/RenderObject.cpp?rev=40107#L189)

[Brent Fulgham]

Test cases committed in @r40552.