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

RESOLVED FIXED in mozilla20

Status

()

Core
DOM: CSS Object Model
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: Daniel DeLorme, Assigned: Benedict Singer)

Tracking

16 Branch
mozilla20
x86_64
Linux
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mentor=bz])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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.

Updated

5 years ago
Attachment #673397 - Attachment mime type: text/plain → text/html

Updated

5 years ago
Component: Untriaged → Layout
Product: Firefox → Core

Updated

5 years ago
Component: Layout → DOM: CSS Object Model
Yeah, offsetTop/Left should probably skip table anonymous boxes.

If someone is interested in picking this up, I can point to the relevant code.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [mentor=bz]
(Assignee)

Comment 2

4 years ago
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.
(Assignee)

Comment 3

4 years ago
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.
Attachment #690681 - Flags: review?(bzbarsky)
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).
Attachment #690681 - Flags: review?(bzbarsky) → review+
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).
Assignee: nobody → singerb.dev
(Assignee)

Comment 6

4 years ago
Created attachment 690852 [details] [diff] [review]
Final patch.

Updated with nit from review.
Attachment #690681 - Attachment is obsolete: true
Attachment #690852 - Flags: review?(bzbarsky)
(Assignee)

Comment 7

4 years ago
(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 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.
Attachment #690852 - Flags: review?(bzbarsky) → review+
Actually, I can just make that as I land this.
http://hg.mozilla.org/integration/mozilla-inbound/rev/939a0ad6e8d6
Flags: in-testsuite+
Target Milestone: --- → mozilla20
https://hg.mozilla.org/mozilla-central/rev/939a0ad6e8d6
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.