Skip to content

Commit

Permalink
Fix #12840: remove undocumented parameter "pass" from .attr. Close gh…
Browse files Browse the repository at this point in the history 
…-1017.
  • Loading branch information
@gibson042
gibson042 committed Nov 5, 2012
1 parent 53cb49c commit 80d45a6
Show file tree
Hide file tree
Showing 3 changed files with 33 additions and 105 deletions.
11 changes: 3 additions & 8 deletions src/attributes.js
View file
Expand Up @@ -280,7 +280,7 @@ jQuery.extend({
}
},

attr: function( elem, name, value, pass ) {
attr: function( elem, name, value ) {
var ret, hooks, notxml,
nType = elem.nodeType;

Expand All @@ -289,10 +289,6 @@ jQuery.extend({
return;
}

if ( pass && jQuery.isFunction( jQuery.fn[ name ] ) ) {
return jQuery( elem )[ name ]( value );
}

// Fallback to prop when attributes are not supported
if ( typeof elem.getAttribute === "undefined" ) {
return jQuery.prop( elem, name, value );
Expand All @@ -311,17 +307,16 @@ jQuery.extend({

if ( value === null ) {
jQuery.removeAttr( elem, name );
return;

} else if ( hooks && "set" in hooks && notxml && (ret = hooks.set( elem, value, name )) !== undefined ) {
} else if ( hooks && notxml && "set" in hooks && (ret = hooks.set( elem, value, name )) !== undefined ) {
return ret;

} else {
elem.setAttribute( name, value + "" );
return value;
}

} else if ( hooks && "get" in hooks && notxml && (ret = hooks.get( elem, name )) !== null ) {
} else if ( hooks && notxml && "get" in hooks && (ret = hooks.get( elem, name )) !== null ) {
return ret;

} else {
Expand Down
49 changes: 30 additions & 19 deletions src/core.js
View file
Expand Up @@ -84,14 +84,14 @@ var
jQuery.fn = jQuery.prototype = {
constructor: jQuery,
init: function( selector, context, rootjQuery ) {
var match, elem, doc;
var match, elem;

// Handle $(""), $(null), $(undefined), $(false)
// HANDLE: $(""), $(null), $(undefined), $(false)
if ( !selector ) {
return this;
}

// Handle $(DOMElement)
// HANDLE: $(DOMElement)
if ( selector.nodeType ) {
this.context = this[0] = selector;
this.length = 1;
Expand All @@ -114,15 +114,29 @@ jQuery.fn = jQuery.prototype = {
// HANDLE: $(html) -> $(array)
if ( match[1] ) {
context = context instanceof jQuery ? context[0] : context;
doc = ( context && context.nodeType ? context.ownerDocument || context : document );

// scripts is true for back-compat
selector = jQuery.parseHTML( match[1], doc, true );
jQuery.merge( this, jQuery.parseHTML(
match[1],
context && context.nodeType ? context.ownerDocument || context : document,
true
) );

// HANDLE: $(html, props)
if ( rsingleTag.test( match[1] ) && jQuery.isPlainObject( context ) ) {
this.attr.call( selector, context, true );
for ( match in context ) {
// Properties of context are called as methods if possible
if ( jQuery.isFunction( this[ match ] ) ) {
this[ match ]( context[ match ] );

// ...and otherwise set as attributes
} else {
this.attr( match, context[ match ] );
}
}
}

return jQuery.merge( this, selector );
return this;

// HANDLE: $(#id)
} else {
Expand Down Expand Up @@ -768,23 +782,22 @@ jQuery.extend({

// Multifunctional method to get and set values of a collection
// The value/s can optionally be executed if it's a function
access: function( elems, fn, key, value, chainable, emptyGet, pass ) {
var exec,
access: function( elems, fn, key, value, chainable, emptyGet ) {

This comment has been minimized.

Sign in to view

Copy link
@scottgonzalez

scottgonzalez Nov 6, 2012

Member

There are still calls to jQuery.access() which pass 7 parameters. For example, .data() uses all 7 params and sets pass to false. We used to determine if function values should be executed based on pass, but now it's based purely on value. This breaks .data( "foo", function() {} ).
var i = 0,
length = elems.length,
bulk = key == null,
i = 0,
length = elems.length;
exec = value !== undefined && jQuery.isFunction( value );

// Sets many values
if ( key && typeof key === "object" ) {
chainable = true;
for ( i in key ) {
jQuery.access( elems, fn, i, key[i], 1, emptyGet, value );
jQuery.access( elems, fn, i, key[i], true, emptyGet );
}
chainable = 1;

// Sets one value
} else if ( value !== undefined ) {
// Optionally, function values get executed if exec is true
exec = pass === undefined && jQuery.isFunction( value );
chainable = true;

if ( bulk ) {
// Bulk operations only iterate when executing function values
Expand All @@ -802,12 +815,10 @@ jQuery.extend({
}

if ( fn ) {
for (; i < length; i++ ) {
fn( elems[i], key, exec ? value.call( elems[i], i, fn( elems[i], key ) ) : value, pass );
for ( ; i < length; i++ ) {
fn( elems[i], key, exec ? value.call( elems[i], i, fn( elems[i], key ) ) : value );
}
}

chainable = 1;
}

return chainable ?
Expand Down
78 changes: 0 additions & 78 deletions test/unit/attributes.js
View file
Expand Up @@ -435,84 +435,6 @@ test( "attr(String, Object)", function() {
equal( jQuery("#name").attr( "nonexisting", undefined ).attr("nonexisting"), undefined, ".attr('attribute', undefined) does not create attribute (#5571)" );
});

test( "attr(jquery_method)", function() {

var $elem = jQuery("<div />"),
elem = $elem[ 0 ],
expected = 2,
attrObj = {};

if ( jQuery.fn.width ) {
expected += 2;
attrObj["width"] = 10;
}

if ( jQuery.fn.offset ) {
expected += 2;
attrObj["offset"] = {
"top": 1,
"left": 0
};
}

if ( jQuery.css ) {
expected += 3;
attrObj["css"] = {
"paddingLeft": 1,
"paddingRight": 1
};
}

expect( expected );

// one at a time
$elem.attr({
"html": "foo"
}, true );
equal( elem.innerHTML, "foo", "attr(html)" );

$elem.attr({
"text": "bar"
}, true );
equal( elem.innerHTML, "bar", "attr(text)" );

// Multiple attributes
$elem.attr( attrObj, true );

if ( jQuery.fn.width ) {
equal( elem.style.width, "10px", "attr({width:})" );

$elem.attr( {
"height": 10
}, true );
equal( elem.style.height, "10px", "attr(height)" );
}

if ( jQuery.fn.offset ) {
equal( elem.style.top, "1px", "attr({offset:})" );

$elem.attr({
offset: {
top: 1,
left: 1
}
}, true );
equal( elem.style.left, "1px", "attr(offset)" );
}

if ( jQuery.css ) {
equal( elem.style.paddingLeft, "1px", "attr({css:})" );
equal( elem.style.paddingRight, "1px", "attr({css:})" );

$elem.attr({
"css": {
"color": "red"
}
}, true );
ok( /^(#ff0000|red)$/i.test( elem.style.color ), "attr(css)" );
}
});

test( "attr(String, Object) - Loaded via XML document", function() {
expect( 2 );
var xml = createDashboardXML();
Expand Down

3 comments on commit 80d45a6

@Krinkle
Copy link
Member

@Krinkle Krinkle commented on 80d45a6 Nov 5, 2012

Choose a reason for hiding this comment

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

Do we have tests for jQuery( .., methods );? Or did you remove the only (indirect) tests for that?

@gibson042
Copy link
Member Author

Choose a reason for hiding this comment

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

Are you referring to https://github.com/jquery/jquery/blob/master/test/unit/core.js#L104 ?

@Krinkle
Copy link
Member

@Krinkle Krinkle commented on 80d45a6 Nov 5, 2012

Choose a reason for hiding this comment

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

Yep! 😄

Please sign in to comment.