[regression] tablesorter doesn't handle headers with colspan properly
Closed, ResolvedPublic

Description

We now always do explodeRowspans before the first sort, to accomodate the new .sort() function. This has the unintended consequence that you basically cannot use rowspans at all anymore, since the JS code will always immediately fragment them back into individual cells right now.

This is a side effect of: https://gerrit.wikimedia.org/r/22562


Version: 1.21.x
Severity: critical

bzimport added a project: MediaWiki-JavaScript.Via ConduitNov 22 2014, 12:49 AM
bzimport added a subscriber: Unknown Object (MLST).
bzimport set Reference to bz41886.
TheDJ created this task.Via LegacyNov 8 2012, 1:07 PM
Ironholds added a comment.Via ConduitNov 9 2012, 11:56 AM

If I'm understanding this right, this has essentially broken rowspan? Given the sheer number of pages this has altered I'm upping the priority - I'll try to give the bugmeister a kick.

Aklapper added a comment.Via ConduitNov 9 2012, 12:30 PM

Krinkle, Sam: Can you take a look at this, please?

High priority: sortable && rowspans don't work together at all, pretty visible for readers.

Example with "sortable" still enabled: https://en.wikipedia.org/w/index.php?title=2012_in_film&oldid=521988570#January.E2.80.93March

Potentially related past code changes (quick check):
https://gerrit.wikimedia.org/r/#/c/30649/ Update jQueryUI to 1.9.1
https://gerrit.wikimedia.org/r/#/c/23100/ make $.tablesorter treat alt like text

Copying from the thread on https://en.wikipedia.org/wiki/Wikipedia:Village_pump_%28technical%29#Javascript_screwing_up_tables :

"It appears that clicking the nth column header actually did sort the nth column, but the first header has colspan="3". For all subsequent headers this means that the column being sorted is two columns to the left of the clicked header. Any chance the table sorter could be modified to handle this?" PrimeHunter 14:14, 8 November 2012 (UTC)

"What is the logic about a colspan="3" on a header, and then saying that the column should be sortable, while that column actually contains 3 columns per row.. There are so many 'implied' assumptions being made by authors in such a process, it's difficult to explain that to a JS. Also probably not that 'accessible' to screenreaders btw. Perhaps we should just avoid such constructs on tables." —TheDJ 14:27, 8 November 2012 (UTC)

Note to myself: The previous issue for this example when values were shown in wrong table cells (making the table completely unreadable) was a markup issue: Using ! instead of | in this specific table: https://en.wikipedia.org/w/index.php?title=2012_in_film&diff=521988570&oldid=521959220

bzimport added a comment.Via ConduitNov 9 2012, 3:16 PM

sumanah wrote:

I agree that this is not HIGHEST priority, because there's no data loss, but it's pretty high. CC'ing Hoo & Matmarex to see whether they can provide a quick JS fix.

If lots of tables across the wikis are mistakenly using ! where they should have used |, and MediaWiki used to be forgiving and now it's not and the tables croak, we should address that (either with an educational campaign, a regex, or building some kind of forgiveness into the rendering).

bzimport added a comment.Via ConduitNov 9 2012, 3:17 PM

sumanah wrote:

But that last item (! instead of |) would be a separate bug.

Ironholds added a comment.Via ConduitNov 9 2012, 3:24 PM

Yeah. If (as I understand it) the VE will standardise formatting on save, it might be something worth temporarily punting on and asking them to sort out if/when they deploy.

Aklapper added a comment.Via ConduitNov 9 2012, 3:57 PM

(In reply to comment #4)

But that last item (! instead of |) would be a separate bug.

-> bug 41889

matmarex added a comment.Via ConduitNov 9 2012, 3:58 PM

Changing description from "tablesorter explodes rowspans now before ever
clicking sort" to "tablesorter doesn't handle headers with colspan properly",
as I understand that this is the real problem.

I'll try to look into it.

matmarex added a comment.Via ConduitNov 9 2012, 4:54 PM

Fixed in I89766c96. The headers now sort on all the columns they span over.

Anomie added a comment.Via ConduitNov 9 2012, 7:51 PM

Gerrit changeset 32593 fixes the original bug reported here (rowspans being exploded automatically).

Aklapper added a comment.Via ConduitNov 14 2012, 1:46 PM

This hasn't gotten in yet - could anybody do a review in gerrit?

matmarex added a comment.Via ConduitNov 16 2012, 9:45 PM

Change merged.

Should this be backported and deployed, maybe?

matmarex added a comment.Via ConduitNov 16 2012, 10:00 PM

Actually, not fixed yet; sorry for the confusion.

This bug would be fixed by change I89766c96 (mine), which isn't yet merged; the merged one, Ibe4cc7e9 (Brad's), fixes bug 41889 (which was split from this one).

Anomie added a comment.Via ConduitNov 19 2012, 5:54 PM

(In reply to comment #12)

Actually, not fixed yet; sorry for the confusion.

This bug would be fixed by change I89766c96 (mine), which isn't yet merged; the
merged one, Ibe4cc7e9 (Brad's), fixes bug 41889 (which was split from this
one).

There are actually three issues mentioned here:

  1. The original bug reported here, that tablesorter was exploding rowspans on page load instead of on first sort. That is fixed by Ibe4cc7e9.
  2. The bug this transformed into, that tablesorter doesn't sort correctly when headers are colspanned. That is fixed by I89766c96, once someone merges it.
  3. Bug 41889, that tablesorter gets cells out of order when row header cells are involved. I just submitted Icb674f7e to fix that one.
Aklapper added a comment.Via ConduitNov 26 2012, 4:27 PM

Brad: Thanks for the analysis!

The patches for 1. and 2. are merged, so only Icb674f7e for bug 41889 is left.

Anomie added a comment.Via ConduitNov 26 2012, 4:30 PM

Since 1 and 2 are both merged now, I'm going to close this bug. Bug 41889 is still open for that issue.

Ironholds removed a subscriber: Ironholds.Via WebFeb 12 2015, 4:59 PM

Add Comment