Save as Dialog is oversized and ugly

VERIFIED FIXED

Status

defect
VERIFIED FIXED
11 years ago
8 years ago

People

(Reporter: dougt, Assigned: mfinkle)

Tracking

Trunk
ARM
Windows Mobile 6 Professional
Dependency tree / graph

Firefox Tracking Flags

(fennec1.0a1-wm+)

Details

Attachments

(4 attachments, 2 obsolete attachments)

Reporter

Description

11 years ago
Posted image screenshot
The save as dialog in Fennec on windows mobile profession is oversized and ugly.
before hacking on the dialog, we should see what it looks like with better sized fonts
I assume you mean we should use smaller fonts.  I don' think we can go too much smaller without making the fonts hard to read.  

In the very least the dialog shouldn't be wider than the screen
Hardware: x86 → ARM
Smaller fonts will help the width, but maybe not all of it. Are we using the system desired fonts and font sizes?
the font in the dialog is the same height as the font in the title bar and the font in the url bar (14 pixels high).  The 'o's in the dialog are atually 1 pixel skinnier than the 'o's in the title bar and url bar (12 vs. 13 pixels)
Group: mozilla-corporation-confidential
Group: mozilla-corporation-confidential
tracking-fennec: --- → 1.0a1-wm+
Madhava, it looks like we need a design for a smaller save as dialog
Assignee: nobody → madhava
I will attach a more permanent version of this soon, but for now:
http://www.grabup.com/uploads/f267460c48dcb6b557706b4ea7ef0f6c.png
madhaeve is that planned to be wince only or maemo too ?
(In reply to comment #7)
> madhaeve is that planned to be wince only or maemo too ?

I am assuming both.
Assignee: madhava → mark.finkle
I see "Open", "Save" and "nothing" actions. What secondary flow is needed for "Open" and "Save", if any?
Status: NEW → ASSIGNED
Posted patch WIP 1: the basics (obsolete) — Splinter Review
This contains the basic functionality, except for "Open" which is currently downloading the file, but failing to actually open the file with the registered app.
Posted image screenshot of dialog
Obligatory screenshot showing the dialog

Comment 13

10 years ago
Wouldn't it be more consistent to have "Cancel" instead of "Nothing"?
Also, I hope using a box-shadow with 10px blur doesn't melt the phone, maybe I'm underestimating the power of modern phones :)
> This contains the basic functionality, except for "Open" which is currently
> downloading the file, but failing to actually open the file with the registered
> app.

