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

Status: RESOLVED FIXED

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

Description

[Martin Bouladour]

Attachment: Illustration of the bug

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.

Comment 1

[Martin Bouladour]

Possible duplicate of bug 320809

Comment 2

[Boris Zbarsky [:bzbarsky]]

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.

Comment 5

[Timmy Willison]

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.

Comment 6

[Timmy Willison]

When I said outerHeight in the 3rd paragraph I meant the native offsetHeight, which we use in outerHeight.

Comment 7

[Boris Zbarsky [:bzbarsky]]

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

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Make offset* properties for tables work with the table wrapper box, not the table box

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?

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Make offset* properties for tables work with the table wrapper box, not the table box

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).

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Make client* properties for tables work with the table wrapper box, not the table box

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Make scroll* properties for tables work with the table wrapper box, not the table box

Comment 12

[Daniel Holbert [:dholbert]]

(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.

Comment 13

[Daniel Holbert [:dholbert]]

(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

Comment 14

[Boris Zbarsky [: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.

Comment 15

[Daniel Holbert [:dholbert]]

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. :)

Comment 16

[Daniel Holbert [:dholbert]]

(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 17

[Daniel Holbert [:dholbert]]

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

Comment 18

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Make client* properties for tables work with the table wrapper box, not the table box

Comment 19

[Boris Zbarsky [:bzbarsky]]

Sorry for the updated review request... the only changes were to the dom/html/test/test_bug375003-2.html test.

Comment 20

[Daniel Holbert [:dholbert]]

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

Comment 21

[Daniel Holbert [:dholbert]]

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

Comment 22

[Daniel Holbert [:dholbert]]

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*")

Comment 23

[Boris Zbarsky [:bzbarsky]]

> Nit: I think you need to update the <title> on this test, too

Yep, done locally.   Thank you!

Comment 24

[Pulsebot]

Pushed by [e-mail]:
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

Comment 25

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/8853777480d1
https://hg.mozilla.org/mozilla-central/rev/d891c10133ea
https://hg.mozilla.org/mozilla-central/rev/0f9af414e717

Comment 26

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/11906 for changes under testing/web-platform/tests

Comment 27

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged

Comment 28

[Kohei Yoshino [:kohei]]

Posted a site compatibility note: https://www.fxsitecompat.com/en-CA/docs/2018/table-s-size-and-position-now-take-caption-into-account/

Comment 29

[Timmy Willison]

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

Comment 30

[Timmy Willison]

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).

Comment 31

[Timmy Willison]

*other browsers

Comment 32

[Timmy Willison]

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

Comment 33

[Boris Zbarsky [:bzbarsky]]

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.

Comment 34

[Boris Zbarsky [:bzbarsky]]

Bug 1520264 tracks comment 30.