Fix colspan in tableselect

The patch looks excellent; I can find no way to improve the code style or implementation. However, there should be a one-line comment above the is_array() check to explain what kind of special case is being processed. Make life easy for the ones who read this code. :)

(On an unrelated note, it seems iffy that theme_tableselect would take almost the same input as theme_table (header and rows/options), but theme_tableselect relying on keyed arrays while theme_table uses only ordered ones. theme_tableselect's approach looks superior; perhaps theme_table could eventually be patched to do it the same way.)

Re-rolling.

It will surely be needed to achieve #301902: Allow more users to see the node admin page.

It is not really a good idea to apply something similar to theme_table(), as it is convenient sometimes to be able to add rows wherever you want without having to know which header it is related to, also (and mainly) theme_table() is not centered on table headers as it is possible to render tables without headers (not that it is a good idea, but it is a possibility).

Obviously, everything is an array now in most cases, as the links are now passed as renderable arrays... so we must check if we are dealing with an array of data directly, or another sort of array (i.e. an array of table cells).

Tests passed. I don't know why it didn't get up to the project server correctly.

re-attaching for the bot, no changes.

Also re-rolled for comment improvement. This is now blocking #301902: Allow more users to see the node admin page so bumping status.

looks good. please use -f switch on diff so we can see quickly what function you changed ... typo: determins

This is not blocking 301902 as explained in #301902-156: Allow more users to see the node admin page, so we can switch it back to normal... but it does not change the fact that it would be useful to be able to have multiple rows for an header.

Corrected the typo.

#13: 365554-tableselect-group-columns_4.patch queued for re-testing.

While working on a complicated tableselect in D7, I stumbled across a limitation in the tableselect element: it does not allow me to properly add cells in a row which spans multiple column cells.

Found this issue, and think my issue is related, since both are about issues with colspan and tableselect. However, mine is aimed at fixing colspan in row cells, while this issue was previously about fixing colspan in header rows.

With a small 2 line addition to the patch above (isset(cell row)), both issues can be squashed. Also, I think I fixed a small typo in a comment from the above patch ('An header' -> 'A header', h is not silent).

For my usecase (cell rows with colspan > 1), this patch fixes:

Case:

$header = array('1', '2', '3', '4');
$rows = array(
  array('data' => 'foo 1 and 2', 'colspan' => 2),
  array('data' => 'foo 3 and 4', 'colspan' => 2),
);

Result before: notices about undefined indexes (it expects 4 cells), and 4 cells per row, while only 2 were defined. Colspan attribute is applied correctly.
Result after: no notices, only 2 cells per row. Colspan attribute is applied correctly.

Changing title, bumping to D8, tagging as "needs backport to D7". Both D7 and D8 patches are attached.

One more tag...

... And I should upload identical patches...

We should be able to add an automated test for this. Also, before/after screenshots illustrating the bug would be helpful for reviewers.

Think I have a vague idea how to test for this (compare results from theme_table and theme_tableselect), but no idea where to add this test. Any pointers?

setting back to 'needs work'. Still would appreciate some pointers where to correctly add tests for this.

Thanks @Kiphaas7! The test will probably go in core/modules/simpletest/tests/form.test, potentially using or adding to form_test.module in the same directory. If you search for "tableselect" in that file, you should be able to find the existing tests for the functionality; look for any that might be related to the theming/rendering of it.

Applied versus 7 and result looks good.

Thanks @kristiaanvandeneynde! So now we just need an automated test.

These tests count the number of header cells/table cells in a rendered table. Should cover the cases of a colspaned header, a colspaned cell, and a colspaned cell under a colspaned header.

Patch could use some work, the comments are sometimes weird to read (e.g.: and two, and two) and there is duplicate code in there:

+  $items['form_test/tableselect/colspan'] = array(
+    'title' => 'Tableselect colspan test',
+    'page callback' => 'drupal_get_form',
+    'page arguments' => array('_form_test_tableselect_colspan_form'),
+    'access callback' => TRUE,
+    'type' => MENU_CALLBACK,
+  );
+  $items['form_test/tableselect/colspan'] = array(
+    'title' => 'Tableselect colspan test',
+    'page callback' => 'drupal_get_form',
+    'page arguments' => array('_form_test_tableselect_colspan_form'),
+    'access callback' => TRUE,
+    'type' => MENU_CALLBACK,
+  );

Right. Great example of why you don't cut patches at 3am. These should have the duplicate code fixed. I cleaned up the comments a bit (switching things like 'column one' to 'the first column') to make things a bit clearer.

Looks much better now :)

Attached simply removes/replaces unneeded t() on the assertion message texts. Reference: http://drupal.org/simpletest-tutorial-drupal7#t

...And, the final test looks great. Thanks @adharris and @kristiaanvandeneynde!

Thanks! Committed/pushed to 8.x, moving to 7.x for backport.

Here is backported patch for D7.

This patch instead please.

#33: tableselect-colspan-365554-33.patch queued for re-testing.

HEAD was broken, so retesting.

Maybe the wrong place, but seems highly related...
There's are other issues with the column count and colspans. However, this may be due to the show/hide row weights feature.

