Refactor tableselect.js

No big issues with the selectors, some can use some help and get rid of sizzle-specific selectors.

More importantly, this needs some work to make this more readable.

claiming this one for now.

Initial stab at it. Still 1 @todo left, comments should be improved, and the code needs some serious reviewing :).

Unassigning for the moment. Might get back to it as I find time for it.

Reviewing my own patch:

+  detach: function (context, settings) {
+    // @todo: Remove event handlers and 'select all' checkboxes.
+  },

Should be without trailing ','.

Leaving at 'needs review' for testbot.

grmbl. Stupid status switch. Might as well update the patch then. Ran it through JSHint, and fixed some errors/warnings stemming from that.

GRMBL. Trailing space.

Couple of pointers regarding the patch:

• Tried to follow a similar pattern as tableheader.js
• It's now possible to have multiple tableselects on the same page (something that was not possible in the old file, I think)
• There was some weirdness going on due to sticky tables. I tried to make that sane by introducing an event in tableheader after the sticky table gets created.
• Another concern was the behavior of the checkbox when it gets cloned into the sticky table header. Fixed it by changing the cloning of tableheader from a shallow clone to a deep clone.
• Every selector and element is cached, so it should be fast even on mobile and large tables (haven't checked though!)
• The shift click behavior has been totally rewritten, I understood what the point of the code was but found it really confusing code.

Addendum to #8:

Patch basically needs to be extensively tested for:

• correct synchronisation between toggling the 'select all' checkbox and the cloned checkbox in the sticky header.
• Correct shift-click behavior
• If the select all checkbox (both in the sticky header and the regular header) gets checked automatically if all checkboxes are selected, either manually and/or via shift click. Possibly mixed in with disabled checkboxes.
• performance on mobile for large tables.

+    // If all checkboxes are checked, make sure the select-all one is checked
+    // too, otherwise keep unchecked.
+    var state = self.checkboxSum === self.checkboxMaxSum ? true : false;
+    self.updateSelectAll(state);

This is similar to the old code, but a possible improvement would be only updating the selectAll if it doesn't have the state it should have. Could be done by simply keeping track of the selectAll state in the object.

Something like this:

// If all checkboxes are checked, make sure the select-all one is checked
// too, otherwise keep unchecked.
var state = self.checkboxSum === self.checkboxMaxSum ? true : false;
if (self.selectAllState !== state) {
  self.updateSelectAll(state);
}

constructor:

// Keep track of properties.
this.selectAllState = null;

createSelectAll:

this.selectAllState = false;

updateSelectAll:

this.selectAllState = state;

Going to fix the @todo and add the comment in #10 somewhere today. Also going to see if I can make the initial creation and state of the tableselect more robust (ie for the edgecase where all checkboxes are already checked at pageload).

Whitespace issues again. Lucky number 13!

By the way: I realize that it looks like there are over 4 times as much lines added than removed; after minification this will change to a little less than 2x the number of characters (1500 old minified file vs ~2900 characters new minified file). I'd call the code increase due to having a bit more explicite coding style, more comments, ability to handle multiple tableselects on the same page, and a detach event.

+ if ($selectAll.length) {
+ if ($table.find('td input:checkbox').length !== 0) {
+ if ($stickyTable.length === 1) {
Is there a convention how to check for existence of jQuery elements? I'm using if($foo.length) and if($foo.length !== 0), what should be used?

+      // If all checkboxes are checked, make sure the select-all one is checked
+      // too, otherwise keep it unchecked.
+      if (this.checkboxChecked === this.checkboxMaxChecked) {
+        state = true;
+      }
+      else {
+        state = false;
+      }

Here, and in some other places, I choose to use an if/else structure. Is the shorthand var a = b ? c : d; allowed or even recommended?

+  toggleSelectAll: function (e) {
+    var self = e.data.TableSelect;
+    var state = $(e.target).prop('checked');

+  toggleCheckbox: function (e) {
+    var self = e.data.TableSelect;
+    var currentCheckbox = e.target;

And would it be more straight forward to use e.target or this? I'm leaning towards this, since this seems to be suggested by the jQuery docs.

if ($foo.length) { }
if (!$foo.length) { }
suffices for me, but I don't think there's an agreed-on standard right now, perhaps we should look into creating one.
jQuery uses this internally when checking for the presence of an array value, which is essentially what we're doing, and it is a pretty widely used standard within the JavaScript/jQuery community.

Where the use of a ternary can easily lead to confusion, a full if statement with curly braces is recommended.
In the case where you're storing exactly the value of your condition I would be fine with doing it directly, but with braces around the condition, like: a = (b === c);
Constructs like the following seem overly verbose to me, but again, there's no standard set in stone or anything and they might help readability for some people.

// imagine 'a' is a boolean already
if (a) {
  b = true;
}
else {
  b = false;
}

Works for me!

Interdiff against version 19. Actually forgot to remove the item from cache in detach.

heckbox -> checkbox

+      // Reference to heckbox DOM node.
+      this.checkboxes[i] = $checkboxes[i];

Haven't tested the detach code _at all_.

+Drupal.behaviors.TableSelect = {
+  attach: function (context) {
+    tableSelectInitHandler(context);
+  },
+
+  detach: function (context) {
+    tableSelectRemoveHander(context);
   }
 };

Could probably be

+Drupal.behaviors.TableSelect = {
+  attach: tableSelectInitHandler(context),
+  detach: tableSelectRemoveHander(context)
};

+      // Remove this entry from the cache.
+      tables.splice(i, 1);

Probably has to be changed to TableSelect.tables.splice(i, 1);, and has to be moved outside the loop: removing an item like this forces the array to be reindexed, which makes the i parameter unreliable at best. The only way I see it (while still supporting IE8, which does not support indexOf or filter):

var indexes = [];
for (var i = 0, il = tables.length; i < il; i++) {
  ...
  indexes.push(i);
  ...
}

// The removed entries need to be removed from cache as
// well, but because .splice() re-indexes an array the only
// way to do this safely is by going through the indexes in
// descending order.
indexes.sort(function (a,b) {return b-a});
for (var i = 0, il = indexes.length; i < il; i++) {
  TableSelect.tables.splice(indexes[i], 1);
}

Setting to needs work for #21 and this comment.

• Fixed #21, and the last 2 comments of #22. The first one errored out. No biggie.
• Fixed another typo TableSelect -> TabeSelect
• Also removed the .once() class in .detach(). This prevented (obviously) proper re-attachment.
• Did some quick basic testing with repeatedly attaching/detaching via firebug, seems to work ok (only tested with one TableSelect instance)

Damm you whitespace!

#25: 1751308-24.patch queued for re-testing.

+++ b/core/misc/tableselect.jsundefined
@@ -1,93 +1,277 @@
-})(jQuery);
+}(jQuery, Drupal));

should be

})(jQuery, Drupal);

