420595 - "you have chosen to open" dialog uses wrong icon

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[u294409]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; pl; rv:1.9b4pre) Gecko/2008030204 Fedora/8 (Werewolf) Minefield/3.0b4pre
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; pl; rv:1.9b4pre) Gecko/2008030204 Fedora/8 (Werewolf) Minefield/3.0b4pre

"save file" button has ok icon, while gtk-save should be used.

Reproducible: Always

Steps to Reproduce:
1. enter *** http://packages.ubuntu.com/hardy/all/ubuntu-wallpapers/download ***
2. pick *** www.nic.funet.fi/pub/mirrors/archive.ubuntu.com *** server
3. look at the dialog which appears
Actual Results:  
wrong icon for "save file" button

Expected Results:  
gtk-save icon for "save file" button

Comment 1

[u294409]

Attachment: screenshot

Comment 2

[Reed Loden [:reed]]

The <button> needs icon="save"...

Comment 3

[Michael Ventnor]

I'm guessing because this is a Mozilla common dialog. In every common dialog we use gtk-ok for the affirmative action and gtk-cancel for the negative one. As trivial as this bug sounds, it would be very hard to fix in time for Firefox 3...

Comment 4

[u294409]

Mozilla :/ .

Comment 5

[Julien "_FrnchFrgg_" RIVAUD]

No, this is not a PromptService dialog. This dialog is the usual "what do you want to do with this file" dialog, but with the selector hidden and button text changed because there is only one choice possible (save as). This is done here:

http://mxr-stage.mozilla.org/seamonkey/source/toolkit/mozapps/downloads/src/nsHelperAppDlg.js.in#444

So what should be done is change the icon attribute to "save", for instance. Will this pose a l10n problem (since we aren't sure that the string is always translated to a "save" equivalent). I don't think so, but it may be refused because of that...

Comment 6

[Stephen Donner [:stephend] Not actively reading bugmail]

-> file handling

Comment 7

[Stephen Donner [:stephend] Not actively reading bugmail]

although, based on the file above, I probably got the wrong component.

I wish https://bugzilla.mozilla.org/describecomponents.cgi?product=Firefox were more clear on the distinction between Download Manager and File Handling.

Comment 8

[Shawn Wilsher :sdwilsh]

Often times there isn't much of a difference ;)

Comment 9

[Julien "_FrnchFrgg_" RIVAUD]

Attachment: Patch v1

Here is a patch that does what I proposed. It fixes the bug for me. Who should I ask review to ?

Comment 10

[Julien "_FrnchFrgg_" RIVAUD]

I can post a patch with unusefull wrapping removed, if wanted.

Comment 11

[Shawn Wilsher :sdwilsh]

Comment on attachment 307146 [details] [diff] [review]
Patch v1

>+        // change the accept button icon to a save one, since it's what it
>+        // will do
nit: just keep everything before the comma please

>+        this.mDialog.document.documentElement.getButton("accept").setAttribute("icon", "save");
also, can you do two things:
1) use a local variable for the accept button to help with line wrapping.  Go ahead and update the other lines of code that use the accept button as well.
2) Move this bit of code to very top of the if statement.

Just to be clear - this just handles the case where it's always going to be save to disk, right?  We aren't handling the case where the user selects save to disk?

Comment 12

[Julien "_FrnchFrgg_" RIVAUD]

Shawn: Yes, it only hanldes the case where we hide the selection UI and change the button label to "Save", when there is no other possible action.

Comment 13

[Julien "_FrnchFrgg_" RIVAUD]

Attachment: Patch v2

Addressed comments

Comment 14

[Julien "_FrnchFrgg_" RIVAUD]

Attachment: Patch v2.1

I attached the wrong patch. This one addresses comments.

Comment 15

[Shawn Wilsher :sdwilsh]

Comment on attachment 307756 [details] [diff] [review]
Patch v2.1

>+        var acceptButton = this.mDialog.document.getButton("accept");
s/var/let/ please

>+        // change the accept button icon to a save one
>+        acceptButton.setAttribute("icon", "save");
>+        // change button labels
>+        acceptButton.label = this.dialogElement("strings").getString("unknownAccept.label");
>+        this.mDialog.document.documentElement.getButton("cancel").label = this.dialogElement("strings").getString("unknownCancel.label");
since we are playing with these lines now, can we have line wrapping at 80 chars please

r=sdwilsh with these fixed.

Comment 16

[Julien "_FrnchFrgg_" RIVAUD]

Attachment: Patch v2.2

With review comments addressed.

Comment 17

[Julien "_FrnchFrgg_" RIVAUD]

Comment on attachment 308215 [details] [diff] [review]
Patch v2.2

Ultra-low cost patch to change the accept button icon when its label change (when "save to disk" is the only available proposition)

Comment 18

[Shawn Wilsher :sdwilsh]

drivers:  This is also a linux only issue but one of those nice polish wins.

Comment 19

[Julien "_FrnchFrgg_" RIVAUD]

It could benefit other platforms if/when they show icons in buttons.

Comment 20

[Damon Sicore (:damons)]

Comment on attachment 308215 [details] [diff] [review]
Patch v2.2

a1.9+=damons

Comment 21

[Reed Loden [:reed]]

Checking in toolkit/mozapps/downloads/src/nsHelperAppDlg.js.in;
/cvsroot/mozilla/toolkit/mozapps/downloads/src/nsHelperAppDlg.js.in,v  <--  nsHelperAppDlg.js.in
new revision: 1.64; previous revision: 1.63
done

