Table's caption element not taken into account for table's offsetTop and offsetHeight values
Categories
(Core :: DOM: CSS Object Model, defect, P3)
Tracking
()
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)
1.55 KB,
text/html
|
Details | |
8.42 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
8.78 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
11.65 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
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.
![]() |
Reporter | |
Updated•12 years ago
|
![]() |
Reporter | |
Comment 1•12 years ago
|
||
Possible duplicate of bug 320809
![]() |
Assignee | |
Comment 2•12 years ago
|
||
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•6 years ago
|
||
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•6 years ago
|
||
When I said outerHeight in the 3rd paragraph I meant the native offsetHeight, which we use in outerHeight.
Updated•6 years ago
|
![]() |
Assignee | |
Comment 7•6 years ago
|
||
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....
![]() |
Assignee | |
Comment 8•6 years ago
|
||
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?
![]() |
Assignee | |
Updated•6 years ago
|
![]() |
Assignee | |
Comment 9•6 years ago
|
||
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).
![]() |
Assignee | |
Updated•6 years ago
|
![]() |
Assignee | |
Comment 10•6 years ago
|
||
![]() |
Assignee | |
Comment 11•6 years ago
|
||
![]() |
Assignee | |
Updated•6 years ago
|
Comment 12•6 years ago
|
||
(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•6 years ago
|
||
(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
![]() |
Assignee | |
Comment 14•6 years ago
|
||
> 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•6 years ago
|
||
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•6 years ago
|
||
(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•6 years ago
|
||
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
![]() |
Assignee | |
Comment 18•6 years ago
|
||
![]() |
Assignee | |
Updated•6 years ago
|
![]() |
Assignee | |
Comment 19•6 years ago
|
||
Sorry for the updated review request... the only changes were to the dom/html/test/test_bug375003-2.html test.
Comment 20•6 years ago
|
||
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•6 years ago
|
||
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•6 years ago
|
||
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*")
![]() |
Assignee | |
Comment 23•6 years ago
|
||
> Nit: I think you need to update the <title> on this test, too
Yep, done locally. Thank you!
Comment 24•6 years ago
|
||
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
Comment 25•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/8853777480d1 https://hg.mozilla.org/mozilla-central/rev/d891c10133ea https://hg.mozilla.org/mozilla-central/rev/0f9af414e717
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/11906 for changes under testing/web-platform/tests
Upstream PR merged
Comment 28•6 years ago
|
||
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•6 years ago
|
||
Should getBoundingClientRect
also work with the table wrapper box on tables?
Comment 30•6 years ago
|
||
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•6 years ago
|
||
*other browsers
Comment 32•6 years ago
|
||
Sorry for the comment spam. Here's an updated test case https://jsfiddle.net/timmywil/Lqrwhnxu/7/
![]() |
Assignee | |
Comment 33•6 years ago
|
||
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.
![]() |
Assignee | |
Comment 34•6 years ago
|
||
Bug 1520264 tracks comment 30.
Description
•