85173 - Save As... zipfile download with very-long url generates oversized save window

Status: VERIFIED FIXED

(SeaMonkey :: UI Design, defect, P3)

Description

[Fernando Cassia]

With Mozilla 0.91, log into www.idrive.com. 
Now enter any public shared space (I used username "Maritxa1998" which I found 
on the Net).

Idrive returns a list of publicly shared files. When you click on one of these 
filenames and start a download, the webserver sends the file with a dynamically 
generated long url like:

www.idrive.com/[verylongstringofchars_sometimes_above_100_chars]/filename.zip

Mozilla 0.91 displays the "file download action" window prompting the user 
what to do with this file that the webserver is sending, displaying the FULL 
URL, making this window several times the size of the screen. The window is so 
long, that the "OK" "CANCEL" etc buttons cannot be seen because they end 
off-screen.

I have a screenshot showing this bug. I would attach it but I've just realized 
that webzilla has no way to attach a file to document visual bugs. (Hint, 
hint!!).

Comment 1

[Fernando Cassia]

Attachment: Screenshot showing the bug

Comment 2

[Fernando Cassia]

Attachment: Screenshot showing the bug

Comment 3

[Matthias Versen [:Matti]]

confirming with win2k build 20010611.. (CVS opt)

Comment 4

[sairuh (rarely reading bugmail)]

->bill

Comment 5

[Asa Dotzler [:asa]]

*** Bug 84741 has been marked as a duplicate of this bug. ***

Comment 6

[Asa Dotzler [:asa]]

Updating Platform and OS to all (my testing and bug 84741 confirm it is a
problem on Mac too)

Comment 7

[sairuh (rarely reading bugmail)]

could someone provide a test username/passwd i could use at idrive.com to test
this? thx!

Comment 8

[sairuh (rarely reading bugmail)]

thanks to Fernando for the clarification: i needed to put "maritxa1998"
 into the "Visitors" field and click the Visit button. d'oh!

Comment 9

[sairuh (rarely reading bugmail)]

i also see this on linux --in fact i cannot even dismiss the dialog [well,
keyboard nav is busted, bug 74012].

Comment 10

[sairuh (rarely reading bugmail)]

[sidenote: was able to dismiss by tabbing around the dialog, hitting spacebar,
then cancelling if i got the Save File file picker --ie, if i didn't manage tab
to the cancel button in the download/helper app dialog.]

Comment 11

[sairuh (rarely reading bugmail)]

excellent suggestion from Fernando, via email:

I think, imho, that a good way to fix this bug is to trim the url displayed
to say 50 chars, and add "(...)" at the end.

For example showing:
Downloading file:
http://www.idrive.com/maritxa1998/adskdhskjdhksdhksdsk(...)file.zip

Comment 12

[Bill Law]

The same thought had occurred to me.

Here's a patch that does that (actually, it replaces the middle with "...", not
the end, because usually the end is more interesting):

Index: embedding/components/ui/helperAppDlg/nsHelperAppDlg.js
===================================================================
RCS file: /cvsroot/mozilla/embedding/components/ui/helperAppDlg/nsHelperAppDlg.js,v
retrieving revision 1.9
diff -u -r1.9 nsHelperAppDlg.js
--- nsHelperAppDlg.js	2001/05/23 06:03:58	1.9
+++ nsHelperAppDlg.js	2001/06/13 20:01:46
@@ -243,7 +243,16 @@
             modified = this.getString( "intro.noDesc" );
         }
         modified = this.replaceInsert( modified, 2,
this.mLauncher.MIMEInfo.MIMEType );
-        modified = this.replaceInsert( modified, 3, this.mSourcePath );
+        var path = this.mSourcePath;
+        const maxPath = 50;
+        // Maximum is 50 characters, plus 3 for the "...".
+        if ( path.length > maxPath + 3 ) {
+            // Path too long, replace middle section with "...".
+            path = path.substring( 0, maxPath/2 ) +
+                   "..." +
+                   path.substring( path.length - maxPath/2 );
+        }
+        modified = this.replaceInsert( modified, 3, path );
         intro.firstChild.nodeValue = "";
         intro.firstChild.nodeValue = modified;
     },

