389705 - tighten up the "Launch Application" dialog (uses too much vertical space)

Status: VERIFIED FIXED

(Firefox :: File Handling, defect, P2)

Description

[(not reading, please use [e-mail] instead)]

tighten up the "Launch Application" dialog (uses too much vertical space)

1) click on ttp://google.com
2) get the new "Launch Application" dialog

I think this dialog needs some cleanup.  

a)  "ttp" line is too tall
b)  "Choose an Application" line is too tall
c)  "Choose..." button is too tall

screen shot coming.

Comment 1

[(not reading, please use [e-mail] instead)]

Attachment: screen shot

Comment 2

[(not reading, please use [e-mail] instead)]

I'm using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a7pre) Gecko/2007072605 Minefield/3.0a7pre

Comment 3

[Shawn Wilsher :sdwilsh]

ttp is supposed to have an image next to it - but we can't do that just yet (that's why it's as tall as it is)

Comment 4

[Jonathan Haas]

Maybe. But the button doesn't have to be as tall as the whole line, does it?

PS: When I select "ttp" and click OK nothing happens. I really don't understand this dialog. What is supposed to happen then? There is no ttp-handler on my System...

Comment 5

[(not reading, please use [e-mail] instead)]

> When I select "ttp" and click OK nothing happens.

see bug #389969

Comment 6

[mcdavis941 (sporadically reading bugmail)]

Do you also want to use an em-based width (rather than px-based) so the dialog can handle changing system font sizes?

http://mxr.mozilla.org/mozilla/source/toolkit/mozapps/handling/content/dialog.xul#48

Comment 7

[Shawn Wilsher :sdwilsh]

Does the width attribute take em sizes?  If not, probably not since we want localizers to be able to change the width...

Comment 8

[mcdavis941 (sporadically reading bugmail)]

Yes, it will take em sizes.  For example, it looks like the "Change Action" dialog is doing it that way.   They're putting the actual em value in a .dtd so it can be localized.

http://mxr.mozilla.org/mozilla/source/browser/components/preferences/changeaction.xul#54
http://mxr.mozilla.org/mozilla/source/browser/locales/en-US/chrome/browser/preferences/changeaction.dtd#2

Comment 9

[Shawn Wilsher :sdwilsh]

I'm not so sure I like the idea now that I think about it more.  What happens if the font increase makes the dialog too wide?

Comment 10

[mcdavis941 (sporadically reading bugmail)]

Several dialogs define width in terms of an em size (e.g. Options window).  Maybe someone from accessibility could advise.  What would someone who is using large fonts prefer to have happen?

Comment 11

[Shawn Wilsher :sdwilsh]

ui feedback wanted as well (beltzner/mconnor)

Comment 12

[Mike Connor [:mconnor]]

too wide is better than too narrow, we should use em everywhere (we do in most places)  Not sure if we want to tighten up the vertical space though.

Comment 13

[Helder "Lthere" Magalhães]

Attachment: Screen shot where the "vertically too short" symptom is shown

Not sure if was due to the latest efforts of minimizing the Launch Application window, but it got too short. I've attached a screen shot which shows this symptom.

I'm using "Classic" theme (Windows 2000 style) in Windows XP with  but I'm sure this isn't the cause for that - default "Blue" theme takes much more space for almost every widget (title bars, buttons, etc.)

Comment 14

[Dan Mosedale (:dmosedale, :dmose)]

Taking, as I'm trying to find owners for the various unowned Firefox 3 blocks.  However!  I've heard this crazy rumor that mfinkle might have a patch in the works for this bug.  If that's the case (or if you'd like it to be, Mark), feel free to grab this bug from me!

Comment 15

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

I'll take this one - you work on the hard ones :)

Comment 16

[Mike Connor [:mconnor]]

moving out bugs that don't need to block b1

Comment 17

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

Attachment: fixes vertical size of row and adds support for handler icon

This patch:
* reduces the height of the richlistbox "button" chooser row
* adds support for fetching the icon for default handlers
* changes dialog width and height to "em"
* removes a period "." from the checkbox label
* removes the unused <description> from the richlistbox helper XBL

Comment 18

[Shawn Wilsher :sdwilsh]

Attachment: screenshot on mac

This doesn't quite look right still on the mac (although I'm not sure if you intended to fix Bug 396635 in the process of this or not).

Note the scrollbars showing up on the mac.

Comment 19

[Shawn Wilsher :sdwilsh]

Comment on attachment 283597 [details] [diff] [review]
fixes vertical size of row and adds support for handler icon

future note - please diff from toplevel (in mozilla/, not in mozilla/toolkit/)

