Contrib.social

TableDrag JS :first-of-type issues

Open on Drupal.org →
Created on 21 October 2019, over 5 years ago
Updated 30 January 2023, about 2 years ago

In the docs for drupal_attach_tabledrag() it mentions

In a more complex case where there are several groups in one column (such as the block regions on the admin/structure/block page), a separate subgroup class must also be added to differentiate the groups.

I've been trying to get that working with minimal additional JS and I think I might've uncovered some bugs in tabledrag. When trying to use subgroups I was finding that dragging a row from one group into the other didn't seem to copy the subgroup class across correctly.

In #2489826: tabledrag is broken → (dcf9ab4) some changes were made to some tabledrag jQuery selectors. I think the changes were meant to be functionally equivelent, just a little more efficient, eg:

-    var $indentationLast = $item.find('td').eq(0).find('.js-indentation').eq(-1);
+    var $indentationLast = $item.find('td:first-of-type').find('.js-indentation').eq(-1);

IIUC the starts of both of those lines basically do the same thing - grab the first td. But there are some other situations where I think the behaviour changed, and I wonder if that was unintentional.

  1. --- a/core/misc/tabledrag.js
+++ b/core/misc/tabledrag.js
@@ -718,7 +718,7 @@
         // take into account hidden rows. Skip backwards until we find a draggable
         // row.
         while ($row.is(':hidden') && $row.prev('tr').is(':hidden')) {
-          $row = $row.prev('tr').eq(0);
+          $row = $row.prev('tr:first-of-type');
           row = $row.get(0);
         }
         return row;

    In this case $row.prev('tr:first-of-type') will only return a value if the previous row is also the first row in the table, rather than iterating each previous row. I've reverted that in the patch, but I wonder if the whole while block is redundant: $row is set from $(this.table.tBodies[0].rows).not(':hidden') at the start of findDropTargetRow(). Should it just be removed? 

  2. @@ -766,9 +766,9 @@
     }
     // Siblings are easy, check previous and next rows.
     else if (rowSettings.relationship === 'sibling') {
-      $previousRow = $changedRow.prev('tr').eq(0);
+      $previousRow = $changedRow.prev('tr:first-of-type');
       previousRow = $previousRow.get(0);
-      var $nextRow = $changedRow.next('tr').eq(0);
+      var $nextRow = $changedRow.next('tr:first-of-type');
       var nextRow = $nextRow.get(0);
       sourceRow = changedRow;
       if ($previousRow.is('.draggable') && $previousRow.find('.' + group).length) {

    This is what caused the original problem and prevented the weight subgroup class being copied over when moving a row into a different group. As before it's looking for the previous row using first-of-type and in this case it means the source row for sibling relationships isn't correctly set. The patch should fix and test this. 

  3. @@ -811,7 +811,7 @@
         // Use the first row in the table as source, because it's guaranteed to
         // be at the root level. Find the first item, then compare this row
         // against it as a sibling.
-        sourceRow = $(this.table).find('tr.draggable').eq(0).get(0);
+        sourceRow = $(this.table).find('tr.draggable:first-of-type').get(0);
         if (sourceRow === this.rowObject.element) {
           sourceRow = $(this.rowObject.group[this.rowObject.group.length - 1]).next('tr.draggable').get(0);
         }

    The original line found the first row with the draggable class but the modified version looks for a row which is both the first and has the draggble class. This causes an issue with field_group on a table with a non-draggable first row: 🐛 Drag and drop acts weird, sometimes not resetting the parent, or even clearing the region value Needs review . The patch should fix and test this.

    4. This is my first time touching tabledrag so careful review welcome :p

    Also it's quite an old commit that introduced this, I know tabledrag is used in lots of places, so I'm not sure if I'm just missing something obvious... (:

    Remaining tasks

  • Decide whether the first JS block above should be removed entirely.
  • Should this be split up into multiple tickets with more meaningful names?
🐛 Bug report
Status

Needs work

Version

10.1 ✨

Component
Javascript  →

Last updated 3 days ago

Created by

🇬🇧United Kingdom AndyF

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

contrib.social Blog FAQ Discussions
Production build 0.71.5 2024