Bug 19094

Summary: offsetTop is wrong in cell <td>
Product: WebKit Reporter: Jean-S├ębastien Goupil <jsgoupil>
Component: Layout and RenderingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Major CC: bdakin, hyatt, kruepl, mitz
Priority: P1    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
patch that corrects offsetTop for table cells
none
patch that corrects offsetTop for table cells with test hyatt: review+

Description Jean-S├ębastien Goupil 2008-05-15 17:08:24 PDT
When you have a cell that specifies its height, the offsetTop will be based on where the text will be written. It should be based where the TD starts. The workaround is to set the vertical-align: top to the TD.
We tried with the latest version of Safari3.1.1(527.17) on Windows2003 and the night build on MAC.

This is the HTML

<html>
<head>
<script type="text/javascript">
function loaded() {
	alert("Offset of the TD: " + document.getElementById("thetd").offsetLeft + "X" + document.getElementById("thetd").offsetTop);
}
</script>
</head>
<body onload="loaded()" id="body">
<table style="background-color: orange;"><tr style="background-color: blue;"><td id="thetd" style="/*vertical-align:top;*/width: 100px; height: 500px;background-color: red"></td></tr></table>
</body>
</html>
Comment 1 Bernhard Kruepl 2008-08-02 15:59:12 PDT
Created attachment 22620 [details]
patch that corrects offsetTop for table cells
Comment 2 Bernhard Kruepl 2008-08-02 16:12:51 PDT
This bug hit me when I wanted to extract table cell coordinates (latest SVN, OSX 10.5.4).

Quite strangely, offsetTop lies intentionally:

RenderTableCell.h, line100:

    // Lie about position to outside observers.
    virtual int yPos() const { return m_y + m_topExtra; }

My patch corrects the lie by subtracting the borderTopExtra again. It breaks the regression, 
fast/dom/Element/offsetTop-table-cell.html explicitly asks for wrong values. I think this 
test is just wrong and should also be patched.

Comment 3 Bernhard Kruepl 2008-08-03 05:58:12 PDT
Created attachment 22623 [details]
patch that corrects offsetTop for table cells with test

I created a patch for offsetTop, updated and extended the appropriate test. I also created a changelog entry. Hope this meets all requirements, the bug seems to have been present for eight years now...
Comment 4 Eric Seidel (no email) 2008-08-04 01:36:47 PDT
Beth is the right person to review this.
Comment 5 Beth Dakin 2008-08-04 12:36:15 PDT
I have a feeling that there is a reason offsetTop lied intentionally. I don't know why it was coded that way originally though. I am cc-ing Hyatt because he might have more information about that. But I am very worried about this change, especially considering the test and the comment.
Comment 6 Dave Hyatt 2008-08-04 12:44:44 PDT
Comment on attachment 22623 [details]
patch that corrects offsetTop for table cells with test

This looks good actually.

r=me.
Comment 7 Dave Hyatt 2008-08-04 12:45:08 PDT
offsetTop is lying unintentionally because it used yPos().

Comment 8 mitz 2008-08-04 13:27:39 PDT
The isTableCell() check is an unnecessary virtual method call, because borderTopExtra() is 0 for anything else. If I land this patch, I will just remove it.
Comment 9 Bernhard Kruepl 2008-08-04 16:31:49 PDT
Fair enough. And you are right, lying was yPos()'s intention, not offsetTop's.
Comment 10 mitz 2008-08-04 21:10:26 PDT
Landed in <http://trac.webkit.org/changeset/35551>.