Closed Bug 294800 Opened 15 years ago Closed 13 years ago

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

Categories

(Core :: Networking, enhancement, P4)

enhancement

Tracking

()

RESOLVED FIXED
mozilla1.9alpha8

People

(Reporter: nicolas.verite, Assigned: dao)

References

(Depends on 4 open bugs)

Details

(Keywords: relnote)

Attachments

(4 files, 30 obsolete files)

17.20 KB, application/xhtml+xml
Details
44.47 KB, patch
bzbarsky
: review+
bzbarsky
: superreview+
damons
: approval1.9+
Details | Diff | Splinter Review
10.57 KB, application/zip
Details
44.03 KB, image/png
Details
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... ;-)
Severity: trivial → enhancement
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/
Firefox received new error pages in its 1.5 pre-versions, I think ftp:// and
file:/// pages should look consistent to them.
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.


Assignee: nobody → rflint
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Priority: -- → P4
Attached patch Switch to an external stylesheet (obsolete) — Splinter Review
Real simple change to make editing the style easier.
Attached file Re-styled page (obsolete) —
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.
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.
Attachment #204415 - Attachment is obsolete: true
Attachment #204416 - Attachment is obsolete: true
(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?

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
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.
Attached patch Patch v1 (obsolete) — Splinter Review
Preliminary patch just so I can get a few eyes on this. This patch themes toolkit only.
Duplicate of this bug: 368857
Here's a user style that uses just CSS to pretty it up. http://userstyles.org/style/show/210
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.
(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?
(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.
(In reply to comment #16)
> http://design-noir.de/bugzilla/ftp/ftp.xhtml

Added a netError-like alternate stylesheet.
I still think the better way to go in this regard would be something in XUL, but this might be a good intermediate step...
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.
See also bug 269017
(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?
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.
Attached file proposed layout (obsolete) —
Attachment #255772 - Flags: ui-review?(beltzner)
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).
Attached file proposed layout v2 (obsolete) —
This one uses "moz-icon" for files.
Attachment #255772 - Attachment is obsolete: true
Attachment #259884 - Flags: ui-review?(beltzner)
Attachment #255772 - Flags: ui-review?(beltzner)
(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.
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.
moz-icon _is_ implemented on Linux as well as on Mac.
(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?
> Does moz-icon work under KDE, as well as GNOME?

If you have the Gnome libraries installed (libgnomeui, if I recall this correctly)
Attached file proposed layout v3 (obsolete) —
* unbolded heading and table headers
* better sorting arrows, as per http://weblogs.mozillazine.org/gerv/archives/2007/04/column_sorting_usability_resul.html
Attachment #259884 - Attachment is obsolete: true
Attachment #262491 - Flags: ui-review?(beltzner)
Attachment #259884 - Flags: ui-review?(beltzner)
Assignee: ryan → dao
Status: ASSIGNED → NEW
Version: unspecified → Trunk
Depends on: 269017
Status: NEW → ASSIGNED
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.
Attachment #262491 - Flags: ui-review?(beltzner) → ui-review+
Attachment #229939 - Attachment is obsolete: true
Attachment #229940 - Attachment is obsolete: true
Attachment #255940 - Attachment is obsolete: true
Attachment #262491 - Attachment is obsolete: true
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!
Attached file new files (obsolete) —
Attached patch WIP patch (obsolete) — Splinter Review
I have absolutely no C++ experience. Would be great if somebody could have a first look at this.
It's also untested so far.
Attached patch WIP patch (obsolete) — Splinter Review
typo removed
Attachment #272700 - Attachment is obsolete: true
(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.
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.
Attachment #272701 - Attachment is obsolete: true
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.
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.
Depends on: 388553
Attached patch patch v1 (obsolete) — Splinter Review
Attachment #272698 - Attachment description: new files (WIP) → new files
Attached patch patch v1 (obsolete) — Splinter Review
um, that was an old patch.
Attachment #272754 - Attachment is obsolete: true
Attachment #272757 - Flags: review?(bzbarsky)
Duplicate of this bug: 269017
No longer depends on: 269017
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.
Attachment #272698 - Attachment is obsolete: true
Attachment #272759 - Flags: review?(bzbarsky)
Attached patch patch v2 (obsolete) — Splinter Review
comment 47 addressed
Attachment #272757 - Attachment is obsolete: true
Attachment #272762 - Flags: review?(bzbarsky)
Attachment #272757 - Flags: review?(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.
(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.
Attachment #272759 - Attachment is obsolete: true
Attachment #272759 - Flags: review?(bzbarsky)
Attachment #272762 - Attachment is obsolete: true
Attachment #272762 - Flags: review?(bzbarsky)
Attached patch patch v3 (obsolete) — Splinter Review
comments addressed, sorting and headers for gopher disabled
Attachment #272880 - Flags: review?(bzbarsky)
Attached file theme files (obsolete) —
Attachment #272881 - Flags: review?(bzbarsky)
Attachment #272881 - Attachment is patch: false
Attachment #272881 - Attachment mime type: text/plain → application/zip
(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.
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.
Summary: beautify default ftp:// and file:/// CSS and icons → beautify FTP/File/Jar/Gopher dir listing (CSS, icons, sortable columns)
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
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.
(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).
(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
Currently, Firefox displays folder icons for all symlinks. With patch v3 applied, it displays no icon but adds symlinks to the folder group.
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?  ;)
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.
Bug 325668: not sure, blocks or depends.
Blocks: 325668
(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?
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.
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.
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
(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.
Setting nsILocalFile.followLinks to true should allow us to find this out.
"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.
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
(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.
Depends on: 389030
Attached patch patch v4 (obsolete) — Splinter Review
- 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?
Attachment #272880 - Attachment is obsolete: true
Attachment #273277 - Flags: review?(bzbarsky)
Attachment #272880 - Flags: review?(bzbarsky)
Attached file theme files (obsolete) —
border-radius removed temporarily (bug 389030)
Attachment #272881 - Attachment is obsolete: true
Attachment #272881 - Flags: review?(bzbarsky)
Attached file theme files (obsolete) —
added 
    white-space: pre;
    font-family: monospace;
for gopher
Attachment #273278 - Attachment is obsolete: true
Attached patch patch v4 synced with trunnk (obsolete) — Splinter Review
bz, will you have cycles for this?
Attachment #273277 - Attachment is obsolete: true
Attachment #274899 - Flags: review?(bzbarsky)
Attachment #273277 - Flags: review?(bzbarsky)
At some point yes, but it might be a few weeks...
Before M8 would be good, but shortly afterwards should be okay, too, I suppose.
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.
Attachment #274899 - Flags: review?(bzbarsky) → review-
Attached file mockup for basic unthemed stylesheet (obsolete) —
IMHO this is just fine without extra icons. Objections?
Attachment #274897 - Attachment is obsolete: true
Attachment #274899 - Attachment is obsolete: true
Attached file unthemed mockup, the right one (obsolete) —
Attachment #275863 - Attachment is obsolete: true
What do you mean by "without extra icons"?
No up icon, no folder icons. Probably also no favicon, if we end up using the theme version.
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.
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.
I'd prefer having a folder icon by default, yes.
Attached file unthemed with folder icons (obsolete) —
Blocks: 391472
Attached patch patch v5 (obsolete) — Splinter Review
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
Attachment #275866 - Attachment is obsolete: true
Attachment #275904 - Flags: review?
Attached file images and css files (obsolete) —
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...
(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.
Attached file unthemed version with gopher-menu.gif (obsolete) —
> 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.

(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.
> 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?
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.
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.
(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.)
(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.
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.
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.
Attachment #275904 - Attachment is obsolete: true
Attachment #275904 - Flags: review?
Attachment #275905 - Attachment is obsolete: true
Attached patch patch v6 (obsolete) — Splinter Review
Attachment #276052 - Flags: review?
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.
Attachment #276052 - Attachment description: patch v4 → patch v6
Attachment #276052 - Flags: review? → review?(bzbarsky)
Attached patch patch v6Splinter Review
correct encoding this time
Attachment #276052 - Attachment is obsolete: true
Attachment #276053 - Flags: review?(bzbarsky)
Attachment #276052 - Flags: review?(bzbarsky)
Attached file images and css files (obsolete) —
The table body's font size is now at 100% instead of 110%.
Comment on attachment 276053 [details] [diff] [review]
patch v6

Looks good.  Thank you for being patient with me!
Attachment #276053 - Flags: superreview+
Attachment #276053 - Flags: review?(bzbarsky)
Attachment #276053 - Flags: review+
I suppose the toolkit stuff needs a separate review?
Attachment #276055 - Flags: review?(mano)
Attachment #276053 - Flags: approval1.9?
Blocks: 273459
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.
Attachment #276055 - Flags: review?(mano) → review+
as per comment 110
Attachment #275877 - Attachment is obsolete: true
Attachment #275933 - Attachment is obsolete: true
Attachment #276055 - Attachment is obsolete: true
Keywords: checkin-needed
Attachment #276053 - Flags: approval1.9? → approval1.9+
Checked in -- for the eye-pleasing, post-1995 win!
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Flags: in-testsuite-
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3 M7
Depends on: 392713
Depends on: 392718
Keywords: relnote
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?
Yes, please.  Should have caught that...  :(
Blocks: 392759
Blocks: 392760
(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
Component: File Handling → Networking
Flags: ui-review+
Product: Firefox → Core
QA Contact: file.handling → networking
Target Milestone: Firefox 3 M7 → ---
Target Milestone: --- → mozilla1.9 M8
So looking at this in practice, why exactly are we using light gray text on a white background?  It's pretty unreadable over here.
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.
Filed bug 392802 and bug 392803 on my UI issues.
Depends on: 392805
Bug 392807 filed on the performance problems I ran into...
Depends on: 395329
Depends on: 394550
Depends on: 396992
As of Trunk build 31-10-2007, tested on fresh profile the icons are not shown.
filed bug 402742 on icons.
Depends on: 404431
Depends on: 412004
No longer depends on: 394550
No longer depends on: 423594
Depends on: 397823
Blocks: 448141
No longer blocks: 448141
* 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/
Depends on: 823920
You need to log in before you can comment on or make changes to this bug.