294800 - beautify FTP/File/Jar/Gopher dir listing (CSS, icons, sortable columns)

Status: RESOLVED FIXED

(Core :: Networking, enhancement, P4)

Description

[Nicolas VERITE]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; fr-FR; rv:1.7.7) Gecko/20050414 Firefox/0.10.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; fr-FR; rv:1.7.7) Gecko/20050414 Firefox/0.10.1

Directory contents are presented with default CSS and icons that come from ages
(maybe Mosaic's era ?), I think it should be time to make it look a bit more...
modern ? ;-)

Reproducible: Always

Steps to Reproduce:
Open a local or remote (FTP) directory.
Actual Results:  
I'm a bit scared... ;-)

Expected Results:  
Look beautiful... ;-)

Comment 1

[Gervase Markham [:gerv]]

This is an automated message, with ID "auto-resolve01".

This bug has had no comments for a long time. Statistically, we have found that
bug reports that have not been confirmed by a second user after three months are
highly unlikely to be the source of a fix to the code.

While your input is very important to us, our resources are limited and so we
are asking for your help in focussing our efforts. If you can still reproduce
this problem in the latest version of the product (see below for how to obtain a
copy) or, for feature requests, if it's not present in the latest version and
you still believe we should implement it, please visit the URL of this bug
(given at the top of this mail) and add a comment to that effect, giving more
reproduction information if you have it.

If it is not a problem any longer, you need take no action. If this bug is not
changed in any way in the next two weeks, it will be automatically resolved.
Thank you for your help in this matter.

The latest beta releases can be obtained from:
Firefox:     http://www.mozilla.org/projects/firefox/
Thunderbird: http://www.mozilla.org/products/thunderbird/releases/1.5beta1.html
Seamonkey:   http://www.mozilla.org/projects/seamonkey/

Comment 2

[Nicolas VERITE]

Firefox received new error pages in its 1.5 pre-versions, I think ftp:// and
file:/// pages should look consistent to them.

Comment 3

[Ryan Flint [:rflint] (ping via IRC for reviews)]

I think at the *very* least we should differentiate between file types and use
the old gopher icons packaged with Fx in resource://gre/res/html/
But I think we can come up with something a little better than that ;)

I'll take this for now and sketch out a rough concept of what I've got in mind
on w.m.o to see if this is actually wanted.

Comment 4

[Ryan Flint [:rflint] (ping via IRC for reviews)]

Attachment: Switch to an external stylesheet

Real simple change to make editing the style easier.

Comment 5

[Ryan Flint [:rflint] (ping via IRC for reviews)]

Attachment: indexed.css

Comment 6

[Zach van Schouwen]

Attachment: Re-styled page

This stylesheet (embedded in HTML for clarity) brings it pretty close to matching the error page... it still doesn't deal with the rather uncharming icons.

Comment 7

[Ryan Flint [:rflint] (ping via IRC for reviews)]

Oops, I've left this one sitting for a bit too long. Current work in progress is available in the URL field. Most of the page structure is pretty tentative and the current sorting script is there for demo purposes only.

Comment 8

[Michael Ventnor]

