Dimensions: fall back to offsetWidth/Height for inline elems #3577
Conversation
|
@timmywil, thanks for your PR! By analyzing the history of the files in this pull request, we identified @markelog, @jeresig and @brandonaaron to be potential reviewers. |
|
Will land today. Get your reviews in! Thanks. |
src/css.js
Outdated
| // Fall back to offsetWidth/Height when value is "auto" | ||
| // This happens for inline elements with no explicit setting (gh-3571) | ||
| if ( val === "auto" ) { | ||
| val = elem[ "offset" + name.toUpperCase().slice( 0, 1 ) + name.slice( 1 ) ]; |
There was a problem hiding this comment.
Why not name[0].toUpperCase()? It should be shorter.
There was a problem hiding this comment.
Sure! I'm used to this for the sake of old browsers.
| @@ -31,6 +30,7 @@ function curCSS( elem, name, computed ) { | |||
| // This is against the CSSOM draft spec: | |||
| // https://drafts.csswg.org/cssom/#resolved-values | |||
| if ( !support.pixelMarginRight() && rnumnonpx.test( ret ) && rmargin.test( name ) ) { | |||
| style = elem.style; | |||
There was a problem hiding this comment.
What's the reason for this change? Isn't it larger?
There was a problem hiding this comment.
No point in retrieving style before this block. I'd like to avoid accessing properties that we don't need.
test/unit/dimensions.js
Outdated
| assert.equal( $div.width(), 0, "Test border specified with pixels" ); | ||
| testElement( $div ); | ||
| // Test span/inline element | ||
| testElement( jQuery( "#display" ) ); |
There was a problem hiding this comment.
This is risky; why not create a span here instead of relying on a global fixture?
There was a problem hiding this comment.
It already relies on a global fixture for the div. I'm not sure the risk. If it gets removed, the test will fail.
There was a problem hiding this comment.
If it gets removed the test will fail but if it got changed from a span to a div, it wouldn't, would it?
There was a problem hiding this comment.
If you don't want to use a span defined in place, how about at least adding an assertion that its computed display is inline? That would eliminate the risk.
There was a problem hiding this comment.
Ok, you convinced me. I will create the span inline.
| // Fall back to offsetWidth/Height when value is "auto" | ||
| // This happens for inline elements with no explicit setting (gh-3571) | ||
| if ( val === "auto" ) { | ||
| val = elem[ "offset" + name[ 0 ].toUpperCase() + name.slice( 1 ) ]; |
There was a problem hiding this comment.
elem[ jQuery.camelCase( "offset-" + name ) ] is probably smaller, but also probably not a big deal.
Fixes gh-3571
Checklist
[x] If needed, a docs issue/PR was created at https://github.com/jquery/api.jquery.com