(bug 38152) jquery.tablesorter: Use .data() instead of .attr()

* .data() is faster when looking up repeatedly

* .data() will return the latest value, even if it has changed over
   time, whereas .attr() only gives the value as it was when the page
   was generated. Much like the difference between attr() and prop()
   when it comes to value, checked, disabled etc.

* Moved buildCache() call outside `if ( firstTime )`.
  Tests show that this is fast enough to do on the fly. Left inline
  comment with proposal on how to make this more elaborate in the
  future would it become necessary.

* jquery.tablesorter.test.js:
 - Add tests for bug 38152
 - Clean up double quotes
 - Use more descriptive id (prefix with mw-bug-)

Change-Id: Ie4f801c5b1f93617fd3fd173d2eaaf173a7604e9

diff --git a/RELEASE-NOTES-1.20 b/RELEASE-NOTES-1.20
index 75d7704..d70b042 100644 (file)
--- a/RELEASE-NOTES-1.20
+++ b/RELEASE-NOTES-1.20
@@ -149,6 +149,9 @@ upgrade PHP if you have not done so prior to upgrading MediaWiki.
 * (bug 31895) mw.loader mode now correct when triggered from a $.fn.ready
   handler that is bound before mediawiki.js's handler (e.g. browser-userscripts
   like greasemonkey).
+* (bug 38152) jquery.tablesorter: Use .data() instead of .attr(), so that live
+  values are used instead of just the fixed values from when the tablesorter
+  was initialized.
 
 === API changes in 1.20 ===
 * (bug 34316) Add ability to retrieve maximum upload size from MediaWiki API.

diff --git a/resources/jquery/jquery.tablesorter.js b/resources/jquery/jquery.tablesorter.js
index 21e1f96..08272a5 100644 (file)
--- a/resources/jquery/jquery.tablesorter.js
+++ b/resources/jquery/jquery.tablesorter.js
@@ -60,7 +60,7 @@
 
        /* Local scope */
 
-       var     ts,
+       var ts,
                parsers = [];
 
        /* Parser utility functions */
