803677 - offsetTop has wrong value for element with display:table-cell

Status: RESOLVED FIXED

Description

[Daniel DeLorme]

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 [:bzbarsky, bz on IRC]]

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]

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]

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 [:bzbarsky, bz on IRC]]

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] [:ted.mielczarek]]

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]

Created attachment 690852 [details] [diff] [review]
Final patch.

Updated with nit from review.

Comment 7

[Benedict Singer]

(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 [:bzbarsky, bz on IRC]]

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 [:bzbarsky, bz on IRC]]

Actually, I can just make that as I land this.

Comment 10

[Boris Zbarsky [:bzbarsky, bz on IRC]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/939a0ad6e8d6

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/939a0ad6e8d6