479808 - Save as Dialog is oversized and ugly

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Doug Turner (:dougt)]

Attachment: screenshot

The save as dialog in Fennec on windows mobile profession is oversized and ugly.

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

before hacking on the dialog, we should see what it looks like with better sized fonts

Comment 2

[Brad Lassey [:blassey] (use needinfo?)]

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

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

Smaller fonts will help the width, but maybe not all of it. Are we using the system desired fonts and font sizes?

Comment 4

[Brad Lassey [:blassey] (use needinfo?)]

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)

Comment 5

[Brad Lassey [:blassey] (use needinfo?)]

Madhava, it looks like we need a design for a smaller save as dialog

Comment 6

[Madhava Enros [:madhava]]

I will attach a more permanent version of this soon, but for now:
http://www.grabup.com/uploads/f267460c48dcb6b557706b4ea7ef0f6c.png

Comment 7

[Antonio Gomes (tonikitoo)]

madhaeve is that planned to be wince only or maemo too ?

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #7)
> madhaeve is that planned to be wince only or maemo too ?

I am assuming both.

Comment 9

[Madhava Enros [:madhava]]

Attachment: save as dialog wireframe

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

I see "Open", "Save" and "nothing" actions. What secondary flow is needed for "Open" and "Save", if any?

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: WIP 1: the basics

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.

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: screenshot of dialog

Obligatory screenshot showing the dialog

Comment 13

[Michael Ventnor]

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 :)

Comment 14

[Antonio Gomes (tonikitoo)]

> 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 ?

Comment 15

[Mark Finkle (:mfinkle) (use needinfo?)]

(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

Comment 16

[Mark Finkle (:mfinkle) (use needinfo?)]

(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

[Michael Ventnor]

(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.

Comment 18

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: WIP 2

Adds themes/wince parts

Comment 19

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch

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?

Comment 20

[Brad Lassey [:blassey] (use needinfo?)]

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?

Comment 21

[Mark Finkle (:mfinkle) (use needinfo?)]

(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.

Comment 22

[Mark Finkle (:mfinkle) (use needinfo?)]

http://hg.mozilla.org/mobile-browser/rev/89c87d27cd7e

Comment 23

[Anna (Waverley)]

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)