Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
Return null for outer/inner width/height calls on window/document. Fi… 
…xes #7557.
  • Loading branch information
timmywil committed May 25, 2011
1 parent 1d1cb58 commit edb2286
Show file tree
Hide file tree
Showing 3 changed files with 37 additions and 9 deletions.
6 changes: 5 additions & 1 deletion src/css.js
View file
Expand Up @@ -170,6 +170,11 @@ jQuery.each(["height", "width"], function( i, name ) {
get: function( elem, computed, extra ) {
var val;

// Tests for window/document
if ( !elem.style ) {
return null;
}

if ( computed ) {
if ( elem.offsetWidth !== 0 ) {
val = getWH( elem, name, extra );
Expand All @@ -196,7 +201,6 @@ jQuery.each(["height", "width"], function( i, name ) {

if ( val < 0 || val == null ) {
val = elem.style[ name ];

// Should return "auto" instead of 0, use 0 for
// temporary backwards-compat
return val === "" || val === "auto" ? "0px" : val;
Expand Down
10 changes: 6 additions & 4 deletions src/dimensions.js
View file
Expand Up @@ -7,15 +7,17 @@ jQuery.each([ "Height", "Width" ], function( i, name ) {

// innerHeight and innerWidth
jQuery.fn["inner" + name] = function() {
return this[0] ?
parseFloat( jQuery.css( this[0], type, "padding" ) ) :
var ret;
return this[0] && !isNaN( ret = parseFloat(jQuery.css( this[0], type, "padding" )) ) ?
ret :
null;
};

// outerHeight and outerWidth
jQuery.fn["outer" + name] = function( margin ) {
return this[0] ?
parseFloat( jQuery.css( this[0], type, margin ? "margin" : "border" ) ) :
var ret;
return this[0] && !isNaN( ret = parseFloat(jQuery.css( this[0], type, margin ? "margin" : "border" )) ) ?
ret :
null;
};

Expand Down
30 changes: 26 additions & 4 deletions test/unit/dimensions.js
View file
Expand Up @@ -107,7 +107,13 @@ test("height() with function args", function() {
});

test("innerWidth()", function() {
expect(4);
expect(8);

equals(jQuery(window).innerWidth(), null, "Test on window without margin option");
equals(jQuery(window).innerWidth(true), null, "Test on window with margin option");

equals(jQuery(document).innerWidth(), null, "Test on document without margin option");
equals(jQuery(document).innerWidth(true), null, "Test on document with margin option");

var $div = jQuery("#nothiddendiv");
// set styles
Expand Down Expand Up @@ -136,7 +142,13 @@ test("innerWidth()", function() {
});

test("innerHeight()", function() {
expect(4);
expect(8);

equals(jQuery(window).innerHeight(), null, "Test on window without margin option");
equals(jQuery(window).innerHeight(true), null, "Test on window with margin option");

equals(jQuery(document).innerHeight(), null, "Test on document without margin option");
equals(jQuery(document).innerHeight(true), null, "Test on document with margin option");

var $div = jQuery("#nothiddendiv");
// set styles
Expand Down Expand Up @@ -165,7 +177,12 @@ test("innerHeight()", function() {
});

test("outerWidth()", function() {
expect(7);
expect(11);

equal( jQuery( window ).outerWidth(), null, "Test on window without margin option" );
equal( jQuery( window ).outerWidth( true ), null, "Test on window with margin option" );
equal( jQuery( document ).outerWidth(), null, "Test on document without margin option" );
equal( jQuery( document ).outerWidth( true ), null, "Test on document with margin option" );

var $div = jQuery("#nothiddendiv");
$div.css("width", 30);
Expand Down Expand Up @@ -195,7 +212,12 @@ test("outerWidth()", function() {
});

test("outerHeight()", function() {
expect(7);
expect(11);

equal( jQuery( window ).outerHeight(), null, "Test on window without margin option" );
equal( jQuery( window ).outerHeight( true ), null, "Test on window with margin option" );
equal( jQuery( document ).outerHeight(), null, "Test on document without margin option" );
equal( jQuery( document ).outerHeight( true ), null, "Test on document with margin option" );

var $div = jQuery("#nothiddendiv");
$div.css("height", 30);
Expand Down

3 comments on commit edb2286

@scottgonzalez
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we can detect window and document as easily as elem.style, why not just return .height()/.width() when called on window or document? If there's no performance penalty for handling them, I think the few extra bytes would be worth it.

@timmywil
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose we could, but that might be misleading since the outer/inner functions don't make sense on the window/document. I'm not sure we want to encourage their usage since in the past they have either returned NaN(1.0-1.3.2) or thrown an exception(1.4+). We now return null as snover once suggested so that at least the return values are consistent.
Also, see next commit. This logic was moved back to dimensions as width/height handle the window and document there as well.

@scottgonzalez
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They may not technically make sense at a very low level, but they certainly make sense at a high level. Ignoring the fact that you can't put a border on the window, the conceptual outer width of the window still exists, it just happens to always be equal to its width.

The only reason I could see us arguing against adding support for this is that it would degrade performance on regular element. But I don't think that a case can be made that a single property check would be problematic.

Please sign in to comment.