Last modified: 2011-04-15 08:54:33 UTC

Wikimedia Bugzilla is closed!

Wikimedia migrated from Bugzilla to Phabricator. Bug reports are handled in Wikimedia Phabricator.
This static website is read-only and for historical purposes. It is not possible to log in and except for displaying bug reports and their history, links might be broken. See T17400, the corresponding Phabricator task for complete and up-to-date bug report information.
Bug 15400 - Wikibits patch #3 - realign sorting icon
Wikibits patch #3 - realign sorting icon
Status: RESOLVED FIXED
Product: MediaWiki
Classification: Unclassified
Parser (Other open bugs)
unspecified
All All
: Low enhancement (vote)
: ---
Assigned To: Nobody - You can work on this!
: patch, patch-need-review
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2008-09-01 02:17 UTC by SharkD
Modified: 2011-04-15 08:54 UTC (History)
1 user (show)

See Also:
Web browser: ---
Mobile Platform: ---
Assignee Huggle Beta Tester: ---


Attachments
unified diff file (2.88 KB, patch)
2008-09-01 02:17 UTC, SharkD
Details
unified diff file (2.68 KB, patch)
2008-09-01 15:29 UTC, SharkD
Details
unified diff file (2.61 KB, patch)
2008-09-01 22:51 UTC, SharkD
Details
unified diff file (2.53 KB, patch)
2008-09-01 22:59 UTC, SharkD
Details
demonstration file (1.80 KB, application/x-zip-compressed)
2008-09-04 03:35 UTC, SharkD
Details
unified diff file (812 bytes, patch)
2008-09-05 23:19 UTC, SharkD
Details
unified diff file (824 bytes, patch)
2008-09-05 23:28 UTC, SharkD
Details
Slightly fixed-up version of previous patch, rebased to recent trunk (1.52 KB, patch)
2008-12-24 23:14 UTC, Aryeh Gregor (not reading bugmail, please e-mail directly)
Details

Description SharkD 2008-09-01 02:17:53 UTC
Created attachment 5237 [details]
unified diff file

Align the sorting icon so that it always remains in a fixed position and doesn't break onto new lines.
Comment 1 Daniel Friesen 2008-09-01 09:06:06 UTC
Note if someone applies this patch. The patch here also include "optimization fixes" and a "more verbose alt tag" which are parts of different bugs not relevant to this patch. Some of those have opposition to being applied.

I have a few issues with this patch as well.

What is with removing the sortable class? What if someone was using that to specially style sortable tables?

