Mottie / tablesorter

Github fork of Christian Bach's tablesorter plugin + awesomeness ~
https://mottie.github.io/tablesorter/docs/
2.61k stars 754 forks source link

Clicking on a checkbox in a filter-false column still triggers a filter event #570

Closed lindonb closed 10 years ago

lindonb commented 10 years ago

I have a column of checkboxes and have set the column header class to filter-false and sorter-false. Nevertheless, when I click on a checkbox in one of the rows (to either check or uncheck the box), a filter event starts. Since no filter is set, the main result is that the page jumps to the top each time a checkbox is clicked. I am using ajax.

Mottie commented 10 years ago

Hi @lindonb!

To me that sounds like whatever script is attached to the checkboxes isn't stopping event propagation and possibly acting like a form submit.

Are you using any other libraries? Would you please share some code? Initialization code and whatever code you are using for the checkboxes would be a start.

lindonb commented 10 years ago

Thanks @Mottie!

This is not an issue with 2.14.5 with the same table - it started in the 2.15 series (the first few versions caused fatal errors for me so not sure which 2.15 version).

I am trying to implement tablesorter in the Tiki Wiki CMS Groupware package so, yes, there are other libraries, but that also makes it difficult to share code other than through a demo site.

You can see the issue at http://remote.frela.info/tiki-adminusers.php (you'll need to login with user name test and password 12345). Just scroll down until the top of the page is no longer visible and then click on one of the checkboxes on the left - the page will jump to the top. (Please ignore the messy appearance - this is a trunk version that is going through a theme overhaul.) Thanks, lindonb

lindonb commented 10 years ago

FYI, the first place tablesorter is called in when the checkbox is clicked is on line 80 of parser-input-select.js in this snippet:

$('table').find('tbody').on('change', 'select, input', function(e){
    if (!alreadyUpdating) {
        var $tar = $(e.target),
        $table = $tar.closest('table');
        alreadyUpdating = true;
        $table.trigger('updateCell', [ $tar.closest('td'), resort ]);    //line 80
        updateServer(e, $table, $tar);
        setTimeout(function(){ alreadyUpdating = false; }, 10);
    }
});

This block of code was there in 2.14 as well so probably not the source of the problem.

Mottie commented 10 years ago

I was looking for the problem earlier... I think Bootstrap might be the problem (ref) because the page doesn't scroll all the way to the top; instead, the "wiki" menu link ends up at the top. I've tried multiple methods to try to block any propagation on those links without success. Bootstrap uses event delegation to bind to those links, so it makes it difficult to block the event.

Could you try disabling Bootstrap and seeing if it is the root of the problem... if not, I'll search elsewhere.

lindonb commented 10 years ago

I tried 2.15.13 on an earlier version of Tiki before Bootstrap was added and I had the same issue. When I switch back to 2.14.4 it goes away. You're right, it doesn't quite go to the top for some reason. Thanks very much for taking a look. Regards, lindonb

lindonb commented 10 years ago

Hi @Mottie!

This is still an issue for me in version 2.16.1, including when not using bootstrap (see demo using url and credentials noted above). The page is not just jumping towards the top, but a filter event is triggered when the checkbox is clicked. The code below in pager-input-select.js picks up any change in an input element and then triggers updateCell:

    // update select and all input types in the tablesorter cache when the change event fires.
    // This method only works with jQuery 1.7+
    // you can change it to use delegate (v1.4.3+) or live (v1.3+) as desired
    // if this code interferes somehow, target the specific table $('#mytable'), instead of $('table')
    $(window).load(function(){
        // this flag prevents the updateCell event from being spammed
        // it happens when you modify input text and hit enter
        var alreadyUpdating = false;
        $('table').find('tbody').on('change', 'select, input', function(e){
            if (!alreadyUpdating) {
                var $tar = $(e.target),
                    $table = $tar.closest('table');
                alreadyUpdating = true;
                $table.trigger('updateCell', [ $tar.closest('td'), resort ]);
                updateServer(e, $table, $tar);
                setTimeout(function(){ alreadyUpdating = false; }, 10);
            }
        });
    });

This leads to a searching function in line 582 of jquery.tablesorter.widgets.js:

// pass true (skipFirst) to prevent the tablesorter.setFilters function from skipping the first input
// ensures all inputs are updated when a search is triggered on the table $('table').trigger('search', [...]);
ts.filter.searching(table, filter, true);

which calls checkFilters on line 1141:

ts.filter.checkFilters(table, filter, skipFirst);

which triggers the filterStart on line 806:

c.$table.trigger('filterStart', [filters]);

Because no filter is set, the same rows are returns and the page jumps to the top of the table if all the rows are not showing. So although it looks like it's just jumping to the top, it is actually filtering as well.

Thanks for taking a look. Regards, lindonb

Mottie commented 10 years ago

Actually, I think what you are seeing is the stickyHeaders widget moving the page to show the entire table. The reason this is done is in case you use the filter in the sticky header and no results are returned, or if the entire table ends up being above the viewport.

See line 1360 of the jquery.tablesorter.widgets.js file:

window.scrollTo(0, $table.position().top);

Comment out that line and see if that fixes your issue.

Mottie commented 10 years ago

Oops, the wrong issue was referenced.

lindonb commented 10 years ago

Thanks @Mottie - commenting out that line seems to do the trick!

Would you be able to fix this permanently in the program? We bring tablesorter as a third-party package in through composer and so try not to local make modifications to avoid upgrade headaches. Looks like when a real filter event occurs it still appropriately goes to the top of the page after filtering even with this line commented out.

Thanks! lindonb

Mottie commented 10 years ago

I guess I could add an option...

Mottie commented 10 years ago

Hey @lindonb!

Try the latest update to the stickyHeaders widget in the working branch:

https://github.com/Mottie/tablesorter/blob/working/js/jquery.tablesorter.widgets.js

lindonb commented 10 years ago

Hi @Mottie!

This doesn't work as well as commenting out the line. Everything is okay when I check a checkbox, but when I use a real column filter (like typing in a user name in the user column header) it no longer goes to the top of the page after filtering which can leave a blank page showing.

Seems like the real issue is that a tablesorter filter call shouldn't be invoked from clicking the checkbox at all since it is in a filter-false column (btw, although tablesorter runs through the functions noted above when a checkbox is clicked, for some reason it doesn't seem to trigger an ajax call to the database like a real filter call would).

What if tablesorter were changed to not trigger a filterStart or updateCell call if the table cell with the checkbox (not just the table header cell) had a class of filter-false? Thanks again, lindonb

Mottie commented 10 years ago

I think if you want to skip updating a checkbox, the column should have both the sorting and filters disabled. Either one would need updated parsed data. So, I've modified the parser-input-select.js file.

Again, the updated file is in the working branch:

https://github.com/Mottie/tablesorter/blob/working/js/parsers/parser-input-select.js

lindonb commented 10 years ago

Perfection! Works like a charm - thanks so much.

Not sure if you still want to keep the stickyHeaders_filteredToTop option for other use cases but I won't need it after this fix. Regards, lindonb

Mottie commented 10 years ago

Great to hear! We can just keep that option, I don't think it would hurt anything to have it.