Comment 13

[Paul Chen]

nav triage team:

Oi, that's a long dialog. Marking nsbeta1+, p3, mozilla0.9.3

Comment 14

[Blake Ross]

Dean Tessman has a patch to make crop="middle" working, so we could also use 
that, but since that would crop the entire label, this patch seems fine to me.  
Since Bill's on sabbatical, I'll taek it and get it in.

Comment 15

[Blake Ross]

Oops...Bill is still checking in, he must be around :-)

Anyways, I'm thinking maybe we should use crop="middle" after all.  This is a
little hackish, and I'm also not sure if hardcoding "..." like that is a
localization problem.

Comment 16

[timeless]

yes hard coding like that would lead to a localization nightmare. please use 
crop="middle".

Comment 17

[Bill Law]

Sounds like a good idea, so long as it works.  I didn't even know about
crop="middle" so it's not like I'm partial to coding it up myself.  And this is
that last Bugzilla comment I'm going to make for a while, anyway.

Comment 18

[Jesse Ruderman]

Another advantage to using crop="middle" (assuming it works) is that it avoids
hard-coding the number of characters to be displayed in the download dialog.

Comment 19

[Viswanath Ramachandran]

Can we move this on to m0.9.3? 

Comment 20

[Blake Ross]

Sure, it's waiting on 50833 anyways...

Comment 21

[:deanis74]

50833 was supposed to get in for 0.9.2, but I don't think that's going to 
happen.  It has an r= and sr= and I mailed drivers for approval but I haven't 
heard anything back.

Comment 22

[sairuh (rarely reading bugmail)]

*** Bug 90086 has been marked as a duplicate of this bug. ***

Comment 23

[:deanis74]

I've got crop="middle" working in my tree, using my patch for bug 50833.  Can
someone save me the time hunting around and suggest where I would put
crop="middle" in this dialog to test if it addresses the problem?

And why is this marked dependent on bug 74012?  That bug is a keyboard problem,
nothing to do with this one.

Comment 24

[Matthias Versen [:Matti]]

*** Bug 93851 has been marked as a duplicate of this bug. ***

Comment 25

[Fernando Cassia]

Well, it seems that Idrive.com closed down their public, free service, where 
this bug was 1st found.

So... how can we test this bug?. I'm sure there are plenty of other sites that 
generate download file urls "on the fly" with long strings... probably Lotus 
Notes/Domino based servers, which are famous for the long dynamic URLs.

Anyone knows any such site?

Comment 26

[Daniel Rogers]

I created a long URL to test this myself.  You can find it at
http://www.island.net/~rogersd/mozilla/foo/bar/baz/alpha/beta/gamma/delta/test/test/test/long/url/to/break/download/window/notreallya.exe


Also, another good test is downloading a file from extra.newsguy.com, as they
use some really long URLs to identify the Usenet articles.

Comment 27

[:deanis74]

There could be a little problem here.  I'm not strong in xul, but it seems that 
the location field is a textbox with a class of scrollbox.  Neither of these 
support the crop attribute.

http://lxr.mozilla.org/seamonkey/source/xpfe/components/ucth/resources/helperApp
DldProgress.xul#75

Comment 28

[:deanis74]

Sorry, that should have been scrollfield not scrollbox.

Comment 29

[:deanis74]

Attachment: or I could be way off (patch)

Comment 30

[:deanis74]

*sigh*  I just realized I've been working with the wrong dialog.  But the patch 
I just attached does crop the filenames in the download progress dialog!

Sorry for all the spam tonight everyone.  I blame it on too much sun.

Comment 31

[:deanis74]

