Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Summary:
The JS code for views_bulk_operations needed a bit of work. The states of checkboxes were being changed, but the 'change' event was not being triggered. Instead the 'click' event was being used by the JS code (and maybe by other modules), which is the incorrect event to use. There was even a note about problems with using the 'click' event because of what it was doing.
I have done my best to correct these issues. I noticed some issues with the method for changing the state of checkboxes (using $().attr()) which will be an issue with jQuery 1.7+. I have not changed this, but I can do this alongside this change.
The short of the change is to always call $().change() on the checkbox elements after their state is changed so other functions can respond. The short of the jQuery 1.7+ issue is that instead of attr() or prop(), we should get the element which could be 'this' (no $) or $('input[type={"checkbox"]').get(0) or .each() and then directly modify the 'checked' property.
For example,
this.checked = !this.checked;
$(this).change();State:
I will submit a patch in the next comment (so that I know the issue #, etc).
Possible problems:
I have removed the triggering of the 'click' event since this will change the state of the checkbox. This should never have been used, but some people may rely on this. I don't know if they do or not.
Comments
Comment #1
damontgomery CreditAttribution: damontgomery commentedHere is the patch.
Comment #2
bojanz CreditAttribution: bojanz commentedThis makes sense, but it really slows down the unchecking of the "select all" checkbox. Before the patch it is instantaneous, after the patch it takes a second for the checkbox to uncheck itself.
Can we do something about that?
Comment #3
damontgomery CreditAttribution: damontgomery commentedCan you elaborate? I'm getting these speeds (eyeballed),
Select 1 item: instantly changes
Deselect 1 item: instantly changes
Select All: instantly changes
Deselect All: short pause (1 sec)
The only issue here is the last one, deselecting a group of boxes.
I added a call to a whole bunch of objects, but this is the whole point of the fix, to correctly alert other functions the checkbox has had it's state changed. These additional calls may cause a noticeable delay, but I haven't noticed. I don't have any solutions and I imagine any meaningful ones would require a refactor of the whole code.
I would proceed with getting the patch included in the project and then addressing speed concerns. There is an actual bug in the current code and this patch fixes it. If doing things the correct way is a little slower, than so be it. Doing a hack job on checkboxes (an area that already causes tons of issues with jQuery and otherwise) is asking for more problems.
Comment #4
maximpodorov CreditAttribution: maximpodorov commentedThe patch is re-rolled for the current dev version.
Comment #5
sepehr.sadatifar CreditAttribution: sepehr.sadatifar commentedThe last patch doesn't work against the jquery 1.10.2
My patch use jquery 'prop' function instead of 'attr' when it's availabe ( jquery 1.6+).
'prop' should be used over 'attr' as explained here.
Comment #6
maximpodorov CreditAttribution: maximpodorov commented@sepehr.sadatifar, your patch solves a part of the problem (use prop when available). My patch solves another part of the problem (generate change event). These patches must be combined.
Comment #7
sepehr.sadatifar CreditAttribution: sepehr.sadatifar commented@maximpodorov change event is triggered in my installation but maybe that's because I use jquery 1.10.2
which jquery version do you use? can you check if change event is triggered when using prop?
Comment #8
maximpodorov CreditAttribution: maximpodorov commentedI use jQuery 1.5, and change event is not triggered.
Comment #9
olofbokedal CreditAttribution: olofbokedal at Odd Hill commentedI used jQuery 1.10 but the change event wasn't trigger.
I changed the latest patch in order to trigger the change event as suggested by the first patch. I also saved which method to use, prop() or change(), in a variable which get rids of several if-statements.
Comment #10
joelpittet#2608360: "Select all" checkbox works only once Didn't realize the duplicate but I've committed the patch I created with a polyfill which does similar to #9 without the variable method.