(In reply to comment #6)
> Created an attachment (id=228678) [edit]
> Re-styled page
> 
> This stylesheet (embedded in HTML for clarity) brings it pretty close to
> matching the error page... it still doesn't deal with the rather uncharming
> icons.
> 

That looks good, though maybe an A:hover{text-decoration: underline;} is in order?

Comment 9

[Zach van Schouwen]

Actually, I rather prefer Ryan's page, now what I've seen it.

May I propose a version with his terrific icons and text styling, but with more space between columns (and one added icon for the ".." link, and a couple minor changes in alignment), such as this:

http://www.columbia.edu/~zmv2102/firefox/ftp.html

Comment 10

[Nicolas VERITE]

I love this one...

But can't these pages be a little "AJAX-ified" ? I mean:
* the columns could be resizeable,
* one could click on columns header in order to re-order the content (classify by size, date, name, etc.),
* dates could be localized,
* etc.

Comment 11

[Ryan Flint [:rflint] (ping via IRC for reviews)]

Attachment: Images for /global/ftp in win/pinstripe

Comment 12

[Ryan Flint [:rflint] (ping via IRC for reviews)]

Attachment: Patch v1

Preliminary patch just so I can get a few eyes on this. This patch themes toolkit only.

Comment 14

[Jason Barnabe (np)]

Here's a user style that uses just CSS to pretty it up. http://userstyles.org/style/show/210

Comment 15

[Dão Gottwald [:dao]]

some notes:

  1. date and time stamps have to be right-aligned. Single-digit months
     need a leading zero.

  2. "Up to higher level directory" should stay outside of the table.

  3. 100 underlined strings in a column are disturbing. As mentioned in
     comment 8, underline should be applied on :hover.

Comment 16

[Dão Gottwald [:dao]]

my proposal:
http://design-noir.de/bugzilla/ftp/ftp.xhtml

Comment 17

[Dão Gottwald [:dao]]

(In reply to comment #16)
> http://design-noir.de/bugzilla/ftp/ftp.xhtml

As a result of my latest changes, this only works with trunk builds.


Ryan, are you still working on this bug?

Comment 18

[Ryan Flint [:rflint] (ping via IRC for reviews)]

(In reply to comment #17)
> 
> Ryan, are you still working on this bug?
> 

Yep, this fell off my radar after the feature freeze for 1.8. But I'm planning on picking it up again soon. I'll get a few of the designs I have left over cleaned up and posted somewhere in the next few days.

Comment 19

[Dão Gottwald [:dao]]

(In reply to comment #16)
> http://design-noir.de/bugzilla/ftp/ftp.xhtml

Added a netError-like alternate stylesheet.

Comment 20

[Robert Kaiser]

I still think the better way to go in this regard would be something in XUL, but this might be a good intermediate step...

Comment 21

[Mike Beltzner [:beltzner, not reading bugmail]]

Agreed; this seems like a good iterative improvement. We might have to reskin down the line for the sake of consistency, but that shouldn't block us getting the change in on trunk to see how people like it.

Comment 22

[Alfred Kayser]

See also bug 269017

Comment 23

[Dão Gottwald [:dao]]

(In reply to comment #22)
> See also bug 269017

What do you mean by "see also"? Will bug 269017 provide the infrastructure and this one the styling? Or do we have a duplicate?

Comment 24

[Alfred Kayser]

I would say this:
* Keep this one as the styling bug
* Keep the other one as the infrastructure 

Even if some styling is included in bug 269017, I am sure that it will need improvements once the infrastructure is there.

Comment 25

[Dão Gottwald [:dao]]

Attachment: proposed layout

Comment 26

[Alfred Kayser]

Attachment: Example of another way to style index output

Note the attachment from Dão looks very nice, but also adds lots of functionality (sorting, clicking on parts of the path, etc) which should be handled as separate items ('bugs' in bugzilla). 

I like the styling and icons though.

So, I would request to remove all javascript, and only use CSS (and corresponding images in chrome:skin to style the output (once bug 269017 is applied).

Comment 27

[Dão Gottwald [:dao]]

Attachment: proposed layout v2

This one uses "moz-icon" for files.

Comment 28

[Michael Ventnor]

(In reply to comment #27)
> Created an attachment (id=259884) [details]
> proposed layout v2
> 
> This one uses "moz-icon" for files.
> 

moz-icon isn't implemented on Linux. IIRC, nor Mac.

Comment 29

[Alfred Kayser]

Two requests:
1. Stick with the normal image formats: JPG, GIF or PNG
2. Make it skinnable: instead of inline style, include a 'chrome://global/skin/index.css' file.

Comment 30

[Christian :Biesinger (don't email me, ping me on IRC)]

moz-icon _is_ implemented on Linux as well as on Mac.

Comment 31

[Michael Ventnor]

(In reply to comment #30)
> moz-icon _is_ implemented on Linux as well as on Mac.
> 

Must be on trunk then. I'm using 2.0 under Lin and I get a protocol error. :)
Does moz-icon work under KDE, as well as GNOME?

Comment 32

[Christian :Biesinger (don't email me, ping me on IRC)]

> Does moz-icon work under KDE, as well as GNOME?

If you have the Gnome libraries installed (libgnomeui, if I recall this correctly)

Comment 33

[Dão Gottwald [:dao]]

Attachment: proposed layout v3

* unbolded heading and table headers
* better sorting arrows, as per http://weblogs.mozillazine.org/gerv/archives/2007/04/column_sorting_usability_resul.html

Comment 34

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 262491 [details]
proposed layout v3

ui-r+  with a couple of nits:

1. Left-justify the column headers instead of centering them.

2. The FTP favicon should be different from the file-folder icon, maybe something more like "computer_go" from the popular Silk set at http://www.famfamfam.com/lab/icons/silk/previews/index_abc.png, but with the arrow pointing down?

3. Less space between the date and the time in the "Last Modified" column; maybe about half of what's there now.

None of those should block and can all be addressed in follow-up bugs if you want.

Comment 35

[Dão Gottwald [:dao]]

Attachment: layout with ui-review comments addressed

Comment 36

[Marco Bonardo [:mak]]

how does it show the path if it is very very long? does it wrap or is it replaced with ...?

how does it show filename if it's very very long?

Could be useful to separate host from path to better show that?

Why is clicking allowed up to the center of the page? it should be clickable only on icon/name or on full row (also size and lastmod)

maybe a very light grey row selection (with radius borders) could help identify the current selected element (it's already good with color changing, but a background row selection could do more for those with eye problems)

Also imho headers could receive some more "love" and styling

This is a very good work, and i like it very much!

Comment 37

[Dão Gottwald [:dao]]

Attachment: new files

Comment 38

[Dão Gottwald [:dao]]

Attachment: WIP patch

I have absolutely no C++ experience. Would be great if somebody could have a first look at this.
It's also untested so far.

Comment 39

[Dão Gottwald [:dao]]

Attachment: WIP patch

typo removed

Comment 40

[Dão Gottwald [:dao]]

(In reply to comment #36)
> how does it show the path if it is very very long? does it wrap or is it
> replaced with ...?

It wraps at "/".

> how does it show filename if it's very very long?

It wraps at "-". Otherwise it stretches and overflows the table.

Comment 41

[Michael Ventnor]

Dao, I've been working on this myself last night. May I post my patch if you don't mind? I've also refined some things such as how long filenames are dealt with.

Comment 42

[Dão Gottwald [:dao]]

I'm building right now -- let me see if I can handle this by myself. I'll definitely come back to your offer if I'm stuck. Otherwise, there can be as much follow-up bugs as needed (e.g. to better deal with long file names).

Then again, as I said, I've got no C++ experience. If you think my patch has serious issues, go ahead and show what you got.

Comment 43

[Michael Ventnor]

I don't think the favicon thing is going to work. I've never been able to get a chrome:// favicon from any webpage including generated ones like the RSS icon.

Comment 44

[Dão Gottwald [:dao]]

Attachment: patch v1

Comment 45

[Dão Gottwald [:dao]]

Attachment: patch v1

um, that was an old patch.

Comment 47

[Michael Ventnor]

Dao, while you're doing this, you can take the opportunity for code cleanup :)

You can change ConvertNonAsciiToNCR to AppendNonAsciiToNCR. Remove the out.Truncate() line in the function. That way you can do 
AppendNonAsciiToNCR(title, buffer);
and get rid of the need of strNCR completely. Look at the latest patch in bug 269017 to see this.

Replace ns(U)Int64 with PR(U)Int64 because IIRC nsInt64 is deprecated. After that you can get rid of the include nsint64.h.

If you don't understand any of this let me know.

Comment 48

[Dão Gottwald [:dao]]

Attachment: new files (sortable.js and stylesheets updated)

Comment 49

[Dão Gottwald [:dao]]

Attachment: patch v2

comment 47 addressed

Comment 50

[Boris Zbarsky [:bzbarsky]]

I'm not sure I'll be able to review this in a reasonable timeframe, given how much more complex than bug 269017 this stuff is and the timeframe I had for that.

That said, I have several issues with the patch:

1)  It's linking to a chrome script from untrusted content.  This is something we've been planning to disallow for some time now, so it's fragile.  I'd rather not introduce regression-prone code like that.  Do you foresee anyone reusing that JS file?  If not, just put it inline here.  If you do, please file a followup bug depending on the scripts-in-chrome bug to fix this code once we close this security hole.

2)  I don't like the isSchemeRemote thing.  What exactly is that trying to accomplish?  Almost no matter what it is, there is a beter way than by doing string checks on the scheme.

What with the sorting, headers, etc, this is looking more and more like the XUL version of this, for what it's worth.  We should really just remove one or the other.

Comment 51

[Dão Gottwald [:dao]]

(In reply to comment #50)
> 1)  It's linking to a chrome script from untrusted content.  This is something
> we've been planning to disallow for some time now, so it's fragile.  I'd rather
> not introduce regression-prone code like that.  Do you foresee anyone reusing
> that JS file?  If not, just put it inline here.

I thought about reusing, but I don't foresee it. I suppose it won't really happen, so I'll inline the script.

> 2)  I don't like the isSchemeRemote thing.  What exactly is that trying to
> accomplish?  Almost no matter what it is, there is a beter way than by doing
> string checks on the scheme.

It's all about the favicon. Beltzner wants a special one for FTP (which I'm also using for Gopher). "isSchemeRemote" is not accurate since JAR can be both remote and local. Yet I think it's okay to just use the "open folder" icon for JAR, so I'll probably end up giving the variable a different name. Alternatively, somebody could tell me about how to implement the better way.

> What with the sorting, headers, etc, this is looking more and more like the XUL
> version of this, for what it's worth.  We should really just remove one or the
> other.

I don't know anything about a XUL version.

Comment 52

[Steffen Wilberg]

(In reply to comment #51)
> I don't know anything about a XUL version.
Firefox doesn't ship directory.xul. Try SeaMonkey, set the pref network.dir.format to 3, and open any ftp page.

http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/xpfe/components/directory/nsDirectoryViewer.cpp&rev=1.129#1389
http://mxr.mozilla.org/seamonkey/source/suite/common/directory/directory.xul
http://mxr.mozilla.org/seamonkey/source/suite/common/directory/directory.js
http://mxr.mozilla.org/seamonkey/source/themes/classic/communicator/directory/directory.css

Comment 53

[Dão Gottwald [:dao]]

Attachment: patch v3

comments addressed, sorting and headers for gopher disabled

Comment 54

[Dão Gottwald [:dao]]

Attachment: theme files

Comment 55

[Dão Gottwald [:dao]]

(In reply to comment #52)
> (In reply to comment #51)
> > I don't know anything about a XUL version.
> Firefox doesn't ship directory.xul. Try SeaMonkey, set the pref
> network.dir.format to 3, and open any ftp page.

That's a tree as known from file browsers -- interesting, but probably a bit overwhelming for Firefox. I think we're on track here, as the current HTML listing is closer to what got ui-review+ than the current XUL listing.

Comment 56

[Dão Gottwald [:dao]]

What the current patch doesn't do is to output

  <h1>Index of <a href="ftp://ftp.mozilla.org/">ftp.mozilla.org</a>/
  <a href="ftp://ftp.mozilla.org/pub/">pub</a>/mozilla.org/</h1>

instead of

  <h1>Index of ftp://ftp.mozilla.org/pub/mozilla.org/</h1>

I'd like to leave that to a follow-up bug.

Comment 57

[Dão Gottwald [:dao]]

Here's an open issue: The current patch groups files and folders when sorting by name. Should symlinks be part of one of these groups, or should they be a separate (the first, second or last?) one?

Folders with symbolic links:
ftp://ftp.mozilla.org/pub/mozilla.org/firefox/releases/
ftp://ftp.mozilla.org/pub/mozilla.org/webtools

Comment 58

[Boris Zbarsky [:bzbarsky]]

Can we tell whether the symlinks are to files or directories?  Ideally, symlinks would be transparent to the user and look like whatever they link to.

Comment 59

[Robert Kaiser]

(In reply to comment #57)
> Should symlinks be part of one of these groups, or should they be a
> separate (the first, second or last?) one?

From what I see elsewhere the usual thing is to order symlinks to directories like other directories and symlinks to files like normal files, often making them italic in style though.
I think we should do that as well (if we can detect if we're pointing to a directory or a normal file).

Comment 60

[Dão Gottwald [:dao]]

(In reply to comment #58)
> Can we tell whether the symlinks are to files or directories?

I don't think so, based on view-source:ftp://ftp.mozilla.org/pub/mozilla.org/webtools

Comment 61

[Dão Gottwald [:dao]]

Currently, Firefox displays folder icons for all symlinks. With patch v3 applied, it displays no icon but adds symlinks to the folder group.

Comment 62

[Boris Zbarsky [:bzbarsky]]

So is this the point when I unsuccessfully try to argue that trying to sort files and directories separately just leads to confusion and annoyance, in addition to clear issues like the symlink one, and we shouldn't be doing it to start with?  ;)

Comment 63

[Dão Gottwald [:dao]]

One reason why I started to work on this is that mixing directories and files confused and annoyed me. You're hardly ever looking for "something" but rather exclusively a file or a directory that starts with a letter.

Grouping symlinks together with directories wouldn't be a regression at least, as using folder icons for symlinks is much more weird. But maybe it's more sensible to put symlinks as a separate group between directories and files.

Comment 64

[[:Aleksej]]

Bug 325668: not sure, blocks or depends.

Comment 65

[Dão Gottwald [:dao]]

(In reply to comment #63)
> But maybe it's more
> sensible to put symlinks as a separate group between directories and files.

On the other hand, there's probably a reason why someone created the links. At least for the two ftp.mozilla.org examples from above, they point to what the user is most likely looking for, so maybe they should be on the top?

Comment 66

[Michael Ventnor]

Maybe you should group symlinks with the folders, but give them a separate icon? Something like a folder with a little arrow symbol in the bottom-right corner.

Comment 67

[Dão Gottwald [:dao]]

The thing is, symlinks can point to files. A folder icon doesn't make sense in that case. (The current implementation even adds alt="Directory: "!)
But that's not the problem -- IMHO using no icon is just fine.

Comment 68

[Brendan Eich [:brendan]]

If you are talking about symlinks in the Unix sense (originated in BSD Unix a while ago; Mac OS X is Unix in this and many other ways), they ought to be transparent (that is, followed) most of the time. ls -l is a counterexample. Is this really an ls -l analogue?

/be

Comment 69

[Dão Gottwald [:dao]]

(In reply to comment #68)
> If you are talking about symlinks in the Unix sense (originated in BSD Unix a
> while ago; Mac OS X is Unix in this and many other ways), they ought to be
> transparent (that is, followed) most of the time. ls -l is a counterexample. Is
> this really an ls -l analogue?

More in the Unix sense, as we don't know where it's pointing to. We don't even know if the target is a directory or a file, which is a problem for the user interface.

Comment 70

[Dan Mosedale (:dmosedale, :dmose)]

Setting nsILocalFile.followLinks to true should allow us to find this out.

Comment 71

[Dão Gottwald [:dao]]

"As of Mozilla 1.7, this attribute is ignored on UNIX systems", according to http://developer.mozilla.org/en/docs/nsILocalFile:followLinks. But for FTP it wouldn't help anyway.

Comment 72

[Dão Gottwald [:dao]]

I think the stylesheet needs some tweaking. The scrollbar is unusable on ftp://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/.

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a7pre) Gecko/2007072021 Minefield/3.0a7pre

Comment 73

[Dão Gottwald [:dao]]

(In reply to comment #72)
> I think the stylesheet needs some tweaking. The scrollbar is unusable on
> ftp://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/.

Looks like -moz-border-radius has serious issues. Scrolling is perfectly smooth if I remove that single property.

Comment 74

[Dão Gottwald [:dao]]

Attachment: patch v4

- symlinks to the top
- long names truncated
- "Show hidden objects" checkbox for file:// added which affects files and directories starting with a dot (to make browsing your home directory on Linux for instance less cumbersome.) The checkbox appears (only if there are such objects) right aligned next to the "Up to higher level directory" link. Is that trivial enough or does it require a new ui-review?

Comment 75

[Dão Gottwald [:dao]]

Attachment: theme files

border-radius removed temporarily (bug 389030)

Comment 76

[Dão Gottwald [:dao]]

Attachment: theme files

added 
    white-space: pre;
    font-family: monospace;
for gopher

Comment 77

[Dão Gottwald [:dao]]

Attachment: patch v4 synced with trunnk

bz, will you have cycles for this?

Comment 78

[Boris Zbarsky [:bzbarsky]]

At some point yes, but it might be a few weeks...

Comment 79

[Dão Gottwald [:dao]]

Before M8 would be good, but shortly afterwards should be okay, too, I suppose.

Comment 80

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 274899 [details] [diff] [review]
patch v4 synced with trunnk

>Index: netwerk/streamconv/converters/nsIndexedToHTML.cpp
>+    buffer.AppendLiteral("<!DOCTYPE html>\n"
>+                         "<html xmlns=\"http://www.w3.org/1999/xhtml\">\n<head>\n"

So are we talking HTML or XML here?  If the former, what's up with the namespace?  If the latter, what's up with the missing <?xml decl and the meta tag?  Looks like we set the type on the channel to text/html, so we should remove the xmlns.

>+                         "<link href=\"chrome://global/skin/dirListing/dirListing.css\" rel=\"stylesheet\" type=\"text/css\" />\n"
>+                         "<meta http-equiv=\"content-type\" content=\"text/html; charset=");

The <meta> needs to go before the <link>.

I didn't review the JS in your script, for what it's worth.

How usable is this UI if the user has JS disabled?  Does it default to a sane behavior, at least?  At first blush, in that situation dotfiles would be invisible on Linux, as would the UI to toggle that behavior?  I'd rather we defaulted the other way, frankly.  That is, have stuff shown by default and use script to hide it.

> nsIndexedToHTML::OnIndexAvailable(nsIRequest *aRequest,

>+        pushBuffer.AppendLiteral(" hidden-object=\"true\"");

Why not |class="hidden-object"|?  That would make the CSS selectors in use here much faster...

Is there a reason that basic functionality (e.g. hiding the hidden items) lives in the theme?  It would make a lot more sense to have a stylesheet that lives in this code for that stuff (so you don't break every single non-Firefox Gecko app with this checkin) and have the theme handle the look only, at best.  Possibly not even that; for example Epiphany doesn't exactly have a theme to style this stuff with.  No matter what, all style rules relevant to actual behavior need to live in the core code.  I'm happy to also load a skin sheet on top of that so themes can override as desired.  Similarly, the icons for up and folder need to live in resource:/ like the currently used file type icons do, not in the skin.

The use of the :first-child and :last-child rules in the stylesheet is somewhat fragile, but maybe we'll fix the bugs involved for 1.9...

Those rules could use documentation as to why you're setting widths to 0, in any case.

I'm not sure what you think setting the width on a ::before is expected to do; it does nothing unless you've also styled it to not be display:inline.  Very similar for height.

Is there a reason all the text is being styled with "font-size: 110%"?

Why is there a "font-size: 100%" on the table?  That's a no-op, no?

>+    pushBuffer.Append(escaped);

It's worth adding a comment before this that the sort key is one of 0, 1, or 2 followed by the filename.  Make it a lot clearer what's going on here.

>+    // Truncate long names to not stretch the table

Er...  "why?".  Personally I'd prefer to see the entirety of filenames.  In any case, since you're appending the "long" version directly as text already, I'm not sure what the point of this code is.

>+    //XXX this should be left to the stylesheet (bug 312156)

Probably worth filing an explicit bug on fixing this in this code, dependent on bug 312156.

>+        if (!(NS_SUCCEEDED(uri->SchemeIs("gopher", &isSchemeGopher)) && isSchemeGopher)) {

Why only for non-gopher?  At the very least, document in a nice comment.

>+            description.Truncate(30);

That's no good.  You might be truncating in the middle of a surrogate pair.  You need to check for that.

Of course you could also be truncating between a combining char and the thing it combines with, etc, but that's harder to detect.  Think truncating after a Hebrew consonant but before the following vowel: it doesn't actually shorten the displayed string.  Please file a followup bug here on the combining char issue (with an XXX comment pointing to the bug)?  Intl doesn't seem to expose functions for dealing with those...

>+            escapedShort.Assign(nsEscapeHTML2(description.get(), description.Length()));

You just leaked the escaped text. You want to use Adopt(), not Assign().

>+        if (PRUint64(size) != PRUint64(LL_MAXUINT)) {

Just LL_MAXUINT without the cast, please.

> void nsIndexedToHTML::FormatSizeString(PRInt64 inSize, nsString& outSizeString)
>+    PRInt64 size(inSize);

Why do you need |size| at all?  Just use inSize directly.

Comment 81

[Dão Gottwald [:dao]]

Attachment: mockup for basic unthemed stylesheet

IMHO this is just fine without extra icons. Objections?

Comment 82

[Dão Gottwald [:dao]]

Attachment: unthemed mockup, the right one

Comment 83

[Boris Zbarsky [:bzbarsky]]

What do you mean by "without extra icons"?

Comment 84

[Dão Gottwald [:dao]]

No up icon, no folder icons. Probably also no favicon, if we end up using the theme version.

Comment 85

[Boris Zbarsky [:bzbarsky]]

I'm fine with no "up" icon and no favicon, I guess.  But no folder icons is a functional regression, which we should avoid.  Especially since we have a perfectly good folder icon to use.

Comment 86

[Dão Gottwald [:dao]]

My thought was that you can spot folders easily because the missing icon and size differentiates them from files. Also, the script will group them by default.

But of course we can add the new Winstripe icon, if that's wanted.

Comment 87

[Boris Zbarsky [:bzbarsky]]

I'd prefer having a folder icon by default, yes.

Comment 88

[Dão Gottwald [:dao]]

Attachment: unthemed with folder icons

Comment 89

[Dão Gottwald [:dao]]

Attachment: patch v5

Note that this patch is not tested. I'm not sure if I'll be able to do this in the near future.

(In reply to comment #80)
> So are we talking HTML or XML here?  If the former, what's up with the
> namespace?  If the latter, what's up with the missing <?xml decl and the meta
> tag?  Looks like we set the type on the channel to text/html, so we should
> remove the xmlns.

fixed

> >+                         "<link href=\"chrome://global/skin/dirListing/dirListing.css\" rel=\"stylesheet\" type=\"text/css\" />\n"
> >+                         "<meta http-equiv=\"content-type\" content=\"text/html; charset=");
> 
> The <meta> needs to go before the <link>.

fixed

> How usable is this UI if the user has JS disabled?  Does it default to a sane
> behavior, at least?  At first blush, in that situation dotfiles would be
> invisible on Linux, as would the UI to toggle that behavior?  I'd rather we
> defaulted the other way, frankly.  That is, have stuff shown by default and use
> script to hide it.

All files are now shown by default.

> > nsIndexedToHTML::OnIndexAvailable(nsIRequest *aRequest,
> 
> >+        pushBuffer.AppendLiteral(" hidden-object=\"true\"");
> 
> Why not |class="hidden-object"|?  That would make the CSS selectors in use here
> much faster...

fixed

> Is there a reason that basic functionality (e.g. hiding the hidden items) lives
> in the theme?  It would make a lot more sense to have a stylesheet that lives
> in this code for that stuff (so you don't break every single non-Firefox Gecko
> app with this checkin) and have the theme handle the look only, at best. 
> Possibly not even that; for example Epiphany doesn't exactly have a theme to
> style this stuff with.  No matter what, all style rules relevant to actual
> behavior need to live in the core code.  I'm happy to also load a skin sheet on
> top of that so themes can override as desired.  Similarly, the icons for up and
> folder need to live in resource:/ like the currently used file type icons do,
> not in the skin.

Inline stylesheet with basic functionality added which also embeds resource://gre/res/html/folder.png for folders (attachment 275877 [details]). Pinstripe overrides that with its own image.

> Those rules could use documentation as to why you're setting widths to 0, in
> any case.

comment added

> I'm not sure what you think setting the width on a ::before is expected to do;
> it does nothing unless you've also styled it to not be display:inline.  Very
> similar for height.

width and height removed. (I was using display:inline-block once, but that's not needed anymore.)

> Is there a reason all the text is being styled with "font-size: 110%"?

Well, I think the message-box font is too small otherwise.

> Why is there a "font-size: 100%" on the table?  That's a no-op, no?

removed

> It's worth adding a comment before this that the sort key is one of 0, 1, or 2
> followed by the filename.  Make it a lot clearer what's going on here.

comment added

> >+    // Truncate long names to not stretch the table
> 
> Er...  "why?".  Personally I'd prefer to see the entirety of filenames.  In any
> case, since you're appending the "long" version directly as text already, I'm
> not sure what the point of this code is.

Because long file names would stretch the column and the whole table otherwise, which is quite a bad user experience. I'm exposing the long version in the title attribute.

> >+    //XXX this should be left to the stylesheet (bug 312156)
> 
> Probably worth filing an explicit bug on fixing this in this code, dependent on
> bug 312156.

bug filed and comment updated

> >+        if (!(NS_SUCCEEDED(uri->SchemeIs("gopher", &isSchemeGopher)) && isSchemeGopher)) {
> 
> Why only for non-gopher?  At the very least, document in a nice comment.

comment added

> >+            description.Truncate(30);
> 
> That's no good.  You might be truncating in the middle of a surrogate pair. 
> You need to check for that.

fixed

> Of course you could also be truncating between a combining char and the thing
> it combines with, etc, but that's harder to detect.  Think truncating after a
> Hebrew consonant but before the following vowel: it doesn't actually shorten
> the displayed string.  Please file a followup bug here on the combining char
> issue (with an XXX comment pointing to the bug)?  Intl doesn't seem to expose
> functions for dealing with those...

bug filed and comment added

> >+            escapedShort.Assign(nsEscapeHTML2(description.get(), description.Length()));
> 
> You just leaked the escaped text. You want to use Adopt(), not Assign().
> 
> >+        if (PRUint64(size) != PRUint64(LL_MAXUINT)) {
> 
> Just LL_MAXUINT without the cast, please.
> 
> > void nsIndexedToHTML::FormatSizeString(PRInt64 inSize, nsString& outSizeString)
> >+    PRInt64 size(inSize);
> 
> Why do you need |size| at all?  Just use inSize directly.

fixed

Comment 90

[Dão Gottwald [:dao]]

Attachment: images and css files

Comment 91

[Boris Zbarsky [:bzbarsky]]

Is there a reason not to keep using resource://gre/res/html/gopher-menu.gif ?  I agree the new graphic looks nicer, I guess...

> Well, I think the message-box font is too small otherwise

Why is that font being used to start with?  Why not use the user's default monospace font, as the in-Gecko default does?

Also note that your message-box font may be nothing like someone else's...

Comment 92

[Dão Gottwald [:dao]]

(In reply to comment #91)
> Is there a reason not to keep using resource://gre/res/html/gopher-menu.gif ? 
> I agree the new graphic looks nicer, I guess...

It's bigger, which means that the rows will take more space. I can attach a mockup for comparison.

> Why is that font being used to start with?  Why not use the user's default
> monospace font, as the in-Gecko default does?

Because we're trying to match error pages. Even if I use sans-serif, that produces a significantly different look. I also see no good reason to stick with monospace.

> Also note that your message-box font may be nothing like someone else's...

Sure, but 10% more shouldn't be harmful even if the user's system font is big. Note that netError.css basically does the same.

Comment 93

[Dão Gottwald [:dao]]

Attachment: unthemed version with gopher-menu.gif

Comment 94

[Boris Zbarsky [:bzbarsky]]

> Because we're trying to match error pages.

Er.... WHY?  An error page shows a short message and replaces an alert.  Using message-box there makes sense. A file listing shows a whole long list of files.  There's no way message-box is appropriate there.

I agree there's no reason to stick with monospace, really... so why is your core patch using it?  Just use the user's default font.  As in, don't set font at all.

Comment 95

[Dão Gottwald [:dao]]

(In reply to comment #94)
> > Because we're trying to match error pages.
> 
> Er.... WHY?  An error page shows a short message and replaces an alert.  Using
> message-box there makes sense. A file listing shows a whole long list of files.
>  There's no way message-box is appropriate there.

message-box is the default font for xul windows and used in all kinds of places. I'm pretty sure SeaMonkey's listing uses that font, too.

> I agree there's no reason to stick with monospace, really... so why is your
> core patch using it?  Just use the user's default font.  As in, don't set font
> at all.

For Firefox, too? I don't think we want that. The default font on WinXP is Times New Roman.

Comment 96

[Boris Zbarsky [:bzbarsky]]

> message-box is the default font for xul windows

Which don't show significant amounts of text.

> I'm pretty sure SeaMonkey's listing uses that font, too.

Seamonkey currently uses the same listing as Firefox; both use the default monospace font.

> The default font on WinXP is Times New Roman.

what about using the default sans-serif font?

Comment 97

[Boris Zbarsky [:bzbarsky]]

Here's a silly question: What does IE do?

I'd also love to hear from beltzner on this.  But using a system font and then messing with it because it's not actually what you want just seems severely wrong to me.

Comment 98

[Alfred Kayser]

IE jumps to the Windows Explorer when 'browsing' the files, and strangely then uses the system font for icons (see desktop preferences) for the file listing (and the three). 

Using 'message-box' is fairly safe as all of the (default) theme of Firefox uses that also.

Comment 99

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #97)
> Here's a silly question: What does IE do?

They do nothing. Default monospace font (looks pretty much like Firefox 2) unless the server sends something prettier.

> I'd also love to hear from beltzner on this.  But using a system font and then
> messing with it because it's not actually what you want just seems severely
> wrong to me.

Well, I ui-r+'d things a while back (the only thing that doesn't look absolutely right is the spacing for the "Last Updated" column, but we can deal with that in a follow-up, as I'm not sure what the best thing to do there is). 

Monospace fonts are harder to read, and if we can make a prettier design at low cost, we should. I agree with Dao that we should be matching against the fonts (but perhaps not the sizes) used in the error dialogs, not because error dialogs are related to this, but because then we're using a consistent font across content-area UI generated by Firefox.

I don't care if you use the same CSS class, or create a new one (which simply mimics the other one), but the same font face should be used. (Times New Roman would also look pretty fugly, IMO.)

Comment 100

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #98)
> IE jumps to the Windows Explorer when 'browsing' the files, and strangely then
> uses the system font for icons (see desktop preferences) for the file listing
> (and the three). 

Yeah, I should have mentioned that I was forcing myself to do ftp-over-http to get the file listing. Handing FTP off to the file server ends up sucking, IMO.

Comment 101

[Boris Zbarsky [:bzbarsky]]

OK.  In the end, the font doesn't really matter that much to me; I just think it's odd to set the system font (which sets a combination of font face, weight, etc, etc) then override parts of it...  But whatever.

Comment 102

[Dão Gottwald [:dao]]

So, if beltzner has nothing against it, I'll use message-box without changing the size. This shouldn't really be a problem; my system is an extreme case (1680 x 1050 @ 15,4").

(In reply to comment #96)
> > message-box is the default font for xul windows
> 
> Which don't show significant amounts of text.

Thunderbird would be a counter-example.

> > I'm pretty sure SeaMonkey's listing uses that font, too.
> 
> Seamonkey currently uses the same listing as Firefox; both use the default
> monospace font.

I meant the XUL version.

Comment 103

[Dão Gottwald [:dao]]

Attachment: patch v6

Comment 104

[Dão Gottwald [:dao]]

Comment on attachment 276052 [details] [diff] [review]
patch v6

The Gecko stylesheet uses font-family:sans-serif now.

There was also a minor escaping bug in the previous patch. This one is tested again.

Comment 105

[Dão Gottwald [:dao]]

Attachment: patch v6

correct encoding this time

Comment 106

[Dão Gottwald [:dao]]

Attachment: images and css files

The table body's font size is now at 100% instead of 110%.

Comment 107

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 276053 [details] [diff] [review]
patch v6

Looks good.  Thank you for being patient with me!

Comment 108

[Dão Gottwald [:dao]]

I suppose the toolkit stuff needs a separate review?

Comment 109

[Boris Zbarsky [:bzbarsky]]

Yeah...

Comment 110

[Asaf Romano (gone)]

Comment on attachment 276055 [details]
images and css files

body {
  border: 1px solid THreeDShadow;
  /*XXX performs badly (bug 389030)
  -moz-border-radius: 10px;*/

we already do this for feeds IIRC, so let's leave it in.

r=mano otherwise.

Comment 111

[Dão Gottwald [:dao]]

Attachment: images and css files (review comment addressed)

as per comment 110

Comment 112

[Jeff Walden [:Waldo]]

Checked in -- for the eye-pleasing, post-1995 win!

Comment 113

[Daniel Luz]

I didn't even know of this bug, but I'm quite pleased with the result. :)

Just a nit: the sorting headers are not keyboard-accessible. Is a new bug needed for that?

Comment 114

[Boris Zbarsky [:bzbarsky]]

Yes, please.  Should have caught that...  :(

Comment 115

[Biju]

(In reply to comment #56)
> What the current patch doesn't do is to output
> 
>   <h1>Index of <a href="ftp://ftp.mozilla.org/">ftp.mozilla.org</a>/
>   <a href="ftp://ftp.mozilla.org/pub/">pub</a>/mozilla.org/</h1>
> 
> instead of
> 
>   <h1>Index of ftp://ftp.mozilla.org/pub/mozilla.org/</h1>
> 
> I'd like to leave that to a follow-up bug.

---------> created bug 392759

(In reply to comment #113)
> I didn't even know of this bug, but I'm quite pleased with the result. :)
> 
> Just a nit: the sorting headers are not keyboard-accessible. Is a new bug
> needed for that?

---------> created Bug 392760

Comment 116

[Boris Zbarsky [:bzbarsky]]

So looking at this in practice, why exactly are we using light gray text on a white background?  It's pretty unreadable over here.

Comment 117

[Boris Zbarsky [:bzbarsky]]

A related issue:  In practice, over here (1600x1200 resolution on a 20-inch monitor), the file type icons are just too small to effectively tell them apart given the 0.6 opacity setting.  Again, why can't we just show the icons that the user expects instead of showing very washed-out ones?  As things stand, the icons just introduce visual clutter and induce eye-strain trying to make them out.  Even at 100% opacity the size is a problem, but at least then there's a chance...  Ideally we'd use bigger icons and all.

Then again, the smallest icon size that seems to be reasonably identifiable over here is 24px, with 32px being much preferable...  I'm not sure we want to go that high, but if we insist on using icons maybe we should.

Comment 118

[Boris Zbarsky [:bzbarsky]]

Filed bug 392802 and bug 392803 on my UI issues.

Comment 119

[Boris Zbarsky [:bzbarsky]]

Bug 392807 filed on the performance problems I ran into...

Comment 120

[Tim Babych]

Attachment: ftp listing without icons

As of Trunk build 31-10-2007, tested on fresh profile the icons are not shown.

Comment 121

[Tim Babych]

filed bug 402742 on icons.

Comment 122

[Biju]

* unknown "file-type" should have an icon
* SYMBOLIC-LINK "file-type" should have an icon

see ftp://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/