@@ -77,9 +77,15 @@
 
        function getElementText( node ) {
                var $node = $( node ),
-                       data = $node.attr( 'data-sort-value' );
-               if ( data !== undefined ) {
-                       return data;
+                       // Use data-sort-value attribute.
+                       // Use data() instead of attr() so that live value changes
+                       // are processed as well (bug 38152).
+                       data = $node.data( 'sortValue' );
+
+               if ( data !== null && data !== undefined ) {
+                       // Cast any numbers or other stuff to a string, methods
+                       // like charAt, toLowerCase and split are expected.
+                       return String( data );
                } else {
                        return $node.text();
                }
@@ -611,9 +617,16 @@
                                                        explodeRowspans( $table );
                                                        // try to auto detect column type, and store in tables config
                                                        table.config.parsers = buildParserCache( table, $headers );
-                                                       // build the cache for the tbody cells
-                                                       cache = buildCache( table );
                                                }
+
+                                               // Build the cache for the tbody cells
+                                               // to share between calculations for this sort action.
+                                               // Re-calculated each time a sort action is performed due to possiblity
+                                               // that sort values change. Shouldn't be too expensive, but if it becomes
+                                               // too slow an event based system should be implemented somehow where
+                                               // cells get event .change() and bubbles up to the <table> here
+                                               cache = buildCache( table );
+
                                                var totalRows = ( $table[0].tBodies[0] && $table[0].tBodies[0].rows.length ) || 0;
                                                if ( !table.sortDisabled && totalRows > 0 ) {
 

diff --git a/tests/qunit/suites/resources/jquery/jquery.tablesorter.test.js b/tests/qunit/suites/resources/jquery/jquery.tablesorter.test.js
index 3a02695..df0144c 100644 (file)
--- a/tests/qunit/suites/resources/jquery/jquery.tablesorter.test.js
+++ b/tests/qunit/suites/resources/jquery/jquery.tablesorter.test.js
@@ -408,7 +408,8 @@ test( 'bug 32047 - caption must be before thead', function() {
 test( 'data-sort-value attribute, when available, should override sorting position', function() {
        var $table, data;
 
-       // Simple example, one without data-sort-value which should be sorted at it's text.
+       // Example 1: All cells except one cell without data-sort-value,
+       // which should be sorted at it's text content value.
        $table = $(
                '<table class="sortable"><thead><tr><th>Data</th></tr></thead>' +
                        '<tbody>' +
@@ -424,30 +425,33 @@ test( 'data-sort-value attribute, when available, should override sorting positi
        data = [];
        $table.find( 'tbody > tr' ).each( function( i, tr ) {
                $( tr ).find( 'td' ).each( function( i, td ) {
-                       data.push( { data: $( td ).data( 'sort-value' ), text: $( td ).text() } );
+                       data.push( {
+                               data: $( td ).data( 'sortValue' ),
+                               text: $( td ).text()
+                       } );
                });
        });
 
        deepEqual( data, [
                {
-                       "data": "Apple",
-                       "text": "Bird"
+                       data: 'Apple',
+                       text: 'Bird'
                }, {
-                       "data": "Bananna",
-                       "text": "Ferret"
+                       data: 'Bananna',
+                       text: 'Ferret'
                }, {
-                       "data": undefined,
-                       "text": "Cheetah"
+                       data: undefined,
+                       text: 'Cheetah'
                }, {
-                       "data": "Cherry",
-                       "text": "Dolphin"
+                       data: 'Cherry',
+                       text: 'Dolphin'
                }, {
-                       "data": "Drupe",
-                       "text": "Elephant"
+                       data: 'Drupe',
+                       text: 'Elephant'
                }
-       ] );
+       ], 'Order matches expected order (based on data-sort-value attribute values)' );
 
-       // Another example
+       // Example 2
        $table = $(
                '<table class="sortable"><thead><tr><th>Data</th></tr></thead>' +
                        '<tbody>' +
@@ -463,28 +467,89 @@ test( 'data-sort-value attribute, when available, should override sorting positi
        data = [];
        $table.find( 'tbody > tr' ).each( function( i, tr ) {
                $( tr ).find( 'td' ).each( function( i, td ) {
-                       data.push( { data: $( td ).data( 'sort-value' ), text: $( td ).text() } );
+                       data.push( {
+                               data: $( td ).data( 'sortValue' ),
+                               text: $( td ).text()
+                       } );
                });
        });
 
        deepEqual( data, [
                {
-                       "data": undefined,
-                       "text": "B"
+                       data: undefined,
+                       text: 'B'
                }, {
-                       "data": undefined,
-                       "text": "D"
+                       data: undefined,
+                       text: 'D'
                }, {
-                       "data": "E",
-                       "text": "A"
+                       data: 'E',
+                       text: 'A'
                }, {
-                       "data": "F",
-                       "text": "C"
+                       data: 'F',
+                       text: 'C'
                }, {
-                       "data": undefined,
-                       "text": "G"
+                       data: undefined,
+                       text: 'G'
                }
-       ] );
+       ], 'Order matches expected order (based on data-sort-value attribute values)' );
+
+       // Example 3: Test that live changes are used from data-sort-value,
+       // even if they change after the tablesorter is constructed (bug 38152).
+       $table = $(
+               '<table class="sortable"><thead><tr><th>Data</th></tr></thead>' +
+                       '<tbody>' +
+                       '<tr><td>D</td></tr>' +
+                       '<tr><td data-sort-value="1">A</td></tr>' +
+                       '<tr><td>B</td></tr>' +
+                       '<tr><td data-sort-value="2">G</td></tr>' +
+                       '<tr><td>C</td></tr>' +
+               '</tbody></table>'
+       );
+       // initialize table sorter and sort once
+       $table
+               .tablesorter()
+               .find( '.headerSort:eq(0)' ).click();
+
+       // Change the sortValue data properties (bug 38152)
+       // - change data
+       $table.find( 'td:contains(A)' ).data( 'sortValue', 3 );
+       // - add data
+       $table.find( 'td:contains(B)' ).data( 'sortValue', 1 );
+       // - remove data, bring back attribute: 2
+       $table.find( 'td:contains(G)' ).removeData( 'sortValue' );
+
+       // Now sort again (twice, so it is back at Ascending)
+       $table.find( '.headerSort:eq(0)' ).click();
+       $table.find( '.headerSort:eq(0)' ).click();
+
+       data = [];
+       $table.find( 'tbody > tr' ).each( function( i, tr ) {
+               $( tr ).find( 'td' ).each( function( i, td ) {
+                       data.push( {
+                               data: $( td ).data( 'sortValue' ),
+                               text: $( td ).text()
+                       } );
+               });
+       });
+
+       deepEqual( data, [
+               {
+                       data: 1,
+                       text: "B"
+               }, {
+                       data: 2,
+                       text: "G"
+               }, {
+                       data: 3,
+                       text: "A"
+               }, {
+                       data: undefined,
+                       text: "C"
+               }, {
+                       data: undefined,
+                       text: "D"
+               }
+       ], 'Order matches expected order, using the current sortValue in $.data()' );
 
 });
 
@@ -527,8 +592,8 @@ test( 'bug 32888 - Tables inside a tableheader cell', function() {
 
        var $table;
        $table = $(
-               '<table class="sortable" id="32888">' +
-               '<tr><th>header<table id="32888-2">'+
+               '<table class="sortable" id="mw-bug-32888">' +
+               '<tr><th>header<table id="mw-bug-32888-2">'+
                        '<tr><th>1</th><th>2</th></tr>' +
                '</table></th></tr>' +
                '<tr><td>A</td></tr>' +
@@ -543,12 +608,13 @@ test( 'bug 32888 - Tables inside a tableheader cell', function() {
                'Child tables inside a headercell should not interfere with sortable headers (bug 32888)'
        );
        equals(
-               $('#32888-2').find('th.headerSort').length,
+               $( '#mw-bug-32888-2' ).find('th.headerSort').length,
                0,
                'The headers of child tables inside a headercell should not be sortable themselves (bug 32888)'
        );
 });
 
+
 var correctDateSorting1 = [
        ['01 January 2010'],
        ['05 February 2010'],