Use .prop instead of .attr where appropriate
authorKrinkle <krinkle@users.mediawiki.org>
Fri, 12 Aug 2011 21:19:45 +0000 (21:19 +0000)
committerKrinkle <krinkle@users.mediawiki.org>
Fri, 12 Aug 2011 21:19:45 +0000 (21:19 +0000)
* Although jQuery covers for us in most cases (since .prop didn't exist before jQuery 1.6 and many people abused .attr in laziness of doing their own loop and setting the property manually). It's better to know what we're doing and call the function we intend to call. Both because jQuery may decide to stop rerouting common mistakes to .prop and because it makes code more readable.

* Aside from switching to prop, for boolean properties also replacing null/undefined with false and 'propname' with true. This is no longer being covered by jQuery when using prop directly (as it shouldn't). When an element is created the HTML specification says that the attribute should be set to it's name (ie. '<foo selected="selected">'), the properties however must remain boolean.

* Changing the attribute value for a boolean property (ie. checkbox.setAttribute( 'checked', 'checked' ) does not make the checkbox enabled. All it does is set the attribute. The reason this works with jQuery's attr() is because jQuery calls .prop internally after a bunch of checking inside attr().

-- Reference --
The list of keys that .attr and .removeAttr jQuery is currently (as of jQuery 1.6.1) remapping to use .prop and .removeProp can be found here:
https://github.com/jquery/jquery/blob/b22c9046529852c7ce567df13397849e11e2b9cc/src/attributes.js#L425
{
tabindex: "tabIndex",
readonly: "readOnly",
"for": "htmlFor",
"class": "className",
maxlength: "maxLength",
cellspacing: "cellSpacing",
cellpadding: "cellPadding",
rowspan: "rowSpan",
colspan: "colSpan",
usemap: "useMap",
frameborder: "frameBorder",
contenteditable: "contentEditable"
}
In addition to those, jQuery also maps these boolean properties to .prop when they are passed to .attr:
rboolean = /^(?:autofocus|autoplay|async|checked|controls|defer|disabled|hidden|loop|multiple|open|readonly|required|scoped|selected)$/i,

(source: https://github.com/jquery/jquery/blob/b22c9046529852c7ce567df13397849e11e2b9cc/src/attributes.js#L9 )

12 files changed:
resources/jquery/jquery.byteLimit.js
resources/jquery/jquery.checkboxShiftClick.js
resources/jquery/jquery.tabIndex.js
resources/mediawiki.action/mediawiki.action.watch.ajax.js
resources/mediawiki.special/mediawiki.special.recentchanges.js
resources/mediawiki.special/mediawiki.special.upload.js
resources/mediawiki/mediawiki.util.js
skins/common/config.js
skins/common/preview.js
tests/qunit/suites/resources/jquery/jquery.byteLimit.test.js
tests/qunit/suites/resources/jquery/jquery.tablesorter.test.js
tests/qunit/suites/resources/mediawiki.special/mediawiki.special.recentchanges.test.js

index 3f18ab1..64ccb5c 100644 (file)
@@ -31,7 +31,7 @@
 
                // Default limit to current attribute value
                if ( limit === undefined ) {
-                       limit = this.attr( 'maxLength' );
+                       limit = this.prop( 'maxLength' );
                }
 
                // Update/set attribute value, but only if there is no callback set.
                // Usually this isn't a problem since browsers ignore maxLength when setting
                // the value property through JavaScript, but Safari 4 violates that rule, so
                // we have to remove or not set the property if we have a callback.
-               if ( fn === undefined ) {
-                       this.attr( 'maxLength', limit );
+               if ( fn == undefined ) {
+                       this.prop( 'maxLength', limit );
                } else {
-                       this.removeAttr( 'maxLength' );
+                       this.removeProp( 'maxLength' );
                }
 
                // Nothing passed and/or empty attribute, return without binding an event.
index cfa696d..7fbc16b 100644 (file)
@@ -18,7 +18,7 @@ $.fn.checkboxShiftClick = function( text ) {
                        $box.slice(
                          Math.min( $box.index( prevCheckbox ), $box.index( e.target ) ),
                          Math.max( $box.index( prevCheckbox ), $box.index( e.target ) ) + 1
-                       ).attr( {checked: e.target.checked ? 'checked' : ''} );
+                       ).prop( {checked: e.target.checked ? true : false} );
                }
                // Either way, update the prevCheckbox variable to the one clicked now
                prevCheckbox = e.target;
index 33e0604..062ff7d 100644 (file)
@@ -10,7 +10,7 @@
 $.fn.firstTabIndex = function() {
        var minTabIndex = null;
        $(this).find( '[tabindex]' ).each( function() {
-               var tabIndex = parseInt( $(this).attr( 'tabindex' ), 10 );
+               var tabIndex = parseInt( $(this).prop( 'tabindex' ), 10 );
                // In IE6/IE7 the above jQuery selector returns all elements,
                // becuase it has a default value for tabIndex in IE6/IE7 of 0
                // (rather than null/undefined). Therefore check "> 0" as well.
@@ -35,7 +35,7 @@ $.fn.firstTabIndex = function() {
 $.fn.lastTabIndex = function() {
        var maxTabIndex = null;
        $(this).find( '[tabindex]' ).each( function() {
-               var tabIndex = parseInt( $(this).attr( 'tabindex' ), 10 );
+               var tabIndex = parseInt( $(this).prop( 'tabindex' ), 10 );
                if ( tabIndex > 0 && !isNaN( tabIndex ) ) {
                        // Initial value
                        if ( maxTabIndex === null ) {
index 93aa29c..3986d80 100644 (file)
@@ -75,9 +75,9 @@ var processResult = function( response, $link ) {
        // Bug 12395 - update the watch checkbox on edit pages when the
        // page is watched or unwatched via the tab.
        if ( watchResponse.watched !== undefined ) {
-               $( '#wpWatchthis' ).attr( 'checked', 'checked' );
+               $( '#wpWatchthis' ).prop( 'checked', 'checked' );
        } else {
-               $( '#wpWatchthis' ).removeAttr( 'checked' );
+               $( '#wpWatchthis' ).removeProp( 'checked' );
        }
        return true;
 };
index 7e284fb..ed7ed37 100644 (file)
@@ -20,7 +20,7 @@
 
                        // Iterates over checkboxes and propagate the selected option
                        $.each( checkboxes, function( i, id ) {
-                               $( '#' + id ).attr( 'disabled', isAllNS );
+                               $( '#' + id ).prop( 'disabled', isAllNS );
                        });
                },
 
index 51f6bd8..f9d354a 100644 (file)
@@ -252,9 +252,9 @@ jQuery( function ( $ ) {
                                $( '.mw-upload-source-error' ).remove();
                                if ( this.checked ) {
                                        // Disable all inputs
-                                       $( 'input[name!="wpSourceType"]', rows ).attr( 'disabled', true );
+                                       $( 'input[name!="wpSourceType"]', rows ).prop( 'disabled', 'disabled' );
                                        // Re-enable the current one
-                                       $( 'input', currentRow ).attr( 'disabled', false );
+                                       $( 'input', currentRow ).prop( 'disabled', false );
                                }
                        };
                }() );
index e6bb9fb..d95cb8e 100644 (file)
                                }
 
                                if ( className ) {
-                                       $messageDiv.attr( 'class', 'mw-js-message-' + className );
+                                       $messageDiv.prop( 'class', 'mw-js-message-' + className );
                                }
 
                                if ( typeof message === 'object' ) {
index e5af7d1..23f7302 100644 (file)
@@ -67,9 +67,9 @@
                $( '.enableForOther' ).click( function() {
                        var $textbox = $( '#' + $(this).attr( 'rel' ) );
                        if ( $(this).val() == 'other' ) { // FIXME: Ugh, this is ugly
-                               $textbox.removeAttr( 'readonly' ).closest( '.config-block' ).slideDown( 'fast' );
+                               $textbox.removeProp( 'readonly' ).closest( '.config-block' ).slideDown( 'fast' );
                        } else {
-                               $textbox.attr( 'readonly', 'readonly' ).closest( '.config-block' ).slideUp( 'fast' );
+                               $textbox.prop( 'readonly', true ).closest( '.config-block' ).slideUp( 'fast' );
                        }
                } );
                
index 82b27bc..9e76ca0 100644 (file)
@@ -37,8 +37,8 @@
                                        //  with the content of the loaded page
                                        var copyContent = page.find( copyElements[i] ).contents();
                                        $(copyElements[i]).empty().append( copyContent );
-                                       var newClasses = page.find( copyElements[i] ).attr('class');
-                                       $(copyElements[i]).attr( 'class', newClasses );
+                                       var newClasses = page.find( copyElements[i] ).prop('class');
+                                       $(copyElements[i]).prop( 'class', newClasses );
                                }
 
                                $.each( copyElements, function(k,v) {
index 70343fd..704740d 100644 (file)
@@ -93,10 +93,8 @@ byteLimitTest({
 byteLimitTest({
        description: 'Limit using the maxlength attribute',
        $input: $( '<input>' )
-               .attr( {
-                       'type': 'text',
-                       'maxlength': '10'
-               })
+               .attr( 'type', 'text' )
+               .prop( 'maxLength', '10' )
                .byteLimit(),
        sample: simpleSample,
        hasLimit: true,
@@ -120,10 +118,8 @@ byteLimitTest({
 byteLimitTest({
        description: 'Limit using a custom value, overriding maxlength attribute',
        $input: $( '<input>' )
-               .attr( {
-                       'type': 'text',
-                       'maxLength': '10'
-               })
+               .attr( 'type', 'text' )
+               .prop( 'maxLength', '10' )
                .byteLimit( 15 ),
        sample: simpleSample,
        hasLimit: true,
@@ -183,10 +179,8 @@ byteLimitTest({
 byteLimitTest({
        description: 'Limit using the maxlength attribute and pass a callback as input filter',
        $input: $( '<input>' )
-               .attr( {
-                       'type': 'text',
-                       'maxLength': '6'
-               })
+               .attr( 'type', 'text' )
+               .prop( 'maxLength', '6' )
                .byteLimit( function( val ) {
                        _titleConfig();
 
index 2efd6cd..556cb10 100644 (file)
@@ -304,7 +304,7 @@ tableTest(
        function( $table ) {
                //Quick&Dirty mod
                $table.find('tr:eq(3) td:eq(1), tr:eq(4) td:eq(1)').remove();
-               $table.find('tr:eq(2) td:eq(1)').attr('rowspan', '3');
+               $table.find('tr:eq(2) td:eq(1)').prop('rowspan', '3');
                $table.tablesorter();
                $table.find('.headerSort:eq(0)').click();
        }
@@ -317,7 +317,7 @@ tableTest(
        function( $table ) {
                //Quick&Dirty mod
                $table.find('tr:eq(3) td:eq(0), tr:eq(4) td:eq(0)').remove();
-               $table.find('tr:eq(2) td:eq(0)').attr('rowspan', '3');
+               $table.find('tr:eq(2) td:eq(0)').prop('rowspan', '3');
                $table.tablesorter();
                $table.find('.headerSort:eq(0)').click();
        }
index fca2a80..fee2223 100644 (file)
@@ -37,34 +37,34 @@ test( '"all" namespace disable checkboxes', function() {
        // TODO abstract the double strictEquals
 
        // At first checkboxes are enabled
-       strictEqual( $( '#nsinvert' ).attr( 'disabled' ), undefined );
-       strictEqual( $( '#nsassociated' ).attr( 'disabled' ), undefined );
+       strictEqual( $( '#nsinvert' ).prop( 'disabled' ), false );
+       strictEqual( $( '#nsassociated' ).prop( 'disabled' ), false );
 
        // Initiate the recentchanges module
        mw.special.recentchanges.init();
 
        // By default
-       strictEqual( $( '#nsinvert' ).attr( 'disabled' ), 'disabled' );
-       strictEqual( $( '#nsassociated' ).attr( 'disabled' ), 'disabled' );
+       strictEqual( $( '#nsinvert' ).prop( 'disabled' ), true );
+       strictEqual( $( '#nsassociated' ).prop( 'disabled' ), true );
 
        // select second option...
        var $options = $( '#namespace' ).find( 'option' );
-       $options.eq(0).removeAttr( 'selected' );
-       $options.eq(1).attr( 'selected', 'selected' );
+       $options.eq(0).removeProp( 'selected' );
+       $options.eq(1).prop( 'selected', true );
        $( '#namespace' ).change();
 
        // ... and checkboxes should be enabled again
-       strictEqual( $( '#nsinvert' ).attr( 'disabled' ), undefined );
-       strictEqual( $( '#nsassociated' ).attr( 'disabled' ), undefined );
+       strictEqual( $( '#nsinvert' ).prop( 'disabled' ), false );
+       strictEqual( $( '#nsassociated' ).prop( 'disabled' ), false );
 
        // select first option ( 'all' namespace)...
-       $options.eq(1).removeAttr( 'selected' );
-       $options.eq(0).attr( 'selected', 'selected' );;
+       $options.eq(1).removeProp( 'selected' );
+       $options.eq(0).prop( 'selected', true );
        $( '#namespace' ).change();
        
        // ... and checkboxes should now be disabled
-       strictEqual( $( '#nsinvert' ).attr( 'disabled' ), 'disabled' );
-       strictEqual( $( '#nsassociated' ).attr( 'disabled' ), 'disabled' );
+       strictEqual( $( '#nsinvert' ).prop( 'disabled' ), true );
+       strictEqual( $( '#nsassociated' ).prop( 'disabled' ), true );
 
        // DOM cleanup
        $env.remove();