I don't like that "more verbose" alt text. We use a simple ↓ on purpose.

Alt text displays on the page when the image cannot be found. Your "Sort in ascending order." and "Sort in descending order." is quite large. In fact, far larger than 90% of the header columns that you will find in tables.

Did you even attempt to look at what the page output was when the browser could not find the image? If you bother to, I expect that you will see that this stretches header columns and makes them almost impossible to read.

mhorvath2161 wrote:

(In reply to comment #1)

I don't like that "more verbose" alt text. We use a simple ↓ on purpose.
Alt text displays on the page when the image cannot be found. Your "Sort in
ascending order." and "Sort in descending order." is quite large. In fact, far
larger than 90% of the header columns that you will find in tables.
Did you even attempt to look at what the page output was when the browser could
not find the image? If you bother to, I expect that you will see that this
stretches header columns and makes them almost impossible to read.

In IE, character entities are not supported as ALT text. It results in a square being rendered instead.

ayg wrote:

• // We have a first row: assume it's the header, and make its contents clickable links
• for (var i = 0; i < firstRow.cells.length; i++) {
• var cell = firstRow.cells[i];
• if ((" "+cell.className+" ").indexOf(" unsortable ") == -1) {
• cell.innerHTML += '&nbsp;&nbsp;<a href="#" class="sortheader" onclick="ts_resortTable(this);return false;"><span class="sortarrow"><img src="'+ ts_image_path + ts_image_none + '" alt="&darr;"/></span></a>';
• }
• }

+ // We have a first row: assume it's the header, and make its contents clickable links
+ for (var i = firstRow.cells.length - 1; i > -1; i--) {
+ var cell = firstRow.cells[i];
+ var cellClass = " " + cell.className + " ";
+ if (cellClass.indexOf(" unsortable ") == -1) {
+ if (cellClass.indexOf(" sortreverse ") == -1)
+ cell.innerHTML += '&nbsp;&nbsp;<a href="#" class="sortheader" onclick="ts_resortTable(this);return false;"><span class="sortarrow" sortdir="up"><img src="'+ ts_image_path + ts_image_none + '" alt="Sort in ascending order."/></span></a>';
+ else
+ cell.innerHTML += '&nbsp;&nbsp;<a href="#" class="sortheader" onclick="ts_resortTable(this);return false;"><span class="sortarrow" sortdir="down"><img src="'+ ts_image_path + ts_image_none + '" alt="Sort in descending order."/></span></a>';
+ }
+ }

Note, you're changing tabs to spaces here. Don't do this. It goes against our coding conventions, and also adds lines to the diff with only whitespace changes. (I've manually removed that change in my comments below for readability.) You've also included changes from your other patches that aren't relevant here, like changing the loop to run backwards, and removing some braces.

The alternate text is also much too long, as Daniel says. If &darr; doesn't work on IE, please file that as a separate bug.

• var arrowHTML; if (reverse) {
• arrowHTML = '<img src="'+ ts_image_path + ts_image_down + '" alt="&darr;"/>';
• newRows.reverse();
• span.setAttribute('sortdir','up');

+ span.innerHTML = '<img src="' + ts_image_path + ts_image_down + '" alt="Sort in ascending order."/>';
+ span.setAttribute('sortdir', 'up');
+ newRows.reverse();

	} else {

• arrowHTML = '<img src="'+ ts_image_path + ts_image_up + '" alt="&uarr;"/>';
• span.setAttribute('sortdir','down');

+ span.innerHTML = '<img src="' + ts_image_path + ts_image_up + '" alt="Sort in descending order."/>';
+ span.setAttribute('sortdir', 'down');

	}

See note above about the alt text. Does this change serve any other purpose? What does the removal of arrowHTML actually do?

I haven't looked at the rest of the patch yet (i.e., the important part).

mhorvath2161 wrote:

(In reply to comment #3)

See note above about the alt text. Does this change serve any other purpose?
What does the removal of arrowHTML actually do?
I haven't looked at the rest of the patch yet (i.e., the important part).

In the original version, the variable arrowHTML is used to "undo" a change done in the previous loop (i.e., the loop does something to all items in an array, arrowHTML is used to "undo" the change to one of those items).

The modified code does additional changes within that loop, and simply "undoing" it wasn't workable. Instead, I placed a check in the loop to not do anything to the single item.

ayg wrote:

Please fix the issues that were identified above -- changing alt text, changing tabs to spaces, changing loops to run backwards, adding lines that don't end with semicolons -- so I can more easily review this. For the time being I'll look at some of your other patches. (I'm trying to do an average of about one patch review per day right now, but it's split up between you and a couple of other people.)

mhorvath2161 wrote:

unified diff file

Update, minus other issues.

Attached:

ayg wrote:

• if ((" "+cell.className+" ").indexOf(" unsortable ") == -1) {
• cell.innerHTML += '&nbsp;&nbsp;<a href="#" class="sortheader" onclick="ts_resortTable(this);return false;"><span class="sortarrow"><img src="'+ ts_image_path + ts_image_none + '" alt="&darr;"/></span></a>';

+ var cellClass = " " + cell.className + " ";
+ if (cellClass.indexOf(" unsortable ") == -1) {
+ if (cellClass.indexOf(" sortreverse ") == -1)
+ cell.innerHTML += '&nbsp;&nbsp;<a href="#" class="sortheader" onclick="ts_resortTable(this);return false;"><span class="sortarrow" sortdir="up"><img src="'+ ts_image_path + ts_image_none + '" alt="&uarr;"/></span></a>';
+ else
+ cell.innerHTML += '&nbsp;&nbsp;<a href="#" class="sortheader" onclick="ts_resortTable(this);return false;"><span class="sortarrow" sortdir="down"><img src="'+ ts_image_path + ts_image_none + '" alt="&darr;"/></span></a>';

The indentation here is pretty wacky. You should indent new control structures. Also, there's a lot of repetition, and it takes a few seconds to spot the differences. Why not something more like:

		var cellClass = " " + cell.className + " ";
		if (cellClass.indexOf(" unsortable ") == -1) {
			var arrowDir = cellClass.indexOf(" sortreverse ") == -1 ? "up" : "down";
			cell.innerHTML += '&nbsp;&nbsp;<a href="#" class="sortheader" onclick="ts_resortTable(this);return false;"><span class="sortarrow" sortdir="'
				+ arrowDir + '"><img src="'+ ts_image_path + ts_image_none
				+ '" alt="&' + arrowDir[0] + 'arr;"/></span></a>';
		}

• spans[i].innerHTML = '<img src="'+ ts_image_path + ts_image_none + '" alt="&darr;"/>';

+ var thisSpan = spans[i]
+ if (thisSpan != span) {
+ var cellClass = " " + thisSpan.parentNode.parentNode.className + " "
+ if (cellClass.indexOf(" sortreverse ") == -1) {
+ thisSpan.setAttribute("sortdir", "up")
+ thisSpan.innerHTML = '<img src="' + ts_image_path + ts_image_none + '" alt="Sort in ascending order."/>'
+ } else {
+ thisSpan.setAttribute("sortdir", "down")
+ thisSpan.innerHTML = '<img src="' + ts_image_path + ts_image_none + '" alt="Sort in descending order."/>'
+ }
+ }

1. You're missing semicolons here.

2. As discussed, alt text like "Sort in descending order" is way too long. It will stretch out the table horribly. Try viewing the page with images disabled to determine appropriate alt text. An up/down arrow should be used, as it is now (if you think this is bad, again, file a new bug).

3. As above, you should be able to simplify this code by having a variable that equals "up" or "down" depending on the presence of the class.

mhorvath2161 wrote:

(In reply to comment #1)

I don't like that "more verbose" alt text. We use a simple &darr; on purpose.
Alt text displays on the page when the image cannot be found. Your "Sort in
ascending order." and "Sort in descending order." is quite large. In fact, far
larger than 90% of the header columns that you will find in tables.
Did you even attempt to look at what the page output was when the browser could
not find the image? If you bother to, I expect that you will see that this
stretches header columns and makes them almost impossible to read.

If the image dimensions are specified explicitely via CSS, then the length of the ALT text will have zero effect on the table size.

-Mike

ayg wrote:

If the image dimensions are given via CSS, and this actually stops things from stretching, then your alt text will be completely unreadable, since it will be crammed into a tiny box. This question is orthogonal to what's being discussed in the present bug, however, and you should start a new bug to discuss it if you want it changed.

mhorvath2161 wrote:

Again, you know nothing but decide to interfere anyway.

Go away.

(In reply to comment #10)

Again, you know nothing but decide to interfere anyway.

Go away.

It's interesting how your implicit claim about not being rude in bug 15402 comment #22 contrasts with this comment.

Aryeh is right that discussions about changing the ALT text has little to do with reverse sorting. Since the reverse sorting part is a lot less controversial that changing the ALT text, it's probably wiser to file a separate bug for the latter, so the former can be resolved more smoothly. Also, discussions about two different things at the same time are generally discouraged around here.

mhorvath2161 wrote:

(In reply to comment #11)

It's interesting how your implicit claim about not being rude in bug 15402
comment #22 contrasts with this comment.

Please don't cherry pick response to make a point, and please stay on topic.

Needs updated patch

sumanah wrote:

This patch no longer applies cleanly to trunk. SharkD, could you revise so it applies to current trunk? You may also want to grab MediaWiki 1.18 beta 1 ( http://lists.wikimedia.org/pipermail/mediawiki-announce/2011-November/thread.html ) to see whether current behavior obviates the need for a fix.

Thanks for contributing to MediaWiki.

(In reply to comment #14)

This patch no longer applies cleanly to trunk. SharkD, could you revise so it
applies to current trunk? You may also want to grab MediaWiki 1.18 beta 1 (
http://lists.wikimedia.org/pipermail/mediawiki-announce/2011-November/thread.html
) to see whether current behavior obviates the need for a fix.

Thanks for contributing to MediaWiki.

Note that the sortable table code has been completely rewritten, it's now in resources/jquery/jquery.tablesorter.js . I'm not sure whether the new table sorter has this feature, but if it does, the patch would probably have to be rewritten completely as well because the code it's patching doesn't exist any more.

[Removing 'patch' keyword as it no longer applies to anything.]