Closed Bug 820891 Opened 7 years ago Closed Last year

Table's caption element not taken into account for table's offsetTop and offsetHeight values

Categories

(Core :: DOM: CSS Object Model, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: martin.bouladour, Assigned: bzbarsky)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Keywords: site-compat)

Attachments

(4 files, 2 obsolete files)

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20100101 Firefox/17.0
Build ID: 20121130221340

Steps to reproduce:

In a Javascript script, I got offsetTop and offsetHeight values of a table element containing a caption element. Also, I got the computed height of the table (using window.getComputedStyle(table).height).


Actual results:

Those values did not take into account the dimensions and position of the caption element. They acted as if the caption element was not part of the table element. See the attached "Illustration of the bug" using Firefox 17.


Expected results:

I believe that those values should have included the caption element's dimensions and position. See the attached "Illustration of the bug" using another browser than Firefox.

I didn't find any information in the W3C specs that contradicts Firefox's behavior here -- but then again, nothing that justifies it either. Nevertheless, caption being a child of table, my intuition says it should be take into account for offsetTop and offsetHeight. Moreover, the behaviors of IE, Chrome, Safari and Opera are all coherent, while Firefox is the only one that does it differently.
Attachment #691385 - Attachment mime type: text/plain → text/html
Possible duplicate of bug 320809
The problem here is that tables generate two separate CSS boxes: the outer box (which includes the caption) and the inner one (which does not).

Then when you ask for information about a box the question becomes "which box".  Note that if we used the outer box in this case your width would be wrong because for various reasons the outer box here extends all the way across the body.
Component: General → DOM: CSS Object Model
Status: UNCONFIRMED → RESOLVED
Closed: 5 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 320809
Status: RESOLVED → UNCONFIRMED
Depends on: 320809
Resolution: DUPLICATE → ---
Duplicate of this bug: 1043294
OS: Linux → All
Hardware: x86_64 → All
Version: 17 Branch → Trunk
Forgive me for commenting on such an old ticket, but I'll do it anyway since this one is still open.

We got a ticket for this recently in jQuery. See https://github.com/jquery/jquery/issues/4105 and test case: https://jsfiddle.net/a6c4jdt3/16/.

We discussed this in a meeting and we're confused about what the spec actually says. There seems to be a distinction between "border edge" and client rect. The table border is not applied around the caption, so by that argument it makes sense that outerHeight would not include the caption. However, I doubt there's more than a couple users that would not be surprised by that behavior. Note that Chrome and Safari do include the caption in `.outerHeight`, so there's a browser inconsistency that needs to be addressed by us either way. But I honestly don't know which direction to take it.

I'm not saying either behavior is desirable. In fact, I expected the border to surround the caption as well. I could put a border on a tbody instead if I wanted to exclude the caption from the border, but that particular behavior is consistent across browsers and I understand it's not likely to change.
When I said outerHeight in the 3rd paragraph I meant the native offsetHeight, which we use in outerHeight.
Priority: -- → P3
The current spec for this at https://drafts.csswg.org/cssom-view/#dom-htmlelement-offsetheight says to use the first CSS layout box associated with the element, which would be the table wrapper.

We should probably consider switching to that....
Flags: needinfo?(bzbarsky)
I'd ask Mats for review normally, since he's been in this code a lot before, but he's on vacation.  Daniel, do you feel comfortable reviewing this?
Attachment #8990822 - Flags: review?(dholbert)
Assignee: nobody → bzbarsky
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
It's not quite clear to me why the "Caption with margin" test ends up with an
offsetWidth that ignores the caption's margin.   In general, I would expect that
horizontal margin to affect the various sizes on a table-outer frame (and it
does for purposes of client*, for example, in the next diff on this bug).
Attachment #8990823 - Flags: review?(dholbert)
Attachment #8990822 - Attachment is obsolete: true
Attachment #8990822 - Flags: review?(dholbert)
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #8)
> I'd ask Mats for review normally, since he's been in this code a lot before,
> but he's on vacation.  Daniel, do you feel comfortable reviewing this?

Sure, I'm happy to review here.
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #9)
> It's not quite clear to me why the "Caption with margin" test ends up with an
> offsetWidth that ignores the caption's margin.

Probably because the API here (nsGenericHTMLElement::GetOffsetRect) uses nsLayoutUtils::GetAllInFlowRectsUnion, which (a few layers down) uses nsLayoutUtils::AddBoxesForFrame, which has a special case for pseudoType == nsCSSAnonBoxes::tableWrapper.  In that special case, we add the boxes for the table and the caption, but not for the table-wrapper (i.e. not aFrame itself).