@doublejosh
This seems like a separate issue to me.
Seeing as this one is already in D8 and about to be ported to D7, I suggest you open an entirely new issue on the matter.

#37 was fixed in this issue: #988930: Sticky table headers need to react properly to "show/hide weights column" link

The problem with 33 is that small variables are easily offended, and previous patch failed to apply the karma operator (++) to i. i (not me) just sat there in the for loop waiting for some sort of acknowledgement for the essential if modest role she was playing in the big scheme of things. Just shows what a little love will do ya.

good spot August1914, was going to post a new patch for that. New patch looks good to me.

Reuploading as test-only and combined just to check the backported test.

Can someone clarify whether #41 is an issue in D8?

There were no code changes between the patches in #29 and #45, so the $i++ issue was introduced in the original backport and not committed to 8.x. However the patch in #45 removes a newline from form.test where it shouldn't (again, introduced in the original backport), attached reroll resolves this.

I tested #45 in Chrome, Firefox, and Safari on OS X, and IE 6-9 by enabling form_test via drush and looking at form_test/tableselect/colspan. All passed, so marking RTBC after manual testing and patch review.

I also did manual testing in Chrome, Firefox, and Safari on OS X before and after the patch in #33 in an attempt to reproduce the behaviour shown in #37. I focused on OS X browsers because of the file name and format of the screenshot attached, I did test IE8 and IE9 as well though. I tested /admin/structure/types/manage/article/fields from a fresh install with and without overlay, resizing the browser window and dragging rows around.

Just chiming in to say thanks for adding test coverage!

This will need documentation -- as it works rather differently than theme_table() does.

I agree with @joachim. Without documentation, I had trouble figuring out if this patch is an API change, API addition, or what - and that's important to know for Drupal 7. It also wasn't really obvious to me why we need to support this nested array thing, rather than relying on the caller to set 'colspan' properly everywhere (i.e., closer to what theme_table does)?... I'm not necessarily saying the patch is wrong, just that it's hard to tell.

There's also some minor code style issues (which appear to be present in the D7 backport only), e.g. this:

+ /**
+ * Test the tableselect #colspan functionality.

There is a space in front of the "/**" that shouldn't be there.

Setting back to needs work, but leaving this at Drupal 7 for now.

David_Rothstein, joachim: I'd say this is an API bug. So this patch would make the API more consistent, thus could also be called an API addition. I'll try to summarize why in this post:

theme_table() always allowed for using the colspan attribute for table cells in both the header and body of the table. Primarly the following two cases can exist:

1. A single header cell can represent a column with mutliple cells in each row (colspan > 1 on the header)
2. Multiple header cells can represent multiple columns with just a single row cell in each row (colspan > 1 on body cells)

theme_tableselect, which could be seen as nothing more than a fancy wrapper around theme_table, does not handle colspan correctly, because of the following piece of code in theme_tableselect():

// As theme_table only maps header and row columns by order, create the
// correct order by iterating over the header fields.
foreach ($element['#header'] as $fieldname => $title) {
  $row['data'][] = $element['#options'][$key][$fieldname];
}

The nasty side effect of this loop is that it assumes the number of cells in the header are equal to the number of cells in an arbitrary table body row; which is a wrong assumption when using colspan.

http://drupal.org/node/365554#comment-2230026 fixed usecase #1,
http://drupal.org/node/365554#comment-5548108 fixed usecase #2,
http://drupal.org/node/365554#comment-5811412 introduced tests for D8,
http://drupal.org/node/365554#comment-6188690 introduced tests for D7.

All this patch does is make the loop mentioned above more forgiving for usecase #1 and #2. I can see why you'd want to use colspan explicitly in the loop for testing this, but I can't really see how that should work out...?

But yeah, the added code is a bit more cryptic, though that's what the inline code comments should fix.

Visual representation of what this patch tries to enable for theme_tableselect. I still see theme_tableselect as a wrapper around theme_table, and theme_table was able to produce these results all along; the pre-processing code in theme_tableselect fails when it encounters a colspan-type structure.

http://jsfiddle.net/fcUjD/

#7: 365554-tableselect-group-columns_3.patch queued for re-testing.

#47: 365554-47-combined.patch queued for re-testing.

Hi all,

Sorry to dig up old, very old topics, but we've encountered this issue while making a Drupal 7 theme.
I've managed to created a patch for Drupal 7, it's completely different from what I see here, let me know how it goes for you with it.

Basically the patch update the colspan attribute of <td>'s elements having the class tabledrag-has-colspan.
The colspan's value to set is computed by computing the length of <th>'s available in the table, except hidden ones and including colspan values.

Patch updated.

And here's the combined patch of #47 and #62.

Updating the patch, replacing the .prop() jquery method with .attr().

Automatically adds the 'tabledrag-has-colspan' to body rows having colspan.

I didn't test this in depth, but the patch applies properly to Drupal core 7.65 and colspans are working in tableselect elements.

Patch in #66 breaks display of field management table for me.

Patch in #47 does not have this issue so using that.

The patch still applies to the 7.x branch.

Adding credit for the commit to D8.