Bug 19094

Summary:

offsetTop is wrong in cell <td>

Product:

WebKit

Reporter:

Jean-Sébastien Goupil <jsgoupil>

Component:

Layout and Rendering

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Major

CC:

bdakin, hyatt, kruepl, mitz

Priority:

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+

Jean-Sébastien Goupil

Reported 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>

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
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Bernhard Kruepl

Comment 1 2008-08-02 15:59:12 PDT

Created attachment 22620 [details] patch that corrects offsetTop for table cells

Bernhard Kruepl

Comment 2 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.

Bernhard Kruepl

Comment 3 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...

Eric Seidel (no email)

Comment 4 2008-08-04 01:36:47 PDT

Beth is the right person to review this.

Beth Dakin

Comment 5 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.

Dave Hyatt

Comment 6 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.

Dave Hyatt

Comment 7 2008-08-04 12:45:08 PDT

offsetTop is lying unintentionally because it used yPos().

mitz

Comment 8 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.

Bernhard Kruepl

Comment 9 2008-08-04 16:31:49 PDT

Fair enough. And you are right, lying was yPos()'s intention, not offsetTop's.

mitz

Comment 10 2008-08-04 21:10:26 PDT

Landed in <http://trac.webkit.org/changeset/35551>.

Note You need to log in before you can comment on or make changes to this bug.