But that's not why the test is failing.

I thought I copied that from tableheader.js.

Yes I did: http://drupalcode.org/project/drupal.git/blob/refs/heads/8.x:/core/misc/...

So I'll fix it there as well in the next patch.

#25: 1751308-24.patch queued for re-testing.

I added some basic *created/*removed events in this patch. I think it would be incredibly helpful if these events had extra parameters containing the index of the instance and a reference to the instance. The second parameter is easy to add, the first might take some thinking if it is _really_ necessary.

Testbot came back green, setting back to needs work for #29/#30 and the above comment.

Sorry, I was wrong, apparently }(jQuery, Drupal)); was the agreed on structure.

interdiff vs 25

.removeOnce() should be used instead of manually removing the class added by .once().

While fixing #36, I thought of an obscure bug: tableselect and tableheader are applied: no biggie, it works. tableselect gets detached and attached while tableheader is left alone: stickyTableCreated will not fire. Plus, the checkbox from the cloned header is removed.

Tried to fix it, but it took more lines than I'm comfortable with :/. Any thoughts/ideas how to fix this more elegantly are more than welcome.

Ignore the attachments of #37. Forgot to commit on my dev branch.

I'm not too keen on having "on" in handler names, and would prefer not having sizzle-specific selectors, but other than that, I don't have any real objections.

Fixed both. I think.

So basically, I think this is ready....

But: I'd love some input from either seutje of nod_ (or anyone else who wants to weigh in) on how to handle the sticky table/tableselect interaction nicely.

At this point, there is slightly awkward code to keep the sticky table in sync. Because the following situations can occur:

1. tableselect is created first, then tableheader. No biggie, expected. Tableselect is bound to tableheader creation event, so synchronizes.
2. tableheader is created first, then tableselect. slightly dirty code to make tableselect synchronize.
3. both exist, tableheader gets removed. Should still work, though the selector for the 'select all' now contains a reference to a deleted element? Shouldn't matter with jQuery.
4. both exist, tableselect is removed. Should work ok.

BAH. I need to stop being sloppy.

+    // Let other scripts know a sticky table header was created.
+    this.$stickyTable.trigger('stickyTableCreated', this);

+    // Trigger an event letting other scripts know TableSelect was removed.
+    $table.trigger('TableSelectRemoved', tables[i]);

+    // Let other scripts know a 'select all' checkbox was created.
+    this.$table.trigger('TableSelectCreated', this);

Should they start with lowercase because they are event names, or uppercase because they reference to an instance of a constructor?

Sorry for being a bit unclear, by "not having sizzle-specific selectors", I meant changing :checkbox to [type=checkbox], which speeds up things on qSA-capable browsers (which is all we support anyway) (perf case -> http://jsperf.com/sizzle-vs-qsa ). The loop-check you changed it too seems a bit awkward and error-prone (it bails as soon as it detects an input that isn't a checkbox?)

On the issue raised in #42: I would say lowercase, as they are custom event names and don't really reference the constructor instances.

Haven't yet properly looked into the issue raised in #41 though

Seutje, will change as soon as I have some time. Thanks for the review.

Needs testing, think I fixed it correctly. #41 still stands.

tag

#45: 1751308-45.patch queued for re-testing.

Here is a related issue that I created, so please consider including this here in the refactored code as well:
https://www.drupal.org/node/2604912