Comment 22

[pal-moz]

this checkin cause below change ?

with 20080311_0018_firefox-3.0b5pre.en-US.win32.zip
http://img210.imageshack.us/img210/660/a1aq7.jpg

with 20080311_0152_firefox-3.0b5pre.en-US.win32.zip
http://img210.imageshack.us/img210/3780/a2zo8.jpg

Comment 23

[u294409]

I don't think so. File changed here is "nsHelperAppDlg.js", while responsible for files handling is other file.

Comment 24

[Jim Jeffery not reading bug-mail 1/2/11]

Clicking 'Save' and 'Always do this...' does not stick, once you start another download the File Picker opens and the settings are back to 'open' disregarding the setting to 'Save' and 'Do always'.

Changing Tools->Options->Applications : to 'Save' brings up the File picker straight away when trying to download .exe file.  

Is this the intended action here?  There is not a hint when setting:
Tools->Options->Main Downloads: 'Ask where to save files..' That there is an interaction with the Applications Tab->Applications settings.  

This is going to lead to confusion for end-users I think.

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b5pre) Gecko/2008031214 Minefield/3.0b5pre Firefox/3.0 Firefox/2.0.0.12 ID:2008031214

Comment 25

[Shawn Wilsher :sdwilsh]

uh....this *only* fixed the icon for linux here.  There was application logic change in this patch...

Comment 26

[Julien "_FrnchFrgg_" RIVAUD]

There shouldn't be any application logic change; there was an error in the patch I attached for checkin, a "document" instead of "documentElement", which probably prevented the rest of the function to execute.

In bug 422585 it was said that I needed to include a test with this patch. What kind of test, and how do I do that ?

REOPENING this bug so I have it on my radar.

Comment 27

[Justin Dolske [:Dolske]]

Backed out:
Checking in toolkit/mozapps/downloads/src/nsHelperAppDlg.js.in;
  new revision: 1.65; previous revision: 1.64

I'd suggest a general test that makes sure initDialog() finished successfully, since sdwilsh seems to really really want a test... whatever.

Comment 28

[Julien "_FrnchFrgg_" RIVAUD]

Shawn, could you give me hints on what would be the best kind of test ? How would I know that initDialog succeeded ? By testing on what it does ? By adding a "initDone" attribute/member somewhere at the end of init, and checking for initDone==true in the test ?

Comment 29

[Shawn Wilsher :sdwilsh]

What I'd like to see is that the dialog has the proper buttons showing in the case where it's only save as.  What we ended up regressing was that it was showing the full dialog with options to open with - I just want to see that those are not there.

Comment 30

[Julien "_FrnchFrgg_" RIVAUD]

We regressed because a typo prevented the function to run completely... I don't really understand what you want here; do you want me to duplicate the code which decides when save should be the only action and put it in a test ? Or do you want me to create files who we know we should always propose save only, and trigger a download of those and then check if the dialog has the correct widgets hidden ?
Or something else ?

Comment 31

[Shawn Wilsher :sdwilsh]

With mochi and browser tests you can specify the headers sent for a file.  You'll probably need to make a browser tests, and with the right headers being sent, you should be able to manipulate the dialog to be the right type.  Then yes - make sure the correct widgets are not shown and the right ones are.

Comment 32

[Julien "_FrnchFrgg_" RIVAUD]

I didn't have time to write the test, and probably won't until the end of next week. I'll write it, just hope it won't be too late. At least it will be done, perhaps not in Firefox 3.

Comment 33

[Matthew Gregan [:kinetik]]

Comment on attachment 308215 [details] [diff] [review]
Patch v2.2

Clearing approval since patch was backed out.

Comment 34

[Matthew Gregan [:kinetik]]

(and we're trying to reduce the list of open bugs with a1.9 patches attached to find ones that need attention)

Comment 35

[Julien "_FrnchFrgg_" RIVAUD]

Bug 445005 introduced a test for the very same case; if I find a way to add to that test a verification that the correct button names are used, could this patch be checked-in anew ?

Shawn, what do you think ?

Comment 36

[Shawn Wilsher :sdwilsh]

(In reply to comment #35)
> Bug 445005 introduced a test for the very same case; if I find a way to add to
> that test a verification that the correct button names are used, could this
> patch be checked-in anew ?
Yup, I'm happy with the test that's landed (that's what I wanted for this before).

Comment 37

[Julien "_FrnchFrgg_" RIVAUD]

Attachment: Unbitrotted patch

Resubmitting a patch since the test has been taken care of.

Comment 38

[Shawn Wilsher :sdwilsh]

Comment on attachment 332637 [details] [diff] [review]
Unbitrotted patch

r=sdwilsh, assuming that is 80 character line wrapping.

Comment 39

[Julien "_FrnchFrgg_" RIVAUD]

Attachment: Unbitrotted patch, with 80 chars wrapping

Carrying review from sdwilsh

Comment 40

[Julien "_FrnchFrgg_" RIVAUD]

Attachment: With real 80 chars line wrapping

Sorry, I uploaded the very same patch as before. Now this one should really have 80 char wrapping.

Comment 41

[Dão Gottwald [:dao]]

http://hg.mozilla.org/index.cgi/mozilla-central/rev/603f2fcb6a54