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
The <button> needs icon="save"...
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...
Mozilla :/ .
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...
-> file handling
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.
Often times there isn't much of a difference ;)
Created attachment 307146 [details] [diff] [review] Patch v1 Here is a patch that does what I proposed. It fixes the bug for me. Who should I ask review to ?
I can post a patch with unusefull wrapping removed, if wanted.
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?
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.
Created attachment 307743 [details] [diff] [review] Patch v2 Addressed comments
Created attachment 307756 [details] [diff] [review] Patch v2.1 I attached the wrong patch. This one addresses comments.
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.
Created attachment 308215 [details] [diff] [review] Patch v2.2 With review comments addressed.
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)
drivers: This is also a linux only issue but one of those nice polish wins.
It could benefit other platforms if/when they show icons in buttons.
Comment on attachment 308215 [details] [diff] [review] Patch v2.2 a1.9+=damons
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
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
I don't think so. File changed here is "nsHelperAppDlg.js", while responsible for files handling is other file.
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/220.127.116.11 ID:2008031214
uh....this *only* fixed the icon for linux here. There was application logic change in this patch...
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.
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.
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 ?
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.
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 ?
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.
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 on attachment 308215 [details] [diff] [review] Patch v2.2 Clearing approval since patch was backed out.
(and we're trying to reduce the list of open bugs with a1.9 patches attached to find ones that need attention)
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 ?
(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).
Created attachment 332637 [details] [diff] [review] Unbitrotted patch Resubmitting a patch since the test has been taken care of.
Comment on attachment 332637 [details] [diff] [review] Unbitrotted patch r=sdwilsh, assuming that is 80 character line wrapping.
Created attachment 333031 [details] [diff] [review] Unbitrotted patch, with 80 chars wrapping Carrying review from sdwilsh
Created attachment 333139 [details] [diff] [review] With real 80 chars line wrapping Sorry, I uploaded the very same patch as before. Now this one should really have 80 char wrapping.