Side navigation
#14084 closed bug (fixed)
Opened June 30, 2013 03:25PM UTC
Closed September 06, 2013 03:29PM UTC
elem.css('width') provides incorrect output with `box-sizing: border-box` if run before document ready
Reported by: | tj@crowdersoftware.com | 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.
Attachments (0)
Change History (33)
Changed July 01, 2013 04:37PM UTC by comment:1
Changed July 01, 2013 04:47PM UTC by comment:2
That is really strange, I played with the jsbin version and it does look broken, but the jsFiddle is what I expected.
Changed July 01, 2013 05:12PM UTC by comment:3
@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.
Changed July 01, 2013 05:15PM UTC by comment:4
Changed July 01, 2013 05:18PM UTC by comment:5
Changed July 01, 2013 05:31PM UTC by comment:6
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.
Changed July 01, 2013 05:41PM UTC by comment:7
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.
Changed July 01, 2013 06:27PM UTC by comment:8
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 |
---|
Changed July 01, 2013 06:40PM UTC by comment:9
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.
Changed July 01, 2013 06:42PM UTC by comment:10
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...
Changed July 01, 2013 06:44PM UTC by comment:11
And what do you think about assuming jQuery.support.boxSizing === true
in 2.x?
Changed July 01, 2013 07:11PM UTC by comment:12
_comment0: | 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. [http://stackoverflow.com/questions/1438883/jquery-why-use-document-ready-if-external-js-at-bottom-of-page 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 [http://www.w3.org/TR/html5/syntax.html#the-end difference], according to the spec. Any scripts using the `defer` flag are executed _before_ the ready event, but not before scripts at the end of the body. Just good to be aware of that. \ \ Finally, in oldIE, there was the [http://blogs.msdn.com/b/ie/archive/2008/04/23/what-happened-to-operation-aborted.aspx operation aborted event], but that could be avoided by ensuring the script tag's parent is actually the body. \ → 1372706747781617 |
---|
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.
Changed July 01, 2013 09:36PM UTC by comment:13
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.
Changed July 01, 2013 09:39PM UTC by comment:14
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.
Changed July 01, 2013 10:07PM UTC by comment:15
@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?
Changed July 01, 2013 10:11PM UTC by comment:16
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.
Changed July 01, 2013 10:40PM UTC by comment:17
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.
Changed July 09, 2013 09:03AM UTC by comment:18
@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].)
Changed July 09, 2013 09:14AM UTC by comment:19
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:
1. Users putting script
tags in head
and using ready
(or onload
)
2. Users putting script
tags in body
and running them immediately (or using ready
or onload
)
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.
Changed July 09, 2013 02:36PM UTC by comment:20
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.
Changed July 09, 2013 02:58PM UTC by comment:21
@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:
1. Use the "in style
" test for boxSizing
(the current check doesn't work earlier in IE, not even when the div
is appended to documentElement
) and move the boxSizingReliable
test (my long comment earlier today), or
2. Detect whether body
is already present and do the existing tests, deferring them until the ready
handler if body
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.
Changed July 09, 2013 07:25PM UTC by comment:22
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.
Changed July 09, 2013 09:00PM UTC by comment:23
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.
Changed July 09, 2013 09:16PM UTC by comment:24
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.
Changed August 19, 2013 05:56PM UTC by comment:25
T.J. Crowder, how is your work going? I can pick up this ticket if you didn't find time.
Changed August 20, 2013 09:22AM UTC by comment:26
@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.
Changed August 20, 2013 06:15PM UTC by comment:27
#14253 is a duplicate of this ticket.
Changed August 21, 2013 10:20PM UTC by comment:28
owner: | → 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).
Changed August 21, 2013 10:20PM UTC by comment:29
milestone: | None → 1.11/2.1 |
---|
Changed September 01, 2013 10:12PM UTC by comment:30
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.
Changed September 01, 2013 11:26PM UTC by comment:31
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.
Changed September 02, 2013 12:02AM UTC by comment:32
@mikesherov boxSizingReliable
is needed for IE10 and IE11 as well, I'm afraid. :(
Changed September 06, 2013 03:29PM UTC by comment:33
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/