Skip to content
Permalink
Browse files
Remove conditional that prevents attr from working on non-Element nod…
…es. Fixes #7451.
  • Loading branch information
csnover authored and jeresig committed Nov 9, 2010
1 parent d1a88b2 commit a64dc0405064d68c7b7cd0f0fc8ea60086cbcd21
Showing 2 changed files with 66 additions and 67 deletions.
@@ -289,91 +289,88 @@ jQuery.extend({
// Try to normalize/fix the name
name = notxml && jQuery.props[ name ] || name;

// Only do all the following if this is a node (faster for style)
if ( elem.nodeType === 1 ) {
// These attributes require special treatment
var special = rspecialurl.test( name );

// Safari mis-reports the default selected property of an option
// Accessing the parent's selectedIndex property fixes it
if ( name === "selected" && !jQuery.support.optSelected ) {
var parent = elem.parentNode;
if ( parent ) {
parent.selectedIndex;

// Make sure that it also works with optgroups, see #5701
if ( parent.parentNode ) {
parent.parentNode.selectedIndex;
}
// These attributes require special treatment
var special = rspecialurl.test( name );

// Safari mis-reports the default selected property of an option
// Accessing the parent's selectedIndex property fixes it
if ( name === "selected" && !jQuery.support.optSelected ) {
var parent = elem.parentNode;
if ( parent ) {
parent.selectedIndex;

// Make sure that it also works with optgroups, see #5701
if ( parent.parentNode ) {
parent.parentNode.selectedIndex;
}
}
}

// If applicable, access the attribute via the DOM 0 way
// 'in' checks fail in Blackberry 4.7 #6931
if ( (name in elem || elem[ name ] !== undefined) && notxml && !special ) {
if ( set ) {
// We can't allow the type property to be changed (since it causes problems in IE)
if ( name === "type" && rtype.test( elem.nodeName ) && elem.parentNode ) {
jQuery.error( "type property can't be changed" );
}

if ( value === null ) {
if ( elem.nodeType === 1 ) {
elem.removeAttribute( name );
}

} else {
elem[ name ] = value;
}
}

// browsers index elements by id/name on forms, give priority to attributes.
if ( jQuery.nodeName( elem, "form" ) && elem.getAttributeNode(name) ) {
return elem.getAttributeNode( name ).nodeValue;
// If applicable, access the attribute via the DOM 0 way
// 'in' checks fail in Blackberry 4.7 #6931
if ( (name in elem || elem[ name ] !== undefined) && notxml && !special ) {
if ( set ) {
// We can't allow the type property to be changed (since it causes problems in IE)
if ( name === "type" && rtype.test( elem.nodeName ) && elem.parentNode ) {
jQuery.error( "type property can't be changed" );
}

// elem.tabIndex doesn't always return the correct value when it hasn't been explicitly set
// http://fluidproject.org/blog/2008/01/09/getting-setting-and-removing-tabindex-values-with-javascript/
if ( name === "tabIndex" ) {
var attributeNode = elem.getAttributeNode( "tabIndex" );
if ( value === null ) {
if ( elem.nodeType === 1 ) {
elem.removeAttribute( name );
}

return attributeNode && attributeNode.specified ?
attributeNode.value :
rfocusable.test( elem.nodeName ) || rclickable.test( elem.nodeName ) && elem.href ?
0 :
undefined;
} else {
elem[ name ] = value;
}
}

return elem[ name ];
// browsers index elements by id/name on forms, give priority to attributes.
if ( jQuery.nodeName( elem, "form" ) && elem.getAttributeNode(name) ) {
return elem.getAttributeNode( name ).nodeValue;
}

if ( !jQuery.support.style && notxml && name === "style" ) {
if ( set ) {
elem.style.cssText = "" + value;
}
// elem.tabIndex doesn't always return the correct value when it hasn't been explicitly set
// http://fluidproject.org/blog/2008/01/09/getting-setting-and-removing-tabindex-values-with-javascript/
if ( name === "tabIndex" ) {
var attributeNode = elem.getAttributeNode( "tabIndex" );

return elem.style.cssText;
return attributeNode && attributeNode.specified ?
attributeNode.value :
rfocusable.test( elem.nodeName ) || rclickable.test( elem.nodeName ) && elem.href ?
0 :
undefined;
}

return elem[ name ];
}

if ( !jQuery.support.style && notxml && name === "style" ) {
if ( set ) {
// convert the value to a string (all browsers do this but IE) see #1070
elem.setAttribute( name, "" + value );
elem.style.cssText = "" + value;
}

// Ensure that missing attributes return undefined
// Blackberry 4.7 returns "" from getAttribute #6938
if ( !elem.attributes[ name ] && (elem.hasAttribute && !elem.hasAttribute( name )) ) {
return undefined;
}
return elem.style.cssText;
}

var attr = !jQuery.support.hrefNormalized && notxml && special ?
// Some attributes require a special call on IE
elem.getAttribute( name, 2 ) :
elem.getAttribute( name );
if ( set ) {
// convert the value to a string (all browsers do this but IE) see #1070
elem.setAttribute( name, "" + value );
}

// Non-existent attributes return null, we normalize to undefined
return attr === null ? undefined : attr;
// Ensure that missing attributes return undefined
// Blackberry 4.7 returns "" from getAttribute #6938
if ( !elem.attributes[ name ] && (elem.hasAttribute && !elem.hasAttribute( name )) ) {
return undefined;
}

var attr = !jQuery.support.hrefNormalized && notxml && special ?
// Some attributes require a special call on IE
elem.getAttribute( name, 2 ) :
elem.getAttribute( name );

// Non-existent attributes return null, we normalize to undefined
return attr === null ? undefined : attr;
}
});

@@ -4,7 +4,7 @@ var bareObj = function(value) { return value; };
var functionReturningObj = function(value) { return (function() { return value; }); };

test("attr(String)", function() {
expect(30);
expect(31);

// This one sometimes fails randomly ?!
equals( jQuery('#text1').attr('value'), "Test", 'Check for value attribute' );
@@ -65,6 +65,8 @@ test("attr(String)", function() {

ok( jQuery("<div/>").attr("doesntexist") === undefined, "Make sure undefined is returned when no attribute is found." );
ok( jQuery().attr("doesntexist") === undefined, "Make sure undefined is returned when no element is there." );

equals( jQuery(document).attr("nodeName"), "#document", "attr works correctly on document nodes (bug #7451)." );
});

if ( !isLocal ) {

4 comments on commit a64dc04

@jitter
Copy link
Contributor

@jitter jitter commented on a64dc04 Nov 10, 2010

Choose a reason for hiding this comment

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

I think this change goes too far just to fix #7451. Now attr() even works on non DOM nodes. Is that intentional?

If not pull request 92. test case which shows the problematic behavior.

Or does jQuery support plain javascript objects as collection elements (in several places there are checks for nodeName and nodeType which indicate to me it doesn't)

@csnover
Copy link
Member Author

@csnover csnover commented on a64dc04 Nov 10, 2010

Choose a reason for hiding this comment

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

It was doing it before already in 1.4.2 on everything except for text and comment nodes, unintentionally, through a call to jQuery.style. We can add back in a check to not run it on text/comment nodes but I am not sure if it makes any real sense to do that.

@jitter
Copy link
Contributor

@jitter jitter commented on a64dc04 Nov 10, 2010

Choose a reason for hiding this comment

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

I know this always unintentionally worked before 1.4.3. The check for text/comment nodes is still there, attr() doesn't set/get for those.

I think attr() not supporting javascript objects would prevent people from adopting bad coding patterns, like using attr() to get/set attributes on objects. Better obj.attrname then $(obj).attr("attrname"). If we don't care if it does/doesn't work with javascript objects it doesn't hurt to leave the DOM node check out.

What I forgot to add to my comment above, and the test case, is that now attr() throws an exception when retrieving non existing attributes from a javascript object. updated test case showing the exception. Line in attribute.js where the exception happens.

@jitter
Copy link
Contributor

@jitter jitter commented on a64dc04 Nov 13, 2010

Choose a reason for hiding this comment

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

Please check #7500 I updated the pull request accordingly.

Please sign in to comment.