Skip to content

Loading…

Fix #12840: remove undocumented parameter "pass" from attr #1017

Closed
wants to merge 2 commits into from

2 participants

@gibson042
jQuery Foundation member

Sizes - compared to master @ 53cb49c

    266788       (+71)  dist/jquery.js                                         
     93099       (-24)  dist/jquery.min.js                                     
     33165       (-63)  dist/jquery.min.js.gz
@gibson042 gibson042 commented on the diff
test/unit/attributes.js
@@ -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() {
@gibson042 jQuery Foundation member

I have no idea why we were testing an undocumented interface.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@timmywil timmywil commented on an outdated diff
src/core.js
((17 lines not shown))
for ( i in key ) {
- jQuery.access( elems, fn, i, key[i], 1, emptyGet, value );
+ jQuery.access( elems, fn, i, key[i], true );
@timmywil jQuery Foundation member
timmywil added a note

Is the removal of passing along emptyGet intentional?

@gibson042 jQuery Foundation member

No, it wasn't; nice catch. Fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@timmywil timmywil commented on the diff
src/attributes.js
@@ -311,9 +307,8 @@ jQuery.extend({
if ( value === null ) {
jQuery.removeAttr( elem, name );
- return;
@timmywil jQuery Foundation member
timmywil added a note

It might be safer for future maintenance to keep the early return even though using notxml in two if statements may compress better.

@timmywil jQuery Foundation member
timmywil added a note

Err, I'm reading this wrong. nvm.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@timmywil timmywil commented on the diff
src/core.js
((15 lines not shown))
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 ] );
+ }
+ }
@timmywil jQuery Foundation member
timmywil added a note

:+1:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@gibson042 gibson042 closed this in 80d45a6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
Showing with 33 additions and 105 deletions.
  1. +3 −8 src/attributes.js
  2. +30 −19 src/core.js
  3. +0 −78 test/unit/attributes.js
View
11 src/attributes.js
@@ -280,7 +280,7 @@ jQuery.extend({
}
},
- attr: function( elem, name, value, pass ) {
+ attr: function( elem, name, value ) {
var ret, hooks, notxml,
nType = elem.nodeType;
@@ -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 );
@@ -311,9 +307,8 @@ jQuery.extend({
if ( value === null ) {
jQuery.removeAttr( elem, name );
- return;
@timmywil jQuery Foundation member
timmywil added a note

It might be safer for future maintenance to keep the early return even though using notxml in two if statements may compress better.

@timmywil jQuery Foundation member
timmywil added a note

Err, I'm reading this wrong. nvm.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
- } 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 {
@@ -321,7 +316,7 @@ jQuery.extend({
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 {
View
49 src/core.js
@@ -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;
@@ -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 ] );
+ }
+ }
@timmywil jQuery Foundation member
timmywil added a note

:+1:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
}
- return jQuery.merge( this, selector );
+ return this;
// HANDLE: $(#id)
} else {
@@ -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 ) {
+ 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
@@ -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 ?
View
78 test/unit/attributes.js
@@ -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() {
@gibson042 jQuery Foundation member

I have no idea why we were testing an undocumented interface.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
-
- 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();
Something went wrong with that request. Please try again.