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.

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

pixelmord created an issue. See original summary.

droplet’s picture

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

Status: Needs work » Needs review
FileSize
666 bytes
stockholmz’s picture

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

stockholmz’s picture

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

Status: Reviewed & tested by the community » Needs work

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

stockholmz’s picture

Status: Needs work » Needs review
FileSize
1.97 KB

We could do something in this direction.

cheatlex’s picture

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

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

#9 - Thanks for the heads up.

mitrpaka’s picture

Status: Needs work » Needs review
droplet’s picture

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

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

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

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

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.