>+
>+      // Try to get some icon for this handler
>+      let mimeSvc = Cc["@mozilla.org/uriloader/external-helper-app-service;1"].
>+                    getService(Ci.nsIMIMEService);
>+      let mimeType = "text/html"; //XXX dumb init for now
>+      try {
>+        mimeType = mimeSvc.getTypeFromURI(this._URI);
>+      }
>+      catch (ex) {}
>+
>+      try {
>+        let mimeInfo = mimeSvc.getFromTypeAndExtension(mimeType, null);
>+        if (mimeInfo instanceof Ci.nsIPropertyBag) {
>+          let url = mimeInfo.getProperty("defaultApplicationIconURL");
>+          if (url)
>+            elm.setAttribute("image", url + "?size=32");
>+        }
>+      }
>+      catch(ex) {}
Something seems wrong about all of this.  If we don't know what the type is, we'll use text/html.  Wouldn't it be better to set it to null, and not do the second try/catch if it's null?

>-richlistitem {
>+richlistitem[type] {
>   min-height: 36px; /* Don't forget to update the richlistbox height! */
> }
Why the change?  I guess I don't follow it..
> 
>+richlistitem {
>+  -moz-box-align: center;
>+}
Same here

With these addressed, I'd suggest Mano or Gavin for reviewers (my review doesn't count for toolkit unless it's a DM patch).

Comment 20

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

(In reply to comment #19)
> (From update of attachment 283597 [details] [diff] [review])

> >+
> >+      // Try to get some icon for this handler
> >+      let mimeSvc = Cc["@mozilla.org/uriloader/external-helper-app-service;1"].
> >+                    getService(Ci.nsIMIMEService);
> >+      let mimeType = "text/html"; //XXX dumb init for now
> >+      try {
> >+        mimeType = mimeSvc.getTypeFromURI(this._URI);
> >+      }
> >+      catch (ex) {}
> >+
> >+      try {
> >+        let mimeInfo = mimeSvc.getFromTypeAndExtension(mimeType, null);
> >+        if (mimeInfo instanceof Ci.nsIPropertyBag) {
> >+          let url = mimeInfo.getProperty("defaultApplicationIconURL");
> >+          if (url)
> >+            elm.setAttribute("image", url + "?size=32");
> >+        }
> >+      }
> >+      catch(ex) {}
> Something seems wrong about all of this.  If we don't know what the type is,
> we'll use text/html.  Wouldn't it be better to set it to null, and not do the
> second try/catch if it's null?
> 

We need to pick an icon if we can't determine the type. I am happy to use any, even the default application.png, just not blank.

> >-richlistitem {
> >+richlistitem[type] {
> >   min-height: 36px; /* Don't forget to update the richlistbox height! */
> > }
> Why the change?  I guess I don't follow it..

We don't want the "Choose and application" row to be 36px tall, only the rows that are "type" of application.

> > 
> >+richlistitem {
> >+  -moz-box-align: center;
> >+}
> Same here
> 

We want the text to be centered vetically in the row. The image and/or the button is taller than the text so the text appears aligned to the top of the row. The UI mockups had the text centered vertically in the row.

> With these addressed, I'd suggest Mano or Gavin for reviewers (my review
> doesn't count for toolkit unless it's a DM patch).
> 

I will use a better default application image, then submit for review again.

Comment 21

[Dan Mosedale (:dmosedale, :dmose)]

If we don't already have a handler or protocol specific image (I'll spin off a separate bug about that), I'm not convinced that showing some generic image is terribly helpful to the user, so I wouldn't sweat that.

Comment 22

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

Attachment: fixes vertical size of row and adds support for handler icon (v2)

Same as previous patch except:
* dropped any changes for dialog resizing (those were fixed in another bug)
* added CSS changes for Mac

Comment 23

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

bug 404546 added so we can get a decent icon for the default handler

Comment 25

[Florian Quèze [:florian]]

(In reply to comment #17)
> * removes the unused <description> from the richlistbox helper XBL

It will probably be used to display hostnames of web handlers (see bug 402620)

Comment 26

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

Attachment: patch that only addresses the height of the richlistitems

This patch changes the CSS to reduce the height of the "button" row of the richlistitems. It also removes the currently unused description label in the richlistitem row and vertically center aligns the remaining name label, since the row is 32px high (from the image)

Comment 27

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

Checking in toolkit/mozapps/handling/content/handler.xml;
/cvsroot/mozilla/toolkit/mozapps/handling/content/handler.xml,v  <--  handler.xml
new revision: 1.2; previous revision: 1.1
done
Checking in toolkit/themes/pinstripe/mozapps/handling/handling.css;
/cvsroot/mozilla/toolkit/themes/pinstripe/mozapps/handling/handling.css,v  <--  handling.css
new revision: 1.2; previous revision: 1.1
done
Checking in toolkit/themes/winstripe/mozapps/handling/handling.css;
/cvsroot/mozilla/toolkit/themes/winstripe/mozapps/handling/handling.css,v  <--  handling.css
new revision: 1.2; previous revision: 1.1
done

Comment 28

[Tony Chung [:tchung]]

Verified fix on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b2) Gecko/2007121014 Firefox/3.0b2.  Cancel and OK buttons now fit completely in the window, and the scrollbar is gone.