Skip to content
Permalink
Browse files
Restrict the attr quick setters to only methods that specifically ask…
… for the functionality. Fixes #5612.
  • Loading branch information
jeresig committed Dec 10, 2009
1 parent 1a4d190 commit f25eedf
Show file tree
Hide file tree
Showing 2 changed files with 20 additions and 1 deletion.
@@ -200,12 +200,27 @@ jQuery.each({
});

jQuery.extend({
attrFn: {
val: true,
addClass: true,
css: true,
html: true,
text: true,
append: true,
prepend: true,
data: true,
width: true,
height: true,
offset: true
},

attr: function( elem, name, value ) {
// don't set attributes on text and comment nodes
if (!elem || elem.nodeType == 3 || elem.nodeType == 8) {
return undefined;
}
if ( name in jQuery.fn && name !== "attr" ) {

if ( name in jQuery.attrFn ) {
return jQuery(elem)[name](value);
}

@@ -885,6 +885,10 @@ jQuery.each( ("blur focus load resize scroll unload click dblclick " +
jQuery.fn[ name ] = function( fn ) {
return fn ? this.bind( name, fn ) : this.trigger( name );
};

if ( jQuery.fnAttr ) {
jQuery.fnAttr[ name ] = true;
}
});

// Prevent memory leaks in IE

2 comments on commit f25eedf

@scottgonzalez
Copy link
Member

Choose a reason for hiding this comment

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

What's the reason for adding the event bindings to .attr()? There can't possibly be existing code like $(el).attr('click', fn) that's expected to bind a listener. I could maybe see supporting .attr('onclick', fn) but the current implementation seems odd to me.

@jeresig
Copy link
Member Author

Choose a reason for hiding this comment

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

It's mostly for simplifying the process of setting properties in bulk - for example:

$("<div/>").attr({
  html: "Some stuff in here.",
  "class": "foo",
  click: function(){}
});

It effectively takes a bunch of the common setter cases, not handled by attr, and integrates them into a single method.

Please sign in to comment.