Opened 10 years ago
Closed 10 years ago
#14084 closed bug (fixed)
elem.css('width') provides incorrect output with `box-sizing: border-box` if run before document ready
Reported by: | Owned by: | m_gol | |
---|---|---|---|
Priority: | low | Milestone: | 1.11/2.1 |
Component: | support | Version: | 1.10.1 |
Keywords: | Cc: | ||
Blocked by: | Blocking: |
Description
I believe the current documentation for height
and width
is incorrect vis-a-vis what .css("height")
and .css("width")
do with box-sizing: border-box
elements. The current documentation for height
says:
Note that
.height()
will always return the content height, regardless of the value of the CSSbox-sizing
property. As of jQuery 1.8, this may require retrieving the CSS height plusbox-sizing
property and then subtracting any potential border and padding on each element when the element hasbox-sizing: border-box
. To avoid this penalty, use.css("height")
rather than.height()
.
(My emphasis. And similarly for .width
/ .css('width')
.)
This suggests that .css("height")
will return something different from .height()
, but it returns exactly the same thing (plus units), regardless of whether the element is using border-box
or content-box
(I didn't look at padding-box
). (And similarly for .css("width")
and .width()
.)
For example, this:
<style> div { height: 120px; width: 100px; padding-top: 20px; padding-left: 10px; background-color: #abc; border: 1px solid black; } .box { box-sizing: border-box; -moz-box-sizing: border-box; } .content { } </style> <div class="box"></div> <div class="content"></div> <script> showDimensions('box'); showDimensions('content'); function showDimensions(cls) { var elm = $('.' + cls), label = "$('." + cls + "')"; console.log(label + ".height() = " + elm.height()); console.log(label + ".css('height') = " + elm.css('height')); console.log(label + ".width() = " + elm.width()); console.log(label + ".css('width') = " + elm.css('width')); } </script>
...gives us:
$('.box').height() = 98
$('.box').css('height') = 98px
$('.box').width() = 88
$('.box').css('width') = 88px
$('.content').height() = 120
$('.content').css('height') = 120px
$('.content').width() = 100
$('.content').css('width') = 100px
Live Example | Live Source (using v1.10.1)
Also, the behavior is the same in v1.6.4 (demo), v1.7.2 (demo), v1.8.3 (demo), v1.9.1 (demo), and v1.10.1 (above), whereas the documentation suggests some change occurred in v1.8.
Change History (33)
comment:1 Changed 10 years ago by
comment:2 Changed 10 years ago by
That is really strange, I played with the jsbin version and it does look broken, but the jsFiddle is what I expected.
comment:3 Changed 10 years ago by
@m_gol, @dmethvin: That is really strange! I can confirm that it behaves differently on jsFiddle as well (I get different numbers rather than the same ones), which is...well...strange.
I don't seem to have the rights to add a file attachment, but this completely standalone version replicates the issue for me (numbers are the same, like jsbin), both with edge and 1.8.3.
Hopefully it's just me doing something silly, but it's not immediately obvious to me what.
comment:4 Changed 10 years ago by
comment:5 Changed 10 years ago by
comment:6 Changed 10 years ago by
This is because jQuery runs some tests after document ready fired and one of these tests is checking boxSizing support. jsFiddle must be injecting scripts asynchronously while jsBin & static pages don't; inline code is executed before tests run which makes jQuery believe boxSizing is not supported in this browser. If you put this code in a document ready call, it works.
We should fix it, IMHO. It seems intuitive for users to be able to assume that JS code put at the end of body doesn't need to wait for document ready. This assumption isn't always true as we can see in this example.
comment:7 Changed 10 years ago by
We could "fix" it for 2.x by setting jQuery.support.boxSizing to true as all supported by 2.x browsers have it true.
It would be best to actually remove this property but I believe we need it for backwards compatibility with 1.x where it's needed - unfortunately jQuery.support is exposed. :(
As for 1.x, I don't see any quick & pretty solution.
comment:8 Changed 10 years ago by
Summary: | `height`/`width` documentation incorrect about `css` and box-sizing: border-box → elem.css('width') provides incorrect output with `box-sizing: border-box` if run before document ready |
---|
comment:9 Changed 10 years ago by
Component: | unfiled → support |
---|---|
Priority: | undecided → low |
Status: | new → open |
iirc, assuming the document is ready at the end of the body can have other issues as well, irrespective of whether jQuery is on the page. But perhaps that was only oldIE? One things seems certain from our experience: we should not let this ticket encourage us to pull support tests requiring the body
out of document ready. We've done this before and it was fraught with problems.
comment:10 Changed 10 years ago by
That's interesting, Timmy, can you share info about some problems of this nature? I've seen suggestions that putting scripts at the end of body is equivalent to invoking them on doc ready so many times...
comment:11 Changed 10 years ago by
And what do you think about assuming jQuery.support.boxSizing === true
in 2.x?
comment:12 Changed 10 years ago by
Did a little searching. I was vaguely recalling a few things.
It is true that scripts running at the end of the body is basically the same as putting them at the end of the body, I would not officially recommend it because there are several conditions that could cause unwanted behavior. This answer on SO goes in more detail. Putting scripts at the end of the body is the same because ALL of the HTML source has been parsed by the browser. The point of the post is, be really sure you're not using something that hasn't been parsed yet (scripts using document.write
or script tags anywhere besides the end of the body can cause errors).
Having said that, there is a difference, according to the spec. Any scripts using the defer
flag are executed _before_ the ready event (under certain circumstances), but not before scripts at the end of the body. Just good to be aware of that.
Finally, in oldIE, there was the operation aborted event, but that could be avoided by ensuring the script tag's parent is actually the body.
But yes, I'd be fine with defaulting to true.
comment:13 Changed 10 years ago by
Given the oft-repeated recommendation to put scripts at the end of body, barring very, very solid reasons contrary, I wouldn't recommend jQuery get things wrong in that situation. Certainly there's no reason this box sizing detect can't be more proactive.
comment:14 Changed 10 years ago by
Certainly there's no reason this box sizing detect can't be more proactive.
Great, can you submit a more proactive patch? The trick is supporting 1.x I think.
comment:15 Changed 10 years ago by
@dmethvin:
Great, can you submit a more proactive patch?
Sure! (And thanks for the invite.) Give me a few days to shake some time free, particularly busy work schedule this week.
What's best, fork on github and do a pull request?
comment:16 Changed 10 years ago by
Definitely fork and pull request, more information is at http://contribute.jquery.org if you need it. Many thanks for your past bug reports, they're exemplars. We'd love to have your name in AUTHORS.txt.
comment:17 Changed 10 years ago by
Just to be clear, no one is arguing against putting scripts at the end of the body. The issue is document ready and whether we can get around needing a body for certain support tests.
comment:18 Changed 10 years ago by
@dmethvin, @m_gol: I'd like to get your input on this if I could.
As you know, there are two relevant support
properties: boxSizing
and boxSizingReliable
. Currently, the pre-ready
code doesn't set boxSizing
at all but sets boxSizingReliable
to true
, then the ready
code sets boxSizing
and optionally changes boxSizingReliable
to false
.
Re boxSizing
:
The current test uses measurement, and so waits for body
(the measurement doesn't work when the element is disconnected). I've tried several browsers* and it seems like the fairly common feature-sniff approach of testing for the property on an element's style
works reliably before body
exists. E.g., prior to the ready
handler:
$.each('boxSizing MozBoxSizing msBoxSizing webkitBoxSizing OBoxSizing'.split(' '), function(i, prop) { if (prop in div.style) { support.boxSizing = true; return false; } });
or more briefly but less readably
$.each('boxSizing MozBoxSizing msBoxSizing webkitBoxSizing OBoxSizing'.split(' '), function(i, prop) { return !(support.boxSizing = prop in div.style); });
where div
is an element that already exists in the function for various other tests.
Does anyone know of a reason I shouldn't do that? Known invalid results or similar? It would also have the advantage that it wouldn't be subject to bug #13543, so no need for the swapping test that fixed it (nice one, @m_gol).
Re boxSizingReliable
:
boxSizingReliable
gets set to false
on ready
based on a call to getComputedStyle
to check whether it's giving the wrong values. The only browser I have handy where boxSizingReliable
is false is Firefox (I have v3.6.15 and v20).
This is more problematic, because the div
we check needs to be in the document for us to use getComputedStyle
on it. I did have an idea, but I'm not that comfortable with it. [ That could be because I'm a bit outside my comfort zone when we get to browser edge cases. :-) ] The div
needs to be in the document, but apparently it doesn't have to be in body
. On the browsers I've checked, before ready
I can append it to documentElement
and do the test successfully, getting the expected values on Firefox (false) and Chrome/Safari/Opera (true) (the test is irrelevant to IE at present). This doesn't (in my tests) trigger creation of the body
element like it would if the parser hit a <div>
tag in the source. In fact, if I don't remove it, it remains a direct child of the html
element even after page load is complete (even on IE! but of course we wouldn't need to do this on IE until/unless it supports getComputedStyle
).
I don't see us appending elements to the documentElement
(temporarily) anywhere else in the source, but provided it doesn't have unpleasant side-effects in browsers I haven't tested, it might be a viable option. But again, when it comes to the vagaries of potential cross-browser issues when appending to the document element (and temporarily creating an invalid structure) prior to body
being created, I get out of my depth.
Thoughts?
Thanks - T.J. :-)
(* Browsers tested so far: IE6, IE8, IE10, Chrome 26 [Linux], Firefox 3.6.15 [Windows], Firefox 20 [Linux], Safari [iOS 6], Opera 12.15 [Linux].)
comment:19 Changed 10 years ago by
Follow-up thought: The use-case we're trying to support is people like me who put scripts at the end of body
, right? So pretty much my entire post just now was a bit silly.
The simpler solution is to test if body
exists (prior to the ready
callback) and if so, do the current boxSizing
/ boxSizingReliable
tests right then; if not, do the current tests on ready
. That supports:
- Users putting
script
tags inhead
and usingready
(oronload
)
- Users putting
script
tags inbody
and running them immediately (or usingready
oronload
)
It would not support users putting the script
tag for jQuery in the head
and then putting their own scripts in the body
and not waiting for ready
. I would think "don't do that" would be an acceptable answer to that scenario. :-) (If we really wanted to support that, we could trigger the tests on the first user-generated call into jQuery, but to me at that point we're going too far.)
To me, this is much less risky than appending the div
to documentElement
. It's actually a very small change.
comment:20 Changed 10 years ago by
Actually, we do already append to documentElement
during initialization: https://github.com/jquery/sizzle/blob/70f333eb1f62a779bffa748543b36093e8832bd0/sizzle.js#L509
I personally am all for taking advantage of the technique in jQuery support testing as well.
comment:21 Changed 10 years ago by
@dgibson: Doh! That's what I get for filtering out Sizzle results in my search. :-)
That's fantastic, it means it's tried and true. (And very useful to know...)
So we have two options:
- Use the "
in style
" test forboxSizing
(the current check doesn't work earlier in IE, not even when thediv
is appended todocumentElement
) and move theboxSizingReliable
test (my long comment earlier today), or
- Detect whether
body
is already present and do the existing tests, deferring them until theready
handler ifbody
isn't present (my shorter follow-up).
I don't think either is bigger in terms of code than the other to a degree that matters.
I prefer Option 1 unless someone has a known invalid result on the in
test, because it handles use-cases Option 2 doesn't handle and doesn't vary at runtime depending on where the script
tag is put. But I'm very new here and defer to those who aren't.
Once we have a consensus, I'll put together the patch (with tests, of course) and send a pull req.
-- T.J.
comment:22 Changed 10 years ago by
Option 1 sounds good. I recall that oldIE leaks memory when the in
operator is used with host objects like window
or document
, dunno if that could be an issue here.
comment:23 Changed 10 years ago by
I also prefer option 1, and would further note (though not necessarily recommend as in-scope for this fix) that documentElement.append( container )
can probably cover all presently-deferred support tests.
comment:24 Changed 10 years ago by
Good deal, I'm on it. Just for boxSizing
for now, but I'll look at the other deferred tests as a follow-on effort to see if we can move them up.
comment:25 Changed 10 years ago by
T.J. Crowder, how is your work going? I can pick up this ticket if you didn't find time.
comment:26 Changed 10 years ago by
@m_gol: I really wanted to be the one to fix this, but my plate is clearly overflowing (and running onto the floor, frankly). If you have the bandwidth in the next week, go for it. :-) If not, I'll see if I can pick it up on Sunday.
comment:28 Changed 10 years ago by
Owner: | set to m_gol |
---|---|
Status: | open → assigned |
I'm taking it. I'll do it as part of my refactoring the support module (i.e. breaking it to tests performed in separate modules).
comment:29 Changed 10 years ago by
Milestone: | None → 1.11/2.1 |
---|
comment:30 Changed 10 years ago by
boxSizingReliable is irrelevant as of FF23. According to our support statement, it can be removed when FF24 comes out. Also, the case in which its hit is quite rare (and involves a sized but disconnected element). I have a PR to remove it once FF24 lands.
comment:31 Changed 10 years ago by
BTW, setting boxSizing by default to true is something we should do as m_gol stated, even for 1.x. True is the correct assumption for like 95% of pageloads on the net.
comment:32 Changed 10 years ago by
@mikesherov boxSizingReliable
is needed for IE10 and IE11 as well, I'm afraid. :(
comment:33 Changed 10 years ago by
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
Fix #14084: attach the test div to documentElement, not body.
Changeset: 776012b8b3898fad2e0727880f1dc4af5c7b33c1
I can't confirm it on jsFiddle, weird:
1.x edge: http://jsfiddle.net/m_gol/kDVyv/
1.8.3: http://jsfiddle.net/m_gol/kDVyv/2/