Contrib.social

TableDrag JS :first-of-type issues

Open on Drupal.org →
Created on 21 October 2019, about 5 years ago
Updated 10 January 2024, 11 months 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

11.0 🔥

Component
Javascript  →

Last updated 1 day ago

Created by

🇬🇧United Kingdom AndyF

Live updates comments and jobs are added and updated live.
  • JavaScript

    Affects the content, performance, or handling of Javascript.

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.

  • Comment almost 2 years ago →
    needs-review-queue-bot

    The Needs Review Queue Bot → tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

    Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

    Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

  • Comment almost 2 years ago →
    🇮🇳India ayush.khare

    Reroll of #43 for 10.1.x

  • Status changed to Needs review almost 2 years ago
  • Comment almost 2 years ago →
    🇮🇳India nikhil_110
  • Status changed to Needs work almost 2 years ago
  • Comment almost 2 years ago →
    🇺🇸United States smustgrave

    This seems like a difficult one to test but from what I can tell it has something to do with parent value.

    Tried using the test module provided by the patch. Can it be updated to include parent value as well.

  • Comment over 1 year ago →
    🇬🇧United Kingdom AndyF

    Tried using the test module provided by the patch. Can it be updated to include parent value as well.

    Thanks for your time @smustgrave! I'm not really sure what you're asking for here?

    Does it really need work rather than review? I tried my best in the IS, there's a test that fails without the patch, it seems to me like NR is appropriate?

    Thanks again!

  • Comment 11 months ago →
    🇫🇮Finland heikkiy Oulu

    The patch does not seem to apply anymore against core 10.2.

  • Status changed to Needs review 11 months ago
  • Comment 11 months ago →
    🇺🇸United States wells Seattle, WA

    Re-roll of #45 attached for Drupal 10.2.x.

  • Open in Jenkins → Open on Drupal.org →
    Environment: PHP 8.1 & MySQL 5.7
    last update 11 months ago
    Custom Commands Failed
  • Status changed to Needs work 11 months ago
  • Comment 11 months ago →
    🇺🇸United States smustgrave

    Recommend turning to an MR for quicker reviews.

    #51 seems to have failures.

  • First commit to issue fork.
  • Merge request !6090TableDrag JS :first-of-type fixed → (Open) created by Unnamed author
  • Status changed to Needs review 11 months ago
  • Comment 11 months ago →
    🇮🇳India gauravvvv Delhi, India
  • Status changed to Needs work 11 months ago
  • Comment 11 months ago →
    🇺🇸United States smustgrave

    New functions should use type hints and returns.

    Also learned we can use constructor promotion.

  • Comment 11 months ago →
    🇫🇮Finland heikkiy Oulu

    I can also confirm that the latest merge request applies against 10.2.

    Are there any clear instructions how to repeat the issue without the patch? Would make it easier to test that it works.

    I originally applied this patch in our project because we had trouble reordering the fields in the node edit form layout but I also had other related patches applied so I would like to confirm I am testing the right thing.

contrib.social Blog FAQ Discussions
Production build 0.71.5 2024