From 39cb0c19d588f416fab06a195ab4b468eac85cbf Mon Sep 17 00:00:00 2001 From: Krinkle Date: Fri, 12 Aug 2011 21:19:45 +0000 Subject: [PATCH] Use .prop instead of .attr where appropriate * 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. ''), 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 ) --- resources/jquery/jquery.byteLimit.js | 8 +++---- resources/jquery/jquery.checkboxShiftClick.js | 2 +- resources/jquery/jquery.tabIndex.js | 4 ++-- .../mediawiki.action.watch.ajax.js | 4 ++-- .../mediawiki.special.recentchanges.js | 2 +- .../mediawiki.special.upload.js | 4 ++-- resources/mediawiki/mediawiki.util.js | 2 +- skins/common/config.js | 4 ++-- skins/common/preview.js | 4 ++-- .../resources/jquery/jquery.byteLimit.test.js | 18 +++++--------- .../jquery/jquery.tablesorter.test.js | 4 ++-- .../mediawiki.special.recentchanges.test.js | 24 +++++++++---------- 12 files changed, 37 insertions(+), 43 deletions(-) diff --git a/resources/jquery/jquery.byteLimit.js b/resources/jquery/jquery.byteLimit.js index 3f18ab1b08..64ccb5cc13 100644 --- a/resources/jquery/jquery.byteLimit.js +++ b/resources/jquery/jquery.byteLimit.js @@ -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. @@ -40,10 +40,10 @@ // 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. diff --git a/resources/jquery/jquery.checkboxShiftClick.js b/resources/jquery/jquery.checkboxShiftClick.js index cfa696d479..7fbc16b57a 100644 --- a/resources/jquery/jquery.checkboxShiftClick.js +++ b/resources/jquery/jquery.checkboxShiftClick.js @@ -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; diff --git a/resources/jquery/jquery.tabIndex.js b/resources/jquery/jquery.tabIndex.js index 33e0604723..062ff7d3a2 100644 --- a/resources/jquery/jquery.tabIndex.js +++ b/resources/jquery/jquery.tabIndex.js @@ -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 ) { diff --git a/resources/mediawiki.action/mediawiki.action.watch.ajax.js b/resources/mediawiki.action/mediawiki.action.watch.ajax.js index 93aa29c908..3986d80b2c 100644 --- a/resources/mediawiki.action/mediawiki.action.watch.ajax.js +++ b/resources/mediawiki.action/mediawiki.action.watch.ajax.js @@ -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; }; diff --git a/resources/mediawiki.special/mediawiki.special.recentchanges.js b/resources/mediawiki.special/mediawiki.special.recentchanges.js index 7e284fbd02..ed7ed37558 100644 --- a/resources/mediawiki.special/mediawiki.special.recentchanges.js +++ b/resources/mediawiki.special/mediawiki.special.recentchanges.js @@ -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 ); }); }, diff --git a/resources/mediawiki.special/mediawiki.special.upload.js b/resources/mediawiki.special/mediawiki.special.upload.js index 51f6bd8b71..f9d354af45 100644 --- a/resources/mediawiki.special/mediawiki.special.upload.js +++ b/resources/mediawiki.special/mediawiki.special.upload.js @@ -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 ); } }; }() ); diff --git a/resources/mediawiki/mediawiki.util.js b/resources/mediawiki/mediawiki.util.js index e6bb9fb6ff..d95cb8ef50 100644 --- a/resources/mediawiki/mediawiki.util.js +++ b/resources/mediawiki/mediawiki.util.js @@ -454,7 +454,7 @@ } if ( className ) { - $messageDiv.attr( 'class', 'mw-js-message-' + className ); + $messageDiv.prop( 'class', 'mw-js-message-' + className ); } if ( typeof message === 'object' ) { diff --git a/skins/common/config.js b/skins/common/config.js index e5af7d145b..23f7302cb8 100644 --- a/skins/common/config.js +++ b/skins/common/config.js @@ -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' ); } } ); diff --git a/skins/common/preview.js b/skins/common/preview.js index 82b27bc1f3..9e76ca09ea 100644 --- a/skins/common/preview.js +++ b/skins/common/preview.js @@ -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) { diff --git a/tests/qunit/suites/resources/jquery/jquery.byteLimit.test.js b/tests/qunit/suites/resources/jquery/jquery.byteLimit.test.js index 70343fd67a..704740d9f3 100644 --- a/tests/qunit/suites/resources/jquery/jquery.byteLimit.test.js +++ b/tests/qunit/suites/resources/jquery/jquery.byteLimit.test.js @@ -93,10 +93,8 @@ byteLimitTest({ byteLimitTest({ description: 'Limit using the maxlength attribute', $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: $( '' ) - .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: $( '' ) - .attr( { - 'type': 'text', - 'maxLength': '6' - }) + .attr( 'type', 'text' ) + .prop( 'maxLength', '6' ) .byteLimit( function( val ) { _titleConfig(); diff --git a/tests/qunit/suites/resources/jquery/jquery.tablesorter.test.js b/tests/qunit/suites/resources/jquery/jquery.tablesorter.test.js index 2efd6cd9a9..556cb10fad 100644 --- a/tests/qunit/suites/resources/jquery/jquery.tablesorter.test.js +++ b/tests/qunit/suites/resources/jquery/jquery.tablesorter.test.js @@ -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(); } diff --git a/tests/qunit/suites/resources/mediawiki.special/mediawiki.special.recentchanges.test.js b/tests/qunit/suites/resources/mediawiki.special/mediawiki.special.recentchanges.test.js index fca2a80b11..fee2223089 100644 --- a/tests/qunit/suites/resources/mediawiki.special/mediawiki.special.recentchanges.test.js +++ b/tests/qunit/suites/resources/mediawiki.special/mediawiki.special.recentchanges.test.js @@ -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(); -- 2.20.1