I've searched, but haven't found an issue for this. I'm not that good with JavaScript/jQuery so this might be me doing something wrong, but I can't see what.

I've tracked it down to the following problem: If the field, that gets the 'tabledrag-handle' prepended has a colspan, then the hidden column is not calculated correctly. The column that is one to the right of the column that should be hidden is hidden.

The markup of the table is fine, I've checked that without the tabledrag enabled. Also, if I replace the one field (the first one in the row) that has the colspan (2), with 2 regular fields each with colspan 1, everything works. That's what's lead me to believe this is actually a bug.

I can provide more info, of course, if that's needed.

Files: 

Comments

tim.plunkett’s picture

This sounds like a duplicate of #988930: Sticky table headers need to react properly to "show/hide weights column" link, does that issue cover what you are describing?

tstoeckler’s picture

Actually, I didn't have any problems with sticky headers, but the Show/hide weights link itself doesn't work correctly.

Berdir’s picture

Can you show your code? drupal_add_tabledrag() can be really tricky to get right...

tstoeckler’s picture

Well, I was able to reproduce it at least. Now we can find out if it's me or tabledrag :)
Try pasting this into a PHP-filter enabled node.
It's the same table twice, except that in one of them the first cell of the first row has a colspan of 2.

tstoeckler’s picture

Shameless bump.

nod_’s picture

Version: 7.x-dev » 8.x-dev

There are some changes coming for this, is this still an issue?

tstoeckler’s picture

Yup, the snippet still works to reproduce the bug in latest 8.x

nod_’s picture

Status: Active » Needs review

Your testing code wasn't working properly, two table with same id = problem.

Code to try out:


