Problem with the "active" class of tablesort

Problem exists in HEAD, too. Attached is a screenshot (from 7.x-dev).

Probably related to #557040: theme_table() adds 'active' class for the wrong columns when using rowspan (insofar, that we should be able to fix both at the same time).

Patch attached.
Fixes problems with all sorts of colspan and rowspan weirdness, except one special case were I think even the HTML result would be unspecified – and you'd have to be pretty insane to even try it.
And because I'm a good monkey, there are even tests for it. Rejoice! (The first patch contains only the tests and is there to demonstrate that it previously failed horribly. (Here's to hoping the second one doesn't fail equally horribly!))

Needs screenshot.

Needs screenshot, work, and removal of debug() calls, which I left, as always, lying around …

Revised patch attached (only minus three debug() lines and plus one isset()).

#5: tablesort_active-5.patch queued for re-testing.

Test works fine locally, and I couldn't tell what in my patch would influence that test.

So, tests pass – anyone wants to set this to RTBC?

+++ includes/tablesort.inc	11 Oct 2010 13:04:04 -0000
@@ -186,34 +186,6 @@ function tablesort_header($cell, $header
-function tablesort_cell($cell, $header, $ts, $i) {

I dont know if webchick/dries will be cool with removing a whole function like this after we have released beta.
On the other hand this does seem to be the correct way to fix this bug

+++ modules/simpletest/tests/theme.test	11 Oct 2010 13:04:08 -0000
@@ -157,6 +157,84 @@ class ThemeTableUnitTest extends DrupalW
+
+

extra space

Anyone else want to chime in?

(needs work for the white space)

Powered by Dreditor.

I dont know if webchick/dries will be cool with removing a whole function like this after we have released beta.
On the other hand this does seem to be the correct way to fix this bug

You're right, of course, this came up in another issue as well. Even if core doesn't use the function, we can't be sure what contrib modules already do. Removing this now would be a completely unnecessary API change.
So, keeping the function without using it anymore, and adding a @todo for Drupal 8.

Also removed the two superfluous lines.

Oops, seems like the DRUPAL_ROOT for the drupal_add_js() call was a stupid idea after all …

#12: tablesort_active-12.patch queued for re-testing.

Still seems to work for me, nobody else here interested?

#12: tablesort_active-12.patch queued for re-testing.

Weird, test results above show this passed … Also works for me locally.
However, needs a re-roll anyways, since -p0 is now deprecated. See attached.

This now needs to be fixed in D8 first, so maybe you want to take care of that "todo" for the D8 versin of the patch

Only then will it be backported to D7

Ah, you're right, of course.

This patch is roughly the same as the patch in #18 but it reduces the amount of code needed by checking that $cell is an array much earlier on. I'm happy with this... lets get one more review for good luck and then we can mark RTBC

#19: 838096-tablesort_active-D8-18.patch queued for re-testing.

I can reproduce this locally:
fatal: corrupt patch at line 116
Please re-roll. (Couldn't tell what's wrong with the patch or that line, though.)

hmmm ... try this Mr. Bot

+++ b/includes/theme.inc
@@ -1710,6 +1726,7 @@ function theme_table($variables) {
+    $rowspans = array(); // Keeps track of the current column when rowspans are involved.

Comments should go on a separate line and have no more than 80 characters per line.
(Yes, I know, I had that wrong, too, but that was a long time ago …)

+++ b/modules/simpletest/tests/theme.test
@@ -156,6 +156,83 @@ class ThemeTableUnitTest extends DrupalWebTestCase {
+  }
+
 }

While I personally like this style better, core policy seems to be to omit that newline between last method and end of class.

Attached: a patch that should fix this.
(By the way: The "18" (or, now, "25") at the end of the filename stands for the comment number, to easily keep track of what version of the patch that is. Meaning, you should increment that number when creating new patches.)

Powered by Dreditor.

Issue #1277874: table cells incorrectly get class 'active' seems to target the same code. However, it looks like the patch for this issue may - as an unforeseen but positive side effect - solve that issue as well.

Might it, or does it? In the latter case, we could just call the other one a duplicate and maybe make additional efforts to get this one in.

I changed the patch for the move to /core and reviewed and tested it as well.

Remarks:

• Reduced a comment to less than 80 characters.
• I removed some spaces in $this-> assertTrue (...)
• The assertTrue in the test might/should(?) be replaced by assertEqual(..., 1, ...) as preg_match returns an int or false,
• In #1277874: table cells incorrectly get class 'active' I talked about 2 headers having the same text, which would go wrong as well. As it turns out, the text of the header is used as argument in the sort parameter, thus that use case is not supported at all. Supporting it would mean exposing (non-translated) field names to users (as part of the query part of the URL in the address bar) which is something we might not want to do.

Considering that my changes have nothing to do with the "real code changes" code changes in the patch itself, I consider this one RTBC, but would like to ask bleen18 or drunken money (or anyone else) to have a last look at it and really set it to RTBC.

I'm happy with the changes from #25 & #28 ...

OK, than we set it to RTBC.

Thanks for your work on this! Please don't mark your own patch RTBC, though. :)

@bleen18: so you will have to set it to RTBC yourself.

+++ b/core/modules/simpletest/tests/theme.testundefined
@@ -172,6 +172,82 @@ class ThemeTableUnitTest extends DrupalWebTestCase {
+   * Tests if correct sort links are generated for headers where requested and
+   * if the "active" class is set correctly.

Minor thing: This should be just one line, less than 80 chars.

+++ b/core/modules/simpletest/tests/theme.testundefined
@@ -172,6 +172,82 @@ class ThemeTableUnitTest extends DrupalWebTestCase {
+    $header = array(
+      array(
+        'data' => 'Header12',
+        'field' => 'a',
+        'sort' => 'asc',
+        'colspan' => 2,
+      ),
+      'Header3',
+      array(
+        'data' => 'Header4',
+        'field' => 'b',
+      ),
+      array(
+        'data' => 'Header5',
+        'field' => 'c',
+      ),
+    );
+    $rows = array(
+      array(
+        array('data' => 'Column1', 'rowspan' => 2,),
+        'Column2',
+        array('data' => 'Column34', 'colspan' => 2,),
+        'Column5',
+      ),
+      array(
+        'Row2Column2',
+        'Row2Column3',
+        'Row2Column4',
+        'Row2Column5',
+      ),
+    );
+    $this->content = theme('table', array('header' => $header, 'rows' => $rows));
+    $this->assertRaw('sort=desc&order=Header12', t('Correct link for active sort-column.'));
+    $this->assertTrue(preg_match('#<th [^>]*class="active"([^<]|<[^/])+Header12#', $this->content), t('Correct class in header for active sort-column.'));
+    $this->assertNoRaw('order=Header3', t('No link for normal column.'));
+    $this->assertRaw('sort=asc&amp;order=Header4', t('Correct link for inactive sort-column.'));
+    $this->assertFalse(preg_match('#<th [^>]*class="active"([^<]|<[^/])+Header4#', $this->content), t('No class in header for inactive sort-column.'));
+    $this->assertNoRaw(' data="', t('"!attr" not set as HTML attribute.', array('!attr' => 'data')));
+    $this->assertNoRaw(' field="', t('"!attr" not set as HTML attribute.', array('!attr' => 'field')));
+    $this->assertNoRaw(' sort="', t('"!attr" not set as HTML attribute.', array('!attr' => 'sort')));
+    $this->assertRaw(' class="active">Column1</td>', t('Correct class set on active column !num.', array('!num' => 1)));
+    $this->assertRaw(' class="active">Column2</td>', t('Correct class set on active column !num.', array('!num' => 2)));
+    $this->assertRaw(' class="active">Row2Column2</td>', t('Correct class set on active column !num of second row.', array('!num' => 2)));
+    $this->assertNoRaw(' class="active">Row2Column3</td>', t('No class set on inactive column !num of second row.', array('!num' => 3)));
+
+    $_GET['sort'] = 'asc';
+    $_GET['order'] = 'Header4';
+    $this->content = theme('table', array('header' => $header, 'rows' => $rows));
+    $this->assertRaw('sort=asc&amp;order=Header12', t('Correct link for inactive sort-column.'));
+    $this->assertFalse(preg_match('#<th [^>]*class="active"([^<]|<[^/])+Header12#', $this->content), t('No class in header for inactive sort-column.'));
+    $this->assertRaw('sort=desc&amp;order=Header4', t('Correct link for active sort-column.'));
+    $this->assertTrue(preg_match('#<th [^>]*class="active"([^<]|<[^/])+Header4#', $this->content), t('Correct class in header for active sort-column.'));
+    $this->assertNoRaw(' class="active">Column1</td>', t('No class set on inactive column !num.', array('!num' => 1)));
+    $this->assertRaw(' class="active">Column34</td>', t('Correct class set on active column with colspan.', array('!num' => 1)));
+    $this->assertRaw(' class="active">Row2Column4</td>', t('Correct class set on active column !num of second row.', array('!num' => 4)));
+    $this->assertNoRaw(' class="active">Row2Column5</td>', t('No class set on inactive column !num of second row.', array('!num' => 5)));
+
+    $_GET['sort'] = 'desc';
+    $_GET['order'] = 'Header5';
+    $this->content = theme('table', array('header' => $header, 'rows' => $rows));
+    $this->assertRaw('sort=asc&amp;order=Header12', t('Correct link for inactive sort-column.'));
+    $this->assertFalse(preg_match('#<th [^>]*class="active"([^<]|<[^/])+Header12#', $this->content), t('No class in header for inactive sort-column.'));
+    $this->assertRaw('sort=asc&amp;order=Header5', t('Correct link for active sort-column.'));
+    $this->assertTrue(preg_match('#<th [^>]*class="active"([^<]|<[^/])+Header5#', $this->content), t('Correct class in header for active sort-column.'));
+    $this->assertNoRaw(' class="active">Column1</td>', t('No class set on first column.'));
+    $this->assertRaw(' class="active">Column5</td>', t('Correct class set on active column.'));
+    $this->assertNoRaw(' class="active">Row2Column4</td>', t('No class set on inactive column !num of second row.', array('!num' => 4)));
+    $this->assertRaw(' class="active">Row2Column5</td>', t('Correct class set on active column !num of second row.', array('!num' => 5)));
+
+    unset($_GET['sort'], $_GET['order']);

Can we get some inline comments in this wall of assertions? Hard to scan.

Also, I don't think bleen's question from #10 has been addressed yet:

I dont know if webchick/dries will be cool with removing a whole function like this after we have released beta.
On the other hand this does seem to be the correct way to fix this bug.

That bit is not backportable, although it is fine for D8. So it would be good to get a separate D7 patch along with the D8 that does not remove the function.

- #11 addresses #10, though maybe not very explicitly. In D7 we leave that function as is, it just won't be called anymore by theme_table(). Patch for D7 is foreseen after the patch for D8 gets in.
- Comments could indeed be added. I will see what I can do.

I will see what I can do.

Well, that was not much I did since then :( Maybe someone else can step up? I'm quite busy with other modules and projects that feed me.

Triage during a BugSmash group triage meeting.

There has been no activity here for 10 years, so maybe this has been resolved?

Is this issue still a problem?

Since we need more information to move forward with this issue, I am keeping the status at Postponed (maintainer needs more info). If we don't receive additional information to help with the issue, it may be closed after three months.

Thanks!

Well, it’s been “fixed” in Drupal 8 insofar as that apparently doesn’t provide the active class on the sorted column anymore, so can’t make mistakes in that regard, either. However, the bug is still present in Drupal 7 – see screenshots. Attached is also a slightly modified re-roll of #16, the last D7 patch.