mark, is the OPEN failing to launch the file on all platforms ?
(In reply to comment #13)
> Wouldn't it be more consistent to have "Cancel" instead of "Nothing"?
> Also, I hope using a box-shadow with 10px blur doesn't melt the phone, maybe
> I'm underestimating the power of modern phones :)

Not in the context of the question used as the prompt: "What would you like to do with Xxx?" Of course we might change this and are open to suggestions.

The box-shadow didn't look slow on my n810. We are also using it for our AlertsSystem override:
http://people.mozilla.com/~mfinkle/fennec/screenshots/addons-alert.png

Also, opening a new XUL window on mobile devices seems to take a long time: ~1350ms on mobile vs ~100ms on desktop
(In reply to comment #14)

> mark, is the OPEN failing to launch the file on all platforms ?

I am only testing on Windows desktop at the moment.

Comment 17

10 years ago
(In reply to comment #15)
> (In reply to comment #13)
> > Wouldn't it be more consistent to have "Cancel" instead of "Nothing"?
> > Also, I hope using a box-shadow with 10px blur doesn't melt the phone, maybe
> > I'm underestimating the power of modern phones :)
> 
> Not in the context of the question used as the prompt: "What would you like to
> do with Xxx?" Of course we might change this and are open to suggestions.

"Cancel" is conventional, you look at it and know immediately it is there to undo an action. "Nothing", to me, still implies that the flow of actions is continuing, rather than reversing. It might take a little longer for users to recognize what that button will do because they see a "Nothing" button far less often than a "Cancel" one.

Also, I've got a bug with a patch to make box-shadow slightly faster on elements with rounded corners.
Posted patch WIP 2 (obsolete) — Splinter Review
Adds themes/wince parts
Attachment #373787 - Attachment is obsolete: true
Posted patch patchSplinter Review
This patch adds support for opening and for prompting. It also uses the new theme.

Brad, can you test this out and do a review?
Attachment #374183 - Attachment is obsolete: true
Attachment #375983 - Flags: review?(bugmail)
Comment on attachment 375983 [details] [diff] [review]
patch

>+    container.top = top < 0 ? 0 : top;
When is top negative?

>     downloadManagerUI.js \
>+    helperAppDialog.js \
>     $(NULL)
funky ws here (is there a tab?)

>+    let file = null;
>+Components.reportError("prompt")
>+

>+      } catch (e) { }
>+Components.reportError("auto: " + autodownload)
>+      if (autodownload) {

>+        let defaultFolder = dnldMgr.userDownloadsDirectory;
>+Components.reportError("folder: " + defaultFolder.path)
>+

>+        catch (e) {
>+Components.reportError("error: " + e)
>+        }
>+Components.reportError("file: " + file.path)
>+

why is Components.reportError never indented?



>+    // TODO: add XP_WIN code
>+/*
>+#ifdef XP_WIN
>+    let ext;
>+    try {
>+      // We can fail here if there's no primary extension set
>+      ext = "." + this.mLauncher.MIMEInfo.primaryExtension;
>+    } catch (e) { }
>+
>+    // Append a file extension if it's an executable that doesn't have one
>+    // but make sure we actually have an extension to add
>+    let leaf = aLocalFile.leafName;
>+    if (aLocalFile.isExecutable() && ext &&
>+        leaf.substring(leaf.length - ext.length) != ext) {
>+      let f = aLocalFile.clone();
>+      aLocalFile.leafName = leaf + ext;
>+
>+      f.remove(false);
>+      this.makeFileUnique(aLocalFile);
>+    }
>+#endif
>+*/

>+
>+//function NSGetModule(aCompMgr, aFileSpec) {
>+//  return XPCOMUtils.generateModule([HelperAppLauncherDialog]);
>+//}

let's not commit commented out code

This looks very similar to toolkit/mozapps/downloads/src/nsHelperAppDlg.js.in, why can't we just use that?  Also, because this mostly looks like copied code I haven't done a very thorough review of it.  If there are significant changes could you please point them out?
Attachment #375983 - Flags: review?(bugmail) → review+
(In reply to comment #20)
> (From update of attachment 375983 [details] [diff] [review])
> >+    container.top = top < 0 ? 0 : top;
> When is top negative?

We align to the bottom of the urlbar, which could be panned up into the negative area.

> >     downloadManagerUI.js \
> >+    helperAppDialog.js \
> >     $(NULL)
> funky ws here (is there a tab?)

Yes, the file was using tabs

> >+    let file = null;
> >+Components.reportError("prompt")
> >+
> 
> why is Components.reportError never indented?

Debugging code. Removed.

> >+    // TODO: add XP_WIN code
> >+/*
> >+#ifdef XP_WIN
> snipped
> >+#endif
> >+*/

Removed

> >+
> >+//function NSGetModule(aCompMgr, aFileSpec) {
> >+//  return XPCOMUtils.generateModule([HelperAppLauncherDialog]);
> >+//}

I left this in with a comment about the fact the standard service uses a delayed registration, so we can't use XPCOMUtils yet.

> This looks very similar to toolkit/mozapps/downloads/src/nsHelperAppDlg.js.in,
> why can't we just use that?  Also, because this mostly looks like copied code I
> haven't done a very thorough review of it.  If there are significant changes
> could you please point them out?

It is similar, but nsHelperAppDlg.js does double duty: It implements the service, but also implements the XUL dialog used by toolkit. The code is rather intertwined. The new code is a minimal implementation of just the service.
http://hg.mozilla.org/mobile-browser/rev/89c87d27cd7e
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Blocks: 436070

Comment 23

8 years ago
VERIFIED FIXED on:

Build ID: Mozilla /5.0 (Android;Linux armv7l;rv:6.0a1) Gecko/20110523 Firefox/6.0a1 Fennec/6.0a1 

Device: HTC Desire Z (Android 2.2)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.