Last Comment Bug 803677 - offsetTop has wrong value for element with display:table-cell
: offsetTop has wrong value for element with display:table-cell
Status: RESOLVED FIXED
[mentor=bz]
:
Product: Core
Classification: Components
Component: DOM: CSS Object Model (show other bugs)
: 16 Branch
: x86_64 Linux
: -- normal (vote)
: mozilla20
Assigned To: Benedict Singer
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-10-19 13:25 PDT by Daniel DeLorme
Modified: 2012-12-12 02:05 PST (History)
5 users (show)
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
incorrect cumulative offset when table-cell element is not first in parent (408 bytes, text/html)
2012-10-19 13:25 PDT, Daniel DeLorme
no flags Details
Initial patch. (3.89 KB, patch)
2012-12-10 19:10 PST, Benedict Singer
bzbarsky: review+
Details | Diff | Review
Final patch. (3.90 KB, patch)
2012-12-11 06:09 PST, Benedict Singer
bzbarsky: review+
Details | Diff | Review

Description Daniel DeLorme 2012-10-19 13:25:28 PDT
Created attachment 673397 [details]
incorrect cumulative offset when table-cell element is not first in parent

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:16.0) Gecko/20100101 Firefox/16.0
Build ID: 20121010234852

Steps to reproduce:

Set display:table-cell on an element where the parent is not table or table-row, and the previous sibling is not table-cell.


Actual results:

offsetTop is zero. This could be due to the table-cell having an *implied* container which has a non-zero offsetTop. But since offsetParent does not reference that implied container, if we try to compute the cumulative offsetTop by going through each offsetParent, we get the wrong value.

I imagine the same problem occurs with offsetLeft.


Expected results:

Either offsetTop should have a non-zero value, or offsetParent should be the implied parent of the table-cell element.
Comment 1 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-10-22 09:26:09 PDT
Yeah, offsetTop/Left should probably skip table anonymous boxes.

If someone is interested in picking this up, I can point to the relevant code.
Comment 2 Benedict Singer 2012-12-09 16:32:17 PST
Working on a patch for this. Right now I my fix deals with this bug, but then I break content/html/content/test/test_bug375003-1.html. Still, I'm hopeful I'll get it right soon.
Comment 3 Benedict Singer 2012-12-10 19:10:42 PST
Created attachment 690681 [details] [diff] [review]
Initial patch.

Ignore anonymous tables when determining offsetParent (the source of offsetTop, etc) by looking at the content element as well as the frame type, and rejecting table frames that aren't from a real table.
Comment 4 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-12-10 19:32:33 PST
Comment on attachment 690681 [details] [diff] [review]
Initial patch.

r=me if you move the computation of isRealTable to after we've checked the frame type, since the common case is to fail the frametype check.

Also, worth filing a followup to think about reimplementing this whole thing in terms of the element tree (which is what the spec requires and is somewhat different from our current behavior).
Comment 5 Ted Mielczarek [:ted.mielczarek] 2012-12-11 05:44:52 PST
Benedict: I just gave your account "editbugs" permissions, so you should be able to assign bugs to yourself now (and fiddle some other bug flags).
Comment 6 Benedict Singer 2012-12-11 06:09:40 PST
Created attachment 690852 [details] [diff] [review]
Final patch.

Updated with nit from review.
Comment 7 Benedict Singer 2012-12-11 06:18:20 PST
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #5)
> Benedict: I just gave your account "editbugs" permissions, so you should be
> able to assign bugs to yourself now (and fiddle some other bug flags).

Thanks Ted!

(In reply to Boris Zbarsky (:bz) from comment #4)
> Comment on attachment 690681 [details] [diff] [review]
> Initial patch.
> 
> Also, worth filing a followup to think about reimplementing this whole thing
> in terms of the element tree (which is what the spec requires and is
> somewhat different from our current behavior).

Followup filed as bug 820364.
Comment 8 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-12-11 14:39:31 PST
Comment on attachment 690852 [details] [diff] [review]
Final patch.

You don't need the else-after-return.  So the code should look like this:

  if (cell or table) {
    return element_is_right_type;
  }
  return false;

r=me with that.  No need for re-review for this change.
Comment 9 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-12-11 14:39:48 PST
Actually, I can just make that as I land this.
Comment 10 Boris Zbarsky [:bz] (Out June 25-July 6) 2012-12-11 14:51:21 PST
http://hg.mozilla.org/integration/mozilla-inbound/rev/939a0ad6e8d6
Comment 11 Ed Morley [:emorley] 2012-12-12 02:05:32 PST
https://hg.mozilla.org/mozilla-central/rev/939a0ad6e8d6

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