Why are you creating a new table inside of the cell headers? There has to be a better method of doing this with css, and if there isn't we shouldn't be doing this.
Have you even tried doing this with any extravagantly styled tables? It looks like if someone decided to use something fancy like outlines then styles would break.
Comment 2 SharkD 2008-09-01 15:28:22 UTC
(In reply to comment #1)
> Note if someone applies this patch. The patch here also include "optimization
> fixes" and a "more verbose alt tag" which are parts of different bugs not
> relevant to this patch. Some of those have opposition to being applied.
> I have a few issues with this patch as well.
> What is with removing the sortable class? What if someone was using that to
> specially style sortable tables?
> Why are you creating a new table inside of the cell headers? There has to be a
> better method of doing this with css, and if there isn't we shouldn't be doing
> this.
> Have you even tried doing this with any extravagantly styled tables? It looks
> like if someone decided to use something fancy like outlines then styles would
> break.

The "sortable" class is only removed from the inner table generated by this script. It is left unchanged on the outer, original table. All other classes/styles get copied to the new table.

I don't think there's a CSS solution that will work consistently across different browsers. The current version was tested by another user (see: http://en.wikipedia.org/wiki/User_talk:SharkD/Sandbox/wikibits_4) across several browsers and shown to work. I also did some proof-of-concept testing using browserhots.org, and it worked in 52 of 54 browsers tested.

Sorry about the other style changes that slipped in. I'll submit a new patch.
Comment 3 SharkD 2008-09-01 15:29:34 UTC
Created attachment 5249 [details]
unified diff file

Update, minus the changes to icon ALT text and FOR loop optimization.
Comment 4 Daniel Friesen 2008-09-01 16:19:46 UTC
(In reply to comment #2)
> The "sortable" class is only removed from the inner table generated by this
> script. It is left unchanged on the outer, original table. All other
> classes/styles get copied to the new table.
> 
Ya, I deleted half the comment after noticing you were inserting a table into the column. Initially it looked like you were recreating the entire table. Just missed that one.

> I don't think there's a CSS solution that will work consistently across
> different browsers. The current version was tested by another user (see:
> http://en.wikipedia.org/wiki/User_talk:SharkD/Sandbox/wikibits_4) across
> several browsers and shown to work. I also did some proof-of-concept testing
> using browserhots.org, and it worked in 52 of 54 browsers tested.
> 

I still don't really like the idea of tables. It's an ugly abuse of tables which the web is trying to move away from. I'd prefer something involving floats and using a spacer on the left to even things out if centered.
Comment 5 SharkD 2008-09-01 16:51:59 UTC
Iw ould prefer using floats, too. But, I have been having issues with floats (described here: http://en.wikipedia.org/wiki/Wikipedia:Village_pump_(technical)#Template.2FCSS_help), and I don't think they're supported as widely by browsers.

I wouldn't say the Web has moved very far from using tables for layout purposes. Just look at the Main page of Wikipedia and see how many tables it uses. The technology really hasn't advanced far enough for there to be a suitable alternative.
Comment 6 Daniel Friesen 2008-09-01 17:26:25 UTC
Well, to me for now Wikipedia's front page is fine. They are positioning a small section of the page.

What I dislike is using tables to align small icons, that I do consider abuse.

And what browsers don't support floats? Last I checked it was CSS1.

The only problem with floats is that on centered things the header is pushed a bit off center.
Though now that I think about it, tables probably give the same issue.

And does this patch consider rtl languages? Aren't they supposed to have icons like these on the left?
A float could easily take this into consideration.

As for the off center issue. It should be possible to make something float on the opposite side to even things out.
Comment 7 SharkD 2008-09-01 19:02:34 UTC
Yes, floats are CSS1. But, there are differences in how browsers handle them. A problem arises when the text is wider than the cell. In IE, the result is to break the text where it comes into contact with the floated content. Firefox breaks the entire text onto a new line.

IE: http://img117.imageshack.us/img117/2406/wikipediatablefloatbreaqj9.png

Firefox: http://img117.imageshack.us/img117/7079/wikipediatablefloatbreatk2.png

There are also problems with vertical alignement. When the text spans multiple lines, the floated content is aligned with the first row of text. This means that the content can't be vertically centered. If multiple lines of text exist, the floated content will be aligned with the first row.

See: http://img117.imageshack.us/img117/4795/wikipediatablefloatbreacm3.png

I'm also not sure what relevance the direction a language is written in has to a user interface element. User interface and text rendering are separate considerations. That said, the table could just as easily be configured to place the icon on the left depending on the language.
Comment 8 Aryeh Gregor (not reading bugmail, please e-mail directly) 2008-09-01 19:18:29 UTC
Could you post screenshots of the problem the patch fixes, how it looks before and after the patch?  I don't know what problem this is trying to solve.  From the verbal description I have to ask whether this couldn't be fixed just by using non-breaking spaces.
Comment 9 SharkD 2008-09-01 21:54:22 UTC
No problem.

Here's a screenshot of the old version:

http://img75.imageshack.us/img75/4307/wikipediatablesortableoig3.png

Notice how the arrow icons appear in all sorts of odd places. Sometimes they break onto lines all by themselves.

Here's a screenshot of the changed version:

http://img75.imageshack.us/img75/3145/wikipediatablesortablenyz1.png

The position of the icons is fixed. They always appear in the same place in a cell.
Comment 10 SharkD 2008-09-01 22:51:30 UTC
Created attachment 5252 [details]
unified diff file

Update. I've simplified the code to some degree, made the style more compact and fixed a bug I introduced while copying from my working, cumulative version.

Here's what this latest version looks like:

http://img296.imageshack.us/img296/5610/wikipediatablesortablefrc5.png
Comment 11 SharkD 2008-09-01 22:59:47 UTC
Created attachment 5253 [details]
unified diff file

That might have been *too* compact, sorry.
Comment 12 Aryeh Gregor (not reading bugmail, please e-mail directly) 2008-09-02 00:18:28 UTC
I see.  The way I'd be inclined to solve this is using absolute positioning.  This CSS more or less works for me:

table.sortable th {
	margin-right: 16px;
	position: relative;
	display: block;
}
.sortarrow {
	position: absolute;
	right: 2px;
	top: 50%;
}
.sortarrow img {
	position: relative;
	top: -5px;
}

In practice, we would want to add a wrapper div inside the header cells and use that for the first set of rules, instead of assuming table.sortable th will do what we want, but this should be good enough for demonstration purposes.  Also, this needs to be tested on various browsers, but I'm pretty sure it will work reliably, since it's just CSS1.  Although one never knows with IE6, which might require a workaround.
Comment 13 SharkD 2008-09-02 01:13:28 UTC
Could you create a demonstration file? In my experience, the browsers tend to be kind of finicky when mixing abeolute with relative positioning. Tables, on the other hand, have had much better support for a much longer time.
Comment 14 Aryeh Gregor (not reading bugmail, please e-mail directly) 2008-09-02 02:04:43 UTC
Just put that CSS in your user CSS on Wikipedia or something.
Comment 15 SharkD 2008-09-02 03:57:00 UTC
This is what it looks like in Firefox:

http://img383.imageshack.us/img383/7273/wikipediatablesortablecto5.png

This is what I mean when I say that support for "CSS solutions" is sketchy. Tables are tried and true and work with even very old browsers.
Comment 16 SharkD 2008-09-02 05:19:04 UTC
(In reply to comment #15)
> This is what it looks like in Firefox:
> http://img383.imageshack.us/img383/7273/wikipediatablesortablecto5.png
> This is what I mean when I say that support for "CSS solutions" is sketchy.
> Tables are tried and true and work with even very old browsers.

FYI, the Firefox behavior is the *correct* behavior. It's IE that is acting screwy in this case.
Comment 17 Aryeh Gregor (not reading bugmail, please e-mail directly) 2008-09-03 00:25:15 UTC
Yeah, okay, so you really do have to add a wrapper div.  (I was testing with a one-column table, so I didn't see that.)  No big deal, it's still a lot smaller than your solution.  A quick first attempt on my part didn't work, probably because something is making assumptions about how many levels deep something is being nested.

Anyway, I'm not going to commit a solution using this table hack, I'd like to see this as proper CSS.  We use tables more than enough already for layout in MediaWiki.
Comment 18 SharkD 2008-09-04 03:35:45 UTC
Created attachment 5287 [details]
demonstration file

I found a CSS solution that works well in the majority of browsers. I've attached two demonstration files showing the CSS and table methods in action. Using browsershots.org I tested the files in various browsers. 50 browsers were tested.

The CSS method didn't work in:
IE4
IE5.5
IE6
IE8
Dillo

The table method didn't work in:
IE4
IE8
Dillo

IE4 can be safely ignored as it's so ancient. IE8 is still in beta so things will probably change in due time. Dillo is a text-only browser I think, so it can be ignored as well. IE5.5 and IE6 are a problem though.
Comment 19 SharkD 2008-09-05 23:19:34 UTC
Created attachment 5293 [details]
unified diff file

OK, I got it to work in IE5.5 and IE6! I've attached a patch. In addition to the patch, the following changes need to be made to the stylesheet:

.sortheader {
	margin-right: 13px;
	position: relative;
	display: inline-block;
}
.sortarrow {
	position: absolute;
	right: -13px;
	top: 50%;
}
.sortarrow img {
	width:12px;
	height:14px;
	position: relative;
	top: -6px;
}
Comment 20 SharkD 2008-09-05 23:28:04 UTC
Created attachment 5294 [details]
unified diff file

Woops. I made a few too many changes that broke the rest of the code. Here's a new patch minus those additional changes.
Comment 21 Aryeh Gregor (not reading bugmail, please e-mail directly) 2008-12-24 23:14:08 UTC
Created attachment 5621 [details]
Slightly fixed-up version of previous patch, rebased to recent trunk

(Term is over and I can get back to reviewing patches.  I didn't forget about this.)

This is the same patch but with the following changes:

1) Works with current trunk, after r44879 (which I committed specifically so this patch would be readable).

2) Removed border="none" on the image, which seems unnecessary.  We already do this in CSS.

3) Adjusted one extra line to account for the extra div.  Without this extra line, actually clicking the icon fails.

I haven't tested in browsers other than Firefox 3 yet.  display: inline-block looks kind of weird . . .
Comment 22 Aryeh Gregor (not reading bugmail, please e-mail directly) 2008-12-24 23:24:44 UTC
Okay, so this doesn't work in IE5 (but it does in IE5.5), and doesn't as recently as Opera 9.23, a 2007 release (it works in Opera 9.5).  It works on Firefox since 1.0, and Safari since at least 3.0 (didn't test earlier).  I don't know if this slight aesthetic improvement is worth breaking compatibility with those browsers.
Comment 23 DieBuche 2011-04-15 08:54:33 UTC
"Align the sorting icon so that it always remains in a fixed position and
doesn't break onto new lines."
Fixed in r86088

Note You need to log in before you can comment on or make changes to this bug.


Navigation
Links