While debugging code through Firebug, I noticed that one click() event was added to each BEF checkbox after each AJAX request (filtering the AJAX-enabled view). So after filtering the view 10 times, there will be 10 click() handlers attached to each checkbox.
To reproduce, create a view, enable AJAX on it, add some exposed filters as checkboxes with BEF, then inspect one of the checkboxes with Firebug + FireQuery before and after applying filters.
The issue comes from this snippet that runs every time the page is updated (so after each AJAX request) since it's attached to Drupal.behaviors:
// Add highlight class to checked checkboxes for better theming$('.bef-tree input[type="checkbox"], .bef-checkboxes input[type="checkbox"]')
// Highlight newly selected checkboxes
.click(function() {
this.checked
? $(this).parent().addClass('highlight')
: $(this).parent().removeClass('highlight');
})
// Highlight ones that are checked on page load
.filter(':checked').parent().addClass('highlight')
;
};
?>
The fix is straightforward, patch incoming.
Comment | File | Size | Author |
---|---|---|---|
#1 | js_leak.1473308-1.patch | 516 bytes | mdupont |
Comments
Comment #1
mdupontOops, here is the snippet:
Patch attached that ensures only 1 instance of this event will be attached to each checkbox.
Comment #2
mdupontComment #3
mikeker CreditAttribution: mikeker commentedSorry for the delay getting to this issue.
Does this still happen with the latest -dev release? The current JS uses the following selector:
$('.form-checkboxes.bef-select-all-none:not(.bef-processed)');
which should eliminate the need for a
.not('bef-processed')
Comment #4
mdupontIndeed, the JS code in the latest dev makes sure that the click event will only be added once.
However I see that the 7.x version is still using the '.not(".bef-processed").addClass("bef-processed")' pattern when D7 bundles the jQuery Once plugin that is the preferred way to do this, see http://drupal.org/node/171213 section "Javascript Behaviors".
Switching issue to a minor task since it's mainly a code quality improvement to use .once() in the parts of the code where it makes sense.
Comment #5
mikeker CreditAttribution: mikeker as a volunteer commentedIssue queue cleanup... I'm sorry for taking so long to get back to this issue!
@mdupont, you are completely correct,
once()
is the better way to handle that. And the 7.x-3.x branch has moved to that pattern. Closing as "won't fix" mostly because it has already been fixed the current branch and the 6.x branch is near it's end-of-life.Thanks.