Skip to content

Commit

Permalink
Reduce the boolean list only to those that have corresponding IDLs th…
Browse files Browse the repository at this point in the history
…at don't require being added to propFix; only set the IDL if it exists

- See http://jsfiddle.net/timmywil/u5NLn/ for how boolean attributes are handled in every browser.
  • Loading branch information
timmywil committed May 7, 2011
1 parent 09c0cf9 commit c085563
Show file tree
Hide file tree
Showing 2 changed files with 18 additions and 11 deletions.
27 changes: 17 additions & 10 deletions src/attributes.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ var rclass = /[\n\t\r]/g,
rtype = /^(?:button|input)$/i,
rfocusable = /^(?:button|input|object|select|textarea)$/i,
rclickable = /^a(?:rea)?$/i,
rboolean = /^(?:autofocus|autoplay|async|checked|controls|declare|defer|disabled|draggable|formnovalidate|hidden|ismap|loop|multiple|muted|noresize|noshade|nowrap|novalidate|open|pubdate|readonly|required|reversed|scoped|seamless|selected|truespeed)$/i,
rboolean = /^(?:autofocus|autoplay|async|checked|controls|defer|disabled|hidden|loop|multiple|open|readonly|required|scoped|selected)$/i,
rinvalidChar = /\:/,
formHook, boolHook;

Expand Down Expand Up @@ -311,15 +311,15 @@ jQuery.extend({
hooks = jQuery.attrHooks[ name ];

if ( !hooks ) {
// Use formHook for forms and if the name contains certain characters
if ( formHook && (jQuery.nodeName( elem, "form" ) || rinvalidChar.test( name )) ) {
hooks = formHook;

// Use boolHook for boolean attributes
} else if ( rboolean.test( name ) &&
(typeof value === "boolean" || value === undefined) ) {
if ( rboolean.test( name ) &&
(typeof value === "boolean" || value === undefined || value.toLowerCase() === name.toLowerCase()) ) {

hooks = boolHook;

// Use formHook for forms and if the name contains certain characters
} else if ( formHook && (jQuery.nodeName( elem, "form" ) || rinvalidChar.test( name )) ) {
hooks = formHook;
}
}

Expand Down Expand Up @@ -352,6 +352,7 @@ jQuery.extend({
},

removeAttr: function( elem, name ) {
var propName;
if ( elem.nodeType === 1 ) {
name = jQuery.attrFix[ name ] || name;

Expand All @@ -364,8 +365,8 @@ jQuery.extend({
}

// Set corresponding property to false for boolean attributes
if ( rboolean.test( name ) ) {
elem[ jQuery.propFix[ name ] || name ] = false;
if ( rboolean.test( name ) && (propName = jQuery.propFix[ name ] || name) in elem ) {
elem[ propName ] = false;
}
}
},
Expand Down Expand Up @@ -465,13 +466,19 @@ boolHook = {
undefined;
},
set: function( elem, value, name ) {
var propName;
if ( value === false ) {
// Remove boolean attributes when set to false
jQuery.removeAttr( elem, name );
} else {
// value is true since we know at this point it's type boolean and not false
// Set boolean attributes to the same name and set the DOM property
elem[ jQuery.propFix[ name ] || name ] = value;
propName = jQuery.propFix[ name ] || name;
if ( propName in elem ) {
// Only set the IDL specifically if it already exists on the element
elem[ propName ] = value;
}

elem.setAttribute( name, name.toLowerCase() );
}
return name;
Expand Down
2 changes: 1 addition & 1 deletion test/unit/attributes.js
Original file line number Diff line number Diff line change
Expand Up @@ -502,7 +502,7 @@ test("removeProp(String)", function() {
strictEqual( ele.nonexisting, undefined, "removeProp works correctly on non DOM element nodes (bug #7500)." );
});
jQuery.each( [commentNode, textNode, attributeNode], function( i, ele ) {
$ele = jQuery( ele );
var $ele = jQuery( ele );
$ele.prop( "nonexisting", "foo" ).removeProp( "nonexisting" );
strictEqual( ele.nonexisting, undefined, "removeProp works correctly on non DOM element nodes (bug #7500)." );
});
Expand Down

2 comments on commit c085563

@aFarkas
Copy link

@aFarkas aFarkas commented on c085563 May 8, 2011

Choose a reason for hiding this comment

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

I'm trying to understand your list. Following your approach, we should also use the following names:

  1. not supported by any browser yet
    The fact, that they always return undefined in all current browsers only means, that there is no support, but from spec, they all behave equal to other attributes in the current list:

    a) equal to reflecting boolean attributes
    - pubdate ( pubDate )
    - reversed
    - scoped
    - seamless

    b) equal to not reflecting boolean attributes (i.e.: checked).
    - muted (the idl is currently supported by modern browsers, but not the content-attribute to set the initial value. There is also a defaultMuted property)

  2. deprecated/ not often used attributes
    My list reducation proposal was heavyly influenced by only adding attributes for back-compat issues. While you try to handle all boolean content-attributes, which have a corresponding idl attribute the same. This means, that also deprecated and less known attributes can be in this list.

    a) deprecated attributes
    - autobuffer (only supported by FF3.5/3.6)
    - truespeed (trueSpeed)

    b) less known/less used attributes
    - ismap
    - declare

@timmywil
Copy link
Member

Choose a reason for hiding this comment

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

scoped has an idl in IE. The others do not yet have support in any browser. I think we can cross that bridge when they do or we might not be returning consistent values (I'd rather return empty string over undefined for now when those are simply present on the DOM with no value; in a sense, we can rely on the user more for those). As for muted, I'm not sure how to support that without extra code (the audio attribute on a video element sets the muted property for video). The deprecated attributes you mention weren't "supported" in the past and I'd rather push towards keeping them deprecated in the name of progressive enhancement (esp. since there's no backcompat issues there). isMap would require an addition to propFix, so I don't think that's worth it since as you say it is rare. Finally, declare is also deprecated.

Please sign in to comment.