Updated the url.  I'll leave that file up for a while.

Comment 32

[:deanis74]

Patch coming to fix this.  I split the location into a separate <text> so that 
it can use crop="center".  The odd thing is, I couldn't get
  location.value = pathString;
to work, I had to use
  location.setAttribute("value", pathString);

As jag pointed out to me, in the bindings setting value should just call 
setAttribute.  Odd.
  
http://lxr.mozilla.org/seamonkey/source/xpfe/global/resources/content/bindings/t
ext.xml#14

Comment 33

[:deanis74]

Attachment: cvs diff -u

Comment 34

[:deanis74]

Attachment: screenshot before this patch

Comment 35

[:deanis74]

Attachment: screenshot after this patch

Comment 36

[jag (Peter Annema)]

Hrm. So actually that <text> should be a <label> (<text> is deprecated), and
apparently we're only putting the xbl bindings on <label>, not on <text>. Not
sure if that was intentional or not.

Comment 37

[John Morrison]

Note that the dorky construct:

    intro.firstChild.nodeValue = "";
    intro.firstChild.nodeValue = modified;

is actually a "highly sophisticated kludge" for a problem with that dialog 
on Linux (I can't for the life of me find the original bug report for that,
but, IIRC, it had to do with the contents shifting and not being repainted 
when you opened the 'Choose...' dialog).

It may be the case that your change makes that no-op line unneeded (or 
conversely, that we will need to come up with another dumb hack :-\).

Comment 38

[:deanis74]

Attachment: new patch

Comment 39

[:deanis74]

New patch using <description> instead of <text>.  I sent this off to jag but I 
realize he's pretty busy.  My build is pre-<description>, can someone test this?

I realize now, after I've attached it, that I still don't use .value.  Argh, I 
shouldn't do this at 2am - I'm really making a mess of this bug!  Before 
testing, change location.setAttribute(..) to:
  location.value = pathString;

Does the screenshot still look the same as attachment 45740 [details]?  I'm wondering 
mostly about the cropping, of course, but also that the url lines up nicely on 
the left with the other text.

Comment 40

[jag (Peter Annema)]

Yeah, looks okay.

So changes I made was to remove comments and replace .setAttribute("value", ...)
with .value = ....;

And secondly I needed to add value="" to the <description> because otherwise it
wouldn't size the element correctly (namely 0 height -> no text visible after
setting value). Bug?

Comment 41

[:deanis74]

I'd say that's a bug, yeah.  Sounds like what jrgm was referring to yesterday. 
I'll do up a new patch tonight with those changes.

Comment 42

[:deanis74]

Attachment: here it is

Comment 43

[Blake Ross]

jag, wanna r= this?

Comment 44

[jag (Peter Annema)]

r=jag

Comment 45

[sairuh (rarely reading bugmail)]

*** Bug 96538 has been marked as a duplicate of this bug. ***

Comment 46

[Asa Dotzler [:asa]]

Comment on attachment 45856 [details] [diff] [review]
here it is

a=asa on behalf of drivers

Comment 47

[:deanis74]

need an sr=... Blake?

Comment 48

[Blake Ross]

--> dean

Comment 49

[Blake Ross]

Checked in.

Comment 50

[R.K.Aa.]

*** Bug 98045 has been marked as a duplicate of this bug. ***

Comment 51

[sairuh (rarely reading bugmail)]

vrfy fixed.

linux, 2001.09.04.08-comm
winnt, 2001.09.04.00-comm
mac 9.1, 2001.09.04.11-comm

Comment 52

[Daniel Rogers]

Ok, the window size is fine now (Build 2001090408), but now it doesn't prompt me
for a location to save the file when the mime type is set to "Save to disk".  It
just starts saving it to /tmp and the original filename is lost.

However, if it asks "What should Mozilla do with this file?" and I choose "Save
this file to disk", it works fine.

This didn't happen in 0.9.3