"you have chosen to open" dialog uses wrong icon

RESOLVED FIXED in mozilla1.9.1a2

Status

()

Toolkit
Downloads API
--
minor
RESOLVED FIXED
10 years ago
7 years ago

People

(Reporter: Jakub 'Livio' Rusinek, Assigned: Julien "_FrnchFrgg_" RIVAUD)

Tracking

({polish})

Trunk
mozilla1.9.1a2
x86
Linux
polish
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [not needed for 1.9])

Attachments

(2 attachments, 6 obsolete attachments)

(Reporter)

Description

10 years ago
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
(Reporter)

Comment 1

10 years ago
Created attachment 306891 [details]
screenshot
Version: unspecified → Trunk
The <button> needs icon="save"...

Comment 3

10 years ago
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...
(Reporter)

Comment 4

10 years ago
Mozilla :/ .
(Assignee)

Comment 5

10 years ago
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...
Status: UNCONFIRMED → NEW
Ever confirmed: true
-> file handling
Component: Download Manager → File Handling
QA Contact: download.manager → 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 ;)
(Assignee)

Comment 9

10 years ago
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 ?
Assignee: nobody → frnchfrgg-mozbugs
Status: NEW → ASSIGNED
(Assignee)

Comment 10

10 years ago
I can post a patch with unusefull wrapping removed, if wanted.
Component: File Handling → Download Manager
QA Contact: file.handling → download.manager
Attachment #307146 - Flags: review?(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?
Attachment #307146 - Flags: review?(sdwilsh) → review-
(Assignee)

Comment 12

10 years ago
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.
(Assignee)

Comment 13

10 years ago
Created attachment 307743 [details] [diff] [review]
Patch v2

Addressed comments
Attachment #307146 - Attachment is obsolete: true
Attachment #307743 - Flags: review?
(Assignee)

Updated

10 years ago
Attachment #307743 - Flags: review? → review?(sdwilsh)
(Assignee)

Comment 14

10 years ago
Created attachment 307756 [details] [diff] [review]
Patch v2.1

I attached the wrong patch. This one addresses comments.
Attachment #307743 - Attachment is obsolete: true
Attachment #307756 - Flags: review?(sdwilsh)
Attachment #307743 - Flags: review?(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.
Attachment #307756 - Flags: review?(sdwilsh) → review+
(Assignee)

Comment 16

10 years ago
Created attachment 308215 [details] [diff] [review]
Patch v2.2

With review comments addressed.
Attachment #307756 - Attachment is obsolete: true
(Assignee)

Comment 17

10 years ago
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)
Attachment #308215 - Flags: approval1.9?
drivers:  This is also a linux only issue but one of those nice polish wins.
Keywords: polish
Target Milestone: --- → Firefox 3 beta5
(Assignee)

Comment 19

10 years ago
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
Attachment #308215 - Flags: approval1.9? → approval1.9+
Keywords: checkin-needed
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
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED

Comment 22

10 years ago
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
(Reporter)

Comment 23

10 years ago
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/2.0.0.12 ID:2008031214

Updated

10 years ago
Depends on: 422464
uh....this *only* fixed the icon for linux here.  There was application logic change in this patch...

Updated

10 years ago
Depends on: 422585
(Assignee)

Comment 26

10 years ago
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
(Assignee)

Comment 28

10 years ago
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.
(Assignee)

Comment 30

10 years ago
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.
(Assignee)

Comment 32

10 years ago
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.
Attachment #308215 - Flags: approval1.9+
(and we're trying to reduce the list of open bugs with a1.9 patches attached to find ones that need attention)
Whiteboard: [not needed for 1.9]
(Assignee)

Comment 35

10 years ago
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).
Product: Firefox → Toolkit
(Assignee)

Comment 37

10 years ago
Created attachment 332637 [details] [diff] [review]
Unbitrotted patch

Resubmitting a patch since the test has been taken care of.
Attachment #308215 - Attachment is obsolete: true
Attachment #332637 - Flags: review?(sdwilsh)
Comment on attachment 332637 [details] [diff] [review]
Unbitrotted patch

r=sdwilsh, assuming that is 80 character line wrapping.
Attachment #332637 - Flags: review?(sdwilsh) → review+
(Assignee)

Comment 39

10 years ago
Created attachment 333031 [details] [diff] [review]
Unbitrotted patch, with 80 chars wrapping

Carrying review from sdwilsh
Attachment #332637 - Attachment is obsolete: true
Attachment #333031 - Flags: review+
(Assignee)

Updated

10 years ago
Keywords: checkin-needed
(Assignee)

Comment 40

10 years ago
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.
Attachment #333031 - Attachment is obsolete: true
Attachment #333139 - Flags: review+
http://hg.mozilla.org/index.cgi/mozilla-central/rev/603f2fcb6a54
Status: REOPENED → RESOLVED
Last Resolved: 10 years ago10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: mozilla1.9beta5 → mozilla1.9.1a2
You need to log in before you can comment on or make changes to this bug.