618550 - Add a symlink icon to dirlisting.css and better align icons relative to text

Status: RESOLVED FIXED

(Camino Graveyard :: General, defect)

Description

[philippe (part-time)]

(…and fix bug 133835 for Camino)

sample page with symlinks:
ftp://ftp.apnic.net/pub/stats/apnic/

proposal: an arrow similar to Apple's alias badge, filing the 16px space

Comment 1

[philippe (part-time)]

Attachment: the icon

optimised png

Comment 2

[philippe (part-time)]

Attachment: patch for dirlisting.css

As discussed on IRC, this includes declarations for margin, etc to match .dir::before, which is specified in a style block in the head of the xhtml document.
I've use some padding to better visually center the icon with the line of text.

screenshot in context:
[1] ftp://ftp.apnic.net/pub/stats/apnic/
    http://dev.l-c-n.com/camino/b618550/dirlisting-v0.9-1.png
[2] ftp://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/
    http://dev.l-c-n.com/camino/b618550/dirlisting-v0.9-2.png


(yes, I'm aware that adding the symlink icon increases the height of the row slightly, similar as what happens with the folder icon / row)

Comment 3

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Comment on attachment 497233 [details] [diff] [review]
patch for dirlisting.css

>diff --git a/geckochrome/skin/classic/global/dirListing/dirListing.css b/geckochrome/skin/classic/global/dirListing/dirListing.css

>+/* for .dir::before these rules are specified in an inline style block in
>+   the xhtml document

I'd reword this slightly, I think:

/* nsIndexedToHTML.cpp fails to include styles for .symlink::before to match
   styles for .dir::before and .file::before */

>+  padding-top: 2px; /* visually center the image with the text */
>+  vertical-align: middle;

(In reply to comment #2)
> I've use some padding to better visually center the icon with the line of text.

It's not necessarily this bug, but can we improve this for the files and directories, too, while we're here?

For folders, it looks like the text's baseline is at least 1px too low relative to the icon when compared to the Finder (where the folder icon is perhaps not necessarily centered in the row itself).

For files (comparing a full-height icon like a plain-text document), it looks like the text's baseline is about 2px too high relative to the icon.  (I'm aware dirListing has taller rows/more top&bottom padding around the text/icons than the Finder, so my comments are only about the relative placement of the text and the icon to each other.)

> (yes, I'm aware that adding the symlink icon increases the height of the row
> slightly, similar as what happens with the folder icon / row)

I think this was necessary; those rows looked like they were missing leading/padding before we added the icon!

Otherwise, I think this looks good, so r=me with those two things fixed (or a counter-argument about the icon-text alignment for folders/files).

Comment 4

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Comment on attachment 497231 [details]
the icon

The icon looks fine; r=ardissone

Comment 5

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: Packaging change

Here's the packaging change that's needed for the new image.

Comment 6

[philippe (part-time)]

Attachment: patch for dirlisting.css v 1.3 & packaging change

updated the stylesheet per comment 3
- change the comment
- adjusted the alignment of the folder and file images to match the Finder more closely

- added the packaging changes from comment 5

screenshots:
http://dev.l-c-n.com/camino/b618550/dirlisting-v1.3-1.png
http://dev.l-c-n.com/camino/b618550/dirlisting-v1.3-2.png
enlarged, pixie view
http://dev.l-c-n.com/camino/b618550/dirlisting-pixie.png

Comment 7

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Comment on attachment 497417 [details] [diff] [review]
patch for dirlisting.css v 1.3 & packaging change

>diff --git a/geckochrome/skin/classic/global/dirListing/dirListing.css b/geckochrome/skin/classic/global/dirListing/dirListing.css
>+.file > img {
>+  vertical-align: bottom;
>+}

This probably wants a matching comment about centering/alignment (can be done on checkin).

Comment 8

[Stuart Morgan]

Comment on attachment 497231 [details]
the icon

sr=smorgan

Comment 9

[Stuart Morgan]

Comment on attachment 497417 [details] [diff] [review]
patch for dirlisting.css v 1.3 & packaging change

>+/* nsIndexedToHTML.cpp fails to include styles for .symlink::before to match
>+   styles for .dir::before and .file::before
>+*/

I'd prefer either:
 /* foo
  * bar
  */
or
 /* foo
    bar */
for the comment style.

Otherwise, sr=smorgan

Comment 10

[philippe (part-time)]

Attachment: patch for checkin

Patch for checkin with the comment about alignment added and the long comment edited.

Carrying over stuart's sr+.

Comment 11

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

http://hg.mozilla.org/camino/rev/00771d3dce74