Maybe we want to skip that special case and just do the standard thing there? (i.e. AddBox for the wrapper-frame itself, which presumably includes the space for the caption's margin)

Source link:
https://dxr.mozilla.org/mozilla-central/rev/085cdfb90903d4985f0de1dc7786522d9fb45596/layout/base/nsLayoutUtils.cpp#4036-4041,4051
Flags: needinfo?(bzbarsky)
> Maybe we want to skip that special case and just do the standard thing there?

Ah.

We would have to audit other consumers of GetAllInFlowRectsUnion and see why the special-case got added.  Sounds like followup fodder.  I filed bug 1474448 on that.
Blocks: 1474448
Flags: needinfo?(bzbarsky)
Comment on attachment 8990823 [details] [diff] [review]
part 1.  Make offset* properties for tables work with the table wrapper box, not the table box

Review of attachment 8990823 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, with one caveat: assuming you agree that comment 13 is a likely explanation for the "Caption with margin" failure, could you file a followup bug and mention it in the table-offset-props.html.ini file and in the extended-commit-message discussion of that failure here.

Or, even better, maybe fix that issue in this patch or another patch on this bug. :)
Attachment #8990823 - Flags: review?(dholbert) → review+
(In reply to Daniel Holbert [:dholbert] from comment #15)
> r=me, with one caveat: assuming you agree that comment 13 is a likely
> explanation for the "Caption with margin" failure, could you file a followup
> bug and mention it in the table-offset-props.html.ini file and in the
> extended-commit-message discussion of that failure here.
> 
> Or, even better, maybe fix that issue in this patch or another patch on this
> bug. :)

Ah, just saw your reply -- yeah, let's update the commit message and the .ini failure file to mention bug 1474448 then.
Comment on attachment 8990824 [details] [diff] [review]
part 2.  Make client* properties for tables work with the table wrapper box, not the table box

Review of attachment 8990824 [details] [diff] [review]:
-----------------------------------------------------------------

r=me
Attachment #8990824 - Flags: review?(dholbert) → review+
Attachment #8990824 - Attachment is obsolete: true
Sorry for the updated review request... the only changes were to the dom/html/test/test_bug375003-2.html test.
Comment on attachment 8990842 [details] [diff] [review]
part 2.  Make client* properties for tables work with the table wrapper box, not the table box

Review of attachment 8990842 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #19)
> Sorry for the updated review request... the only changes were to the
> dom/html/test/test_bug375003-2.html test.

no prob; those changes look sane, I didn't bother to dig too far.

r=me
Attachment #8990842 - Flags: review?(dholbert) → review+
Comment on attachment 8990826 [details] [diff] [review]
part 3.  Make scroll* properties for tables work with the table wrapper box, not the table box

Review of attachment 8990826 [details] [diff] [review]:
-----------------------------------------------------------------

r=me
Attachment #8990826 - Flags: review?(dholbert) → review+
Comment on attachment 8990826 [details] [diff] [review]
part 3.  Make scroll* properties for tables work with the table wrapper box, not the table box

Review of attachment 8990826 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/web-platform/tests/css/cssom-view/table-scroll-props.html
@@ +71,5 @@
>  
>      for (var t of tests) {
>        test(function() {
>          target().innerHTML = t[0];
> +        assert_equals(table().scrollWidth, t[1], t[3] + " scrollWidth");

Nit: I think you need to update the <title> on this test, too, like you did in part 2 when copying the test from part 1.

(Looks like this is a "hg cp" of table-client-props.html, so it's probably inheriting the title of that testcase, which is "client* properties on tables", but in htis new copy, it should say "scroll*")
> Nit: I think you need to update the <title> on this test, too

Yep, done locally.   Thank you!
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8853777480d1
part 1.  Make offset* properties for tables work with the table wrapper box, not the table box.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/d891c10133ea
part 2.  Make client* properties for tables work with the table wrapper box, not the table box.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f9af414e717
part 3.  Make scroll* properties for tables work with the table wrapper box, not the table box.  r=dholbert
https://hg.mozilla.org/mozilla-central/rev/8853777480d1
https://hg.mozilla.org/mozilla-central/rev/d891c10133ea
https://hg.mozilla.org/mozilla-central/rev/0f9af414e717
Status: ASSIGNED → RESOLVED
Closed: 5 years agoLast year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/11906 for changes under testing/web-platform/tests
Depends on: 1474974

Should getBoundingClientRect also work with the table wrapper box on tables?

Nevermind, I thought we were using getBoundingClientRect internally to get height, but we're using window.getComputedStyle(elem).height. I just wonder if this should also include the caption height (note that it does in browsers).

*other browsers

Sorry for the comment spam. Here's an updated test case https://jsfiddle.net/timmywil/Lqrwhnxu/7/

getComputedStyle says to use the "used value" for height.

The invariant here should ideally be that if you set the height the the thing getComputedStyle returned then the layout should not change. It doesn't sound like that's true in other browsers...

I suggest filing a separate bug on the getComputedStyle bit here and ccing me and ":emilio" on it.

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