I ran into this problem because I used a JavaScript library to restyle checkboxes and it relies on a change event to be fired after the prop "checked" has been changed.

Since this happens most of the time when jQuery is used to set a prop by using jQuery's .prop() function I tried to rewrite this in jQuery like:

$.propHooks.checked = {
  set: function(elem, value, name) {
    var ret = (elem[ name ] = value);
    $(elem).trigger("change");
    return ret;
  }
};

But ATM tableselect.js is using element.checked = true; what prevents the possibility to do the above.

So I would suggest to either use jQuery to set the prop (see attached patch) or to even fire the change event directly after setting the "checked" prop in vanilla JS.

CommentFileSizeAuthor
#11 core-js-tableselect-2604912-11.patch1.94 KBstockholmz
#7 core-js-tableselect-2604912-7.patch1.97 KBstockholmz
#3 core-js-tableselect-2604912-3.patch666 bytessdstyles
core-js-tableselect.patch630 bytespixelmord
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pixelmord created an issue. See original summary.

droplet’s picture

droplet CreditAttribution: droplet commented
Category: Task » Bug report
Status: Active » Needs work
Issue tags: +Novice, +js-novice

the patch exposed a gateway to jQuery but didn't address the main issue. we need to trigger a change event.

sdstyles’s picture

sdstyles CreditAttribution: sdstyles at FFW commented
Status: Needs work » Needs review
FileSize
core-js-tableselect-2604912-3.patch666 bytes
stockholmz’s picture

stockholmz CreditAttribution: stockholmz as a volunteer commented

#3 Patch is great. Fixes the issue, and removes the need for implementing custom jQuery hooks like stated in the bug.

stockholmz’s picture

stockholmz CreditAttribution: stockholmz as a volunteer commented
Status: Needs review » Reviewed & tested by the community
droplet’s picture

droplet CreditAttribution: droplet commented
Status: Reviewed & tested by the community » Needs work

only fire event on actually changes. (Needs an if condition)

stockholmz’s picture

stockholmz CreditAttribution: stockholmz as a volunteer commented
Status: Needs work » Needs review
FileSize
core-js-tableselect-2604912-7.patch1.97 KB

We could do something in this direction.

cheatlex’s picture

cheatlex CreditAttribution: cheatlex as a volunteer commented
Status: Needs review » Reviewed & tested by the community

Looks fine to me and when tested on simpletest it din't brake anything

droplet’s picture

droplet CreditAttribution: droplet commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/misc/tableselect.js
@@ -42,12 +42,17 @@
+        var stateChanged = ($checkbox.prop('checked') !== state ? true : false);

@@ -57,18 +62,22 @@
+          var stateChanged = ($checkbox.prop('checked') !== event.target.checked ? true : false);

can be simplified.
@see #2603988: Unnecessary ternary condition in toolbar JS

The last submitted patch, 3: core-js-tableselect-2604912-3.patch, failed testing.

stockholmz’s picture

stockholmz CreditAttribution: stockholmz as a volunteer commented
FileSize
core-js-tableselect-2604912-11.patch1.94 KB

#9 - Thanks for the heads up.

mitrpaka’s picture

mitrpaka CreditAttribution: mitrpaka as a volunteer commented
Status: Needs work » Needs review
droplet’s picture

droplet CreditAttribution: droplet commented
Status: Needs review » Reviewed & tested by the community
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs steps to reproduce

Thanks for the bugfix and thanks @droplet for the subsystem maintainer review and signoff!

Before we commit this to core, can we add steps to reproduce it to the issue? As much as possible they should use only core, but if there's module/etc. code that's needed it'd be good to include that on the issue too. This is especially important since we don't have automated frontend testing yet.

droplet’s picture

droplet CreditAttribution: droplet commented

In Drupal, our scripts modified the default HTML behaviours.

Standard:
https://jsfiddle.net/o07vzdu9/

So, if you run the script AFTER PATCH in Drupal in anywhere with Checkbox, you should get the change event.

This is especially important since we don't have automated frontend testing yet.

We should have more confidence on this topic. Especially when we working on Development branch, eg. D8.1 in future. From my experience, I can see most of JS errors come from PHP / Twig code refactoring more than JS changes itself.

In past day, I've spent few years in D8.0. However, I think most of my job can be done with 1 ~ 3 months. Our process now is very inefficiency IMO. We should think about it if we wanna a better Drupal frontend in future.

( Although I don't work on CSS in Drupal a lot, I think there have same problems. Even worse than JS IMO. )

droplet’s picture

droplet CreditAttribution: droplet commented
Status: Needs review » Reviewed & tested by the community
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Fixed

Thanks for the steps to reproduce.

Committed/pushed to 8.1.x and cherry-picked to 8.0.x. Thanks!

  • catch committed 9f53bf0 on 8.1.x 
    Issue #2604912 by stockholmz, pixelmord, sdstyles: Tableselect.js select...

  • catch committed 2105be5 on 
    Issue #2604912 by stockholmz, pixelmord, sdstyles: Tableselect.js select...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.