$output = '';
$output .= "This is broken:";
$header = array('First name', 'Last name', 'Weight');
$rows = array();
$rows[] = array(
  'data' =>array(
    array('data' => 'Mark Smith', 'colspan' => 2),
    array('data' => '
2
'), ), 'class' => array('draggable'), ); $rows[] = array( 'data' =>array( array('data' => 'Mark'), array('data' => 'Smith'), array('data' => '
2
'), ), 'class' => array('draggable'), ); drupal_add_tabledrag('test-table', 'order', 'sibling', 'test-weight'); $output .= theme('table', array('header' => $header, 'rows' => $rows, 'attributes' => array('id' => 'test-table'))); return $output;

This is going to be a pain to fix…

nod_’s picture

Status: Needs review » Needs work
tstoeckler’s picture

Yup, I can reproduce that, too.

I'm really not that good with jQuery/JS, so I won't be able to fix this, but I'll stay subscribed and will try any fix out.

Nephele’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
1.61 KB
1.54 KB
PASSED: [[SimpleTest]]: [MySQL] 63,260 pass(es). View

The attached patch fixes the problem in my testing. I've also attached the same patch backported to 7.x.

tstoeckler’s picture

@Nephele: It's really awesome when someone comes along and fixes an issue you had over 3 years ago! Thanks, you made my day! :-)

Unfortunately I could not get it work quite yet. I had to update the example code a bit because of updated core. This is what I used:

    $build = array(
      '#type' => 'table',
      '#header' => array('First name', 'Last name', 'Weight'),
      '#tabledrag' => array(array(
        'action' => 'order',
        'relationship' => 'sibling',
        'group' => 'weight',
      )),
      '#id' => 'test-table',
    );
    $build[1]['name'] = array(
      '#markup' => 'Mark Smith',
      '#wrapper_attributes' => array('colspan' => 2),
    );
    $build[1]['weight'] = array(
      '#markup' => 2,
    );
    $build[2]['first_name'] = array(
      '#markup' => 'Mark',
      '#wrapper_attributes' => array('class' => array('weight')),
    );
    $build[2]['last_name'] = array(
      '#markup' => 'Smith',
    );
    $build[2]['weight'] = array(
      '#markup' => 2,
      '#wrapper_attributes' => array('class' => array('weight')),
    );

    return $build;

I successfully reproduced the problem in 8.x but unfortunately your patch did not fix the problem. A drush cr didn't help either.

jhedstrom’s picture

Status: Needs review » Needs work
Issue tags: +Needs manual testing

Needs work per #12. Patch still applies.

nod_’s picture

Issue tags: +Needs reroll

And needs everything else basically :D

sushilkr’s picture

FileSize
10.97 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Invalid PHP syntax in core/includes/entity.inc. View

Rerolled

lokapujya’s picture

The reroll has changes to files other than the .js file that shouldn't be part of this patch. Can you post another patch and set to needs review so that it gets tested, and also remove needs reroll tag.

jgeryk’s picture

Assigned: Unassigned » jgeryk

going to fix up/reroll

googletorp’s picture

Assigned: jgeryk » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs reroll

Status: Needs review » Needs work

The last submitted patch, 15: 997370_15.patch, failed testing.

jgeryk’s picture

FileSize
1.63 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 101,049 pass(es). View

Fixed #15 patch file to only include changes to tabledrag.js

alx_benjamin’s picture

Status: Needs work » Needs review
lokapujya’s picture

Status: Needs review » Needs work
alx_benjamin’s picture

@lokapujya, why are we changing status to needs work. There is a patch that passed testing. So it needs to be reviewed by community and if appropriate committed to repo.

If you are changing status to needs work than please give us reasons why and what work needs to be done.

Or am I wrong?

lokapujya’s picture

I didn't have time to write up a review; I was going to come back and add a comment, but had to take off. The patch needed more work than just a reroll before the reroll, so it clearly needs work still.

Since the php module for Drupal 8 doesn't seem to be working, the way to reproduce the problem is to put the code from #12 into a custom block.

Just quickly reviewing it there are some white space errors. Also, the next patch should come with screenshots that show it working.

+++ b/core/misc/tabledrag.js
@@ -278,7 +278,8 @@
-              cell = field.closest('td');
+			   // Find the parent cell -- whether the match was in a td or th element.
++              cell = field.closest('td, th');

white space, extra + ?

lokapujya’s picture

The fix is probably somewhere in this function:
Drupal.tableDrag.prototype.addColspanClass
because it's the only code that adds "tabledrag-hide" and that is what gets messed up when you add a colspan. That class is no longer on the weight column, but on one of the other span columns.

lokapujya’s picture

If you refresh the page while the weights are hidden, the table colspan looks ok. if you refresh the page while the weights are visible, the colspan expands to 3 columns. In both cases, the weight column doesn't disappear.

After applying the patch, and making the fix described in #24, plus moving the :
'#wrapper_attributes' => array('class' => array('weight')),
line in example from 12 from
$build[2]['first_name']
to
$build[1]['weight']
everything actually seems to be working!

lokapujya’s picture

updated code used to reproduce problem:

<?php

    $build = array(
      '#type' => 'table',
      '#header' => array('First name', 'Last name', 'Weight'),
      '#tabledrag' => array(array(
        'action' => 'order',
        'relationship' => 'sibling',
        'group' => 'weight',
      )),
      '#id' => 'test-table',
    );
    $build[1]['name'] = array(
      '#markup' => 'Mark Smith',
      '#wrapper_attributes' => array('colspan' => 2),
    );
    $build[1]['weight'] = array(
      '#markup' => 2,
      '#wrapper_attributes' => array('class' => array('weight')),
    );
    $build[2]['first_name'] = array(
      '#markup' => 'Mark',
    );
    $build[2]['last_name'] = array(
      '#markup' => 'Smith',
    );
    $build[2]['weight'] = array(
      '#markup' => 2,
      '#wrapper_attributes' => array('class' => array('weight')),
    );

    return $build;
?>
lokapujya’s picture

droplet’s picture

+++ b/core/misc/tabledrag.js
@@ -289,7 +290,16 @@
+          cell.parent().children().each(function (n) {
+            if (n < origIndex && this.colSpan && this.colSpan > 1) {
+              columnIndex += this.colSpan - 1;
+            }

one thing I don't like #28 is loop the same elements twice. I did some work before but hasn't time to do further clean up.

lokapujya’s picture

The code in #29 doesn't work. The weight column is still visible.

Droplet, can you further explain where the #28 code is looping twice?

doesn't the #29 code loop twice also?:

+++ b/core/misc/tabledrag.js
@@ -311,15 +317,24 @@
+      $cells.each(function (n) {
+        if (n < origIndex && this.colSpan && this.colSpan > 1) {
+          index += this.colSpan - 1;
+        }
+      });
droplet’s picture

doesn't the #29 code loop twice also?:

We have to simplify it in further :)

droplet’s picture

Status: Needs work » Needs review
FileSize
1.62 KB

OK. I will clean up the tabledrag in other thread. Here's the version without twice loop.

lokapujya’s picture

what do you meant by the other thread?

droplet’s picture

@lokapujya,

#29 I added some performance tweaks & code cleanup that do not related to this issue and I removed in #32.

lokapujya’s picture

OK, but also #29 didn't work as I mentioned in #30 (the weight was still visible), so is this fixed now?

swetashahi’s picture

Assigned: Unassigned » swetashahi
swetashahi’s picture

FileSize
269.67 KB
305.59 KB

Tested with SimplyTest.me patch 32. Issue still exists. On the blocks page, on clicking hide row weights the operations column gets hidden

Screenshot with row weights hidden and shown

hidden

shown

swetashahi’s picture

Assigned: swetashahi » Unassigned
Status: Needs review » Needs work
lokapujya’s picture

We may need to go back to the #28 patch that was working.

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.