Skip to content
Permalink
Browse files

Make sure that .width()/.height() don't return NaN also standardize o…

…n returning instead of auto for default values (which is what we do elsewhere in .css() as well). Fixes #7225.
  • Loading branch information...
jeresig committed Oct 22, 2010
1 parent 7e02cee commit 53396b879bd29c090824da182e3cf69158829f82
Showing with 26 additions and 28 deletions.
  1. +2 −6 src/css.js
  2. +22 −20 src/dimensions.js
  3. +2 −2 test/unit/css.js
@@ -169,15 +169,11 @@ jQuery.each(["height", "width"], function( i, name ) {
});
}

if ( val < 0 ) {
return elem.style[ name ] || "0px";
}

if ( val === 0 ) {
if ( val <= 0 ) {
val = curCSS( elem, name, name );

if ( val != null ) {
return val;
return val === "auto" ? "" : val;
}
}

@@ -33,27 +33,29 @@ jQuery.each([ "Height", "Width" ], function( i, name ) {
});
}

return jQuery.isWindow( elem ) ?
if ( jQuery.isWindow( elem ) ) {
// Everyone else use document.documentElement or document.body depending on Quirks vs Standards mode
elem.document.compatMode === "CSS1Compat" && elem.document.documentElement[ "client" + name ] ||
elem.document.body[ "client" + name ] :

// Get document width or height
(elem.nodeType === 9) ? // is it a document
// Either scroll[Width/Height] or offset[Width/Height], whichever is greater
Math.max(
elem.documentElement["client" + name],
elem.body["scroll" + name], elem.documentElement["scroll" + name],
elem.body["offset" + name], elem.documentElement["offset" + name]
) :

// Get or set width or height on the element
size === undefined ?
// Get width or height on the element
parseFloat( jQuery.css( elem, type ) ) :

// Set the width or height on the element (default to pixels if value is unitless)
this.css( type, typeof size === "string" ? size : size + "px" );
return elem.document.compatMode === "CSS1Compat" && elem.document.documentElement[ "client" + name ] ||
elem.document.body[ "client" + name ];

// Get document width or height
} else if ( elem.nodeType === 9 ) {
// Either scroll[Width/Height] or offset[Width/Height], whichever is greater
return Math.max(
elem.documentElement["client" + name],
elem.body["scroll" + name], elem.documentElement["scroll" + name],
elem.body["offset" + name], elem.documentElement["offset" + name]
);

// Get or set width or height on the element
} else if ( size === undefined ) {
var orig = jQuery.css( elem, type ), ret = parseFloat( orig );
return jQuery.isNaN( ret ) ? orig : ret;

// Set the width or height on the element (default to pixels if value is unitless)
} else {
return this.css( type, typeof size === "string" ? size : size + "px" );
}
};

});
@@ -13,8 +13,8 @@ test("css(String|Hash)", function() {

var div = jQuery( "<div>" );

equals( div.css("width") || "auto", "auto", "Width on disconnected node." );
equals( div.css("height") || "auto", "auto", "Height on disconnected node." );
equals( div.css("width"), "", "Width on disconnected node." );
equals( div.css("height"), "", "Height on disconnected node." );

div.css({ width: 4, height: 4 });

6 comments on commit 53396b8

@csnover

This comment has been minimized.

Copy link
Member

replied Oct 22, 2010

1.4.2:
INPUT 0x0 [auto 90px] hidden
INPUT 0x0 [13px 90px] hidden
TEXTAREA 0x0 [auto 90px] hidden
DIV 0x0 [auto 90px] hidden
SPAN 0x0 [auto 90px] hidden
DIV 0x0 [30px 90px] hidden

53396b8:
INPUT x90 [ 90px] hidden
INPUT 13x90 [13px 90px] hidden
TEXTAREA x90 [ 90px] hidden
DIV x90 [ 90px] hidden
SPAN x90 [ 90px] hidden
DIV 30x90 [30px 90px] hidden

@csnover

This comment has been minimized.

Copy link
Member

replied Oct 25, 2010

This commit is still behaving differently than 1.4.2 in ways that are likely to cause regressions. only .css should be returning width/height information (.width/.height should still say zero), and the word “auto” should not be converted to an empty string. (I think there was some discussion about IE doing it differently, but I checked, and IE says “auto” like everyone else in 1.4.2.)

@jeresig

This comment has been minimized.

Copy link
Member Author

replied Oct 25, 2010

I disagree about .width/.height only returning 0 - that was causing problems in and of itself. This case is a definite edge case - it's only when they're inside an element that is hidden. We could walk up the tree and flip all display: none elements to visible then get the dimensions but that's just untennable. We either can return 0 as a result (which is incorrect) or "auto" (which is correct, although different from the other numerical values). Returning "auto" is actually something useful and can be used. There is a difference between "no regressions" and "no changes" - having a "no changes" policy is silly when it doesn't take into account fixing bugs.

If we want to move to "auto" then we need to do it across the board (such as making sure that opacity return "auto"). I definitely hit cases where IE returned "" for computed style values (as you can see in the above test suite changes) so additional changes will have to be made there. It's definitely not consistent across browsers - but the chance that we'll break anything is very low (as client code would've been broken to begin with).

@jeresig

This comment has been minimized.

Copy link
Member Author

replied Oct 25, 2010

One thing that I really like about returning "" when the value is auto is that it makes it really easy to set later on - it doesn't matter if the input is actually something complex, setting "" will always reset the display back to the default (stylesheet-based) display.

@csnover

This comment has been minimized.

Copy link
Member

replied Oct 26, 2010

I agree 100% that the new behaviour is much better by pretty much every metric. However, the behavior of 1.4.2 was consistent all the way back to jQuery 1.2, so it is incredibly likely that someone’s code will be busted by this change—and in a release that is all about fixing regressions, I think it would be bad if we intentionally introduce a change that breaks longstanding previous behaviour.

(Either way, empty string shouldn’t be returned by width/height; it should be a number.)

@dmethvin

This comment has been minimized.

Copy link
Member

replied Oct 26, 2010

I do think we should be sensitive to regressions, I've run into several myself in the past. If we are planning to get 1.5 out in January that would seem like a good time to break things in the name of progress.

Please sign in to comment.
You can’t perform that action at this time.