Bug 19094 - offsetTop is wrong in cell <td>
Summary: offsetTop is wrong in cell <td>
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P1 Major
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-05-15 17:08 PDT by Jean-Sébastien Goupil
Modified: 2008-08-04 21:10 PDT (History)
4 users (show)

See Also:


Attachments
patch that corrects offsetTop for table cells (603 bytes, patch)
2008-08-02 15:59 PDT, Bernhard Kruepl
no flags Details | Formatted Diff | Diff
patch that corrects offsetTop for table cells with test (6.82 KB, patch)
2008-08-03 05:58 PDT, Bernhard Kruepl
hyatt: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>.