Part of the JavaScript selectors clean-up effort.
#1574470: Selectors clean-up
#1415788: Javascript winter clean-up

CommentFileSizeAuthor
#45 1751308-45.patch14.23 KBKiphaas7
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1751308-45.patch. Unable to apply patch. See the log in the details link for more information. View
#45 interdiff.txt5.66 KBKiphaas7
#40 1751308-40.patch14.68 KBKiphaas7
PASSED: [[SimpleTest]]: [MySQL] 40,396 pass(es). View
#40 interdiff.txt3.96 KBKiphaas7
#38 1751308-38.patch14.21 KBKiphaas7
PASSED: [[SimpleTest]]: [MySQL] 40,370 pass(es). View
#38 interdiff.txt3.06 KBKiphaas7
#37 1751308-37.patch14.3 KBKiphaas7
PASSED: [[SimpleTest]]: [MySQL] 40,375 pass(es). View
#37 interdiff.txt235 bytesKiphaas7
#35 1751308-35.patch13.55 KBKiphaas7
PASSED: [[SimpleTest]]: [MySQL] 40,325 pass(es). View
#35 interdiff.txt2.03 KBKiphaas7
#25 1751308-24.patch13.47 KBKiphaas7
PASSED: [[SimpleTest]]: [MySQL] 40,329 pass(es). View
#23 1751308-23.patch13.54 KBKiphaas7
PASSED: [[SimpleTest]]: [MySQL] 40,320 pass(es). View
#20 1751308-20.patch12.87 KBKiphaas7
PASSED: [[SimpleTest]]: [MySQL] 40,737 pass(es). View
#20 interdiff.txt4.13 KBKiphaas7
#19 1751308-19.patch12.91 KBKiphaas7
PASSED: [[SimpleTest]]: [MySQL] 40,733 pass(es). View
#13 1751308-13.patch13.26 KBKiphaas7
PASSED: [[SimpleTest]]: [MySQL] 40,703 pass(es). View
#12 1751308-12.patch13.26 KBKiphaas7
PASSED: [[SimpleTest]]: [MySQL] 40,704 pass(es). View
#7 1751308-7.patch11.38 KBKiphaas7
PASSED: [[SimpleTest]]: [MySQL] 40,633 pass(es). View
#6 1751308-6.patch11.38 KBKiphaas7
PASSED: [[SimpleTest]]: [MySQL] 40,635 pass(es). View
#3 1751308-3.patch11.33 KBKiphaas7
PASSED: [[SimpleTest]]: [MySQL] 40,632 pass(es). View
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

nod_’s picture

Title: Selectors clean-up: tableselect.js » 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.

Kiphaas7’s picture

Assigned: Unassigned » Kiphaas7

claiming this one for now.

Kiphaas7’s picture

Status: Active » Needs review
FileSize
11.33 KB
PASSED: [[SimpleTest]]: [MySQL] 40,632 pass(es). View

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

Kiphaas7’s picture

Assigned: Kiphaas7 » Unassigned

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

Kiphaas7’s picture

Status: Needs review » Needs work

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.

Kiphaas7’s picture

Status: Needs work » Needs review
FileSize
11.38 KB
PASSED: [[SimpleTest]]: [MySQL] 40,635 pass(es). View

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

Kiphaas7’s picture

FileSize
11.38 KB
PASSED: [[SimpleTest]]: [MySQL] 40,633 pass(es). View

GRMBL. Trailing space.

Kiphaas7’s picture

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.
Kiphaas7’s picture

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.
Kiphaas7’s picture

Status: Needs review » Needs work
+    // 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;
Kiphaas7’s picture

Assigned: Unassigned » Kiphaas7

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).

Kiphaas7’s picture

Status: Needs work » Needs review
FileSize
13.26 KB
PASSED: [[SimpleTest]]: [MySQL] 40,704 pass(es). View
Kiphaas7’s picture

FileSize
13.26 KB
PASSED: [[SimpleTest]]: [MySQL] 40,703 pass(es). View

Whitespace issues again. Lucky number 13!

Kiphaas7’s picture

Assigned: Kiphaas7 » Unassigned

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.

Kiphaas7’s picture

Status: Needs review » Needs work

+ 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?

Kiphaas7’s picture

+  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.

seutje’s picture

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;
}
Kiphaas7’s picture

FileSize
12.91 KB
PASSED: [[SimpleTest]]: [MySQL] 40,733 pass(es). View

Works for me!

Kiphaas7’s picture

Status: Needs work » Needs review
FileSize
4.13 KB
12.87 KB
PASSED: [[SimpleTest]]: [MySQL] 40,737 pass(es). View

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

Kiphaas7’s picture

heckbox -> checkbox

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

Haven't tested the detach code _at all_.

Kiphaas7’s picture

Status: Needs review » Needs work
+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.

Kiphaas7’s picture

Status: Needs work » Needs review
FileSize
13.54 KB
PASSED: [[SimpleTest]]: [MySQL] 40,320 pass(es). View
  • 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)

Status: Needs review » Needs work

The last submitted patch, 1751308-23.patch, failed testing.

Kiphaas7’s picture

Status: Needs work » Needs review
FileSize
13.47 KB
PASSED: [[SimpleTest]]: [MySQL] 40,329 pass(es). View

Damm you whitespace!

The last submitted patch, 1751308-24.patch, failed testing.

Kiphaas7’s picture

Status: Needs work » Needs review

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

Status: Needs review » Needs work
Issue tags: +JavaScript clean-up

The last submitted patch, 1751308-24.patch, failed testing.

seutje’s picture

+++ 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.

Kiphaas7’s picture

Status: Needs review » Needs work

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.

Kiphaas7’s picture

Status: Needs work » Needs review

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

Kiphaas7’s picture

Status: Needs work » Needs review

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.

Kiphaas7’s picture

Assigned: Unassigned » Kiphaas7
Status: Needs review » Needs work
seutje’s picture

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

Kiphaas7’s picture

Assigned: Kiphaas7 » Unassigned
Status: Needs work » Needs review
FileSize
2.03 KB
13.55 KB
PASSED: [[SimpleTest]]: [MySQL] 40,325 pass(es). View

interdiff vs 25

Kiphaas7’s picture

Status: Needs review » Needs work

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

Kiphaas7’s picture

Status: Needs work » Needs review
FileSize
235 bytes
14.3 KB
PASSED: [[SimpleTest]]: [MySQL] 40,375 pass(es). View

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.

Kiphaas7’s picture

FileSize
3.06 KB
14.21 KB
PASSED: [[SimpleTest]]: [MySQL] 40,370 pass(es). View

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

seutje’s picture

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.

Kiphaas7’s picture

FileSize
3.96 KB
14.68 KB
PASSED: [[SimpleTest]]: [MySQL] 40,396 pass(es). View

Fixed both. I think.

Kiphaas7’s picture

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.
Kiphaas7’s picture

Status: Needs review » Needs work

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?

seutje’s picture

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

Kiphaas7’s picture

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

Kiphaas7’s picture

Status: Needs work » Needs review
FileSize
5.66 KB
14.23 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1751308-45.patch. Unable to apply patch. See the log in the details link for more information. View

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

nod_’s picture

Issue tags: +Needs JS testing

tag

nod_’s picture

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

Status: Needs review » Needs work

The last submitted patch, 1751308-45.patch, failed testing.

pixelmord’s picture

Issue summary: View changes

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

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.