Closed Bug 389705 Opened 16 years ago Closed 15 years ago

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

Categories

(Firefox :: File Handling, defect, P2)

x86
Windows XP
defect

Tracking

()

VERIFIED FIXED
Firefox 3 beta2

People

(Reporter: moco, Assigned: mfinkle)

References

()

Details

(Keywords: polish, Whiteboard: [proto])

Attachments

(4 files, 2 obsolete files)

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.
I'm using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a7pre) Gecko/2007072605 Minefield/3.0a7pre
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)
Flags: blocking-firefox3?
Blocks: 385065
No longer depends on: 385065
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...
> When I select "ttp" and click OK nothing happens.

see bug #389969
Flags: blocking-firefox3? → blocking-firefox3+
Keywords: polish
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
Does the width attribute take em sizes?  If not, probably not since we want localizers to be able to change the width...
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
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?
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?
ui feedback wanted as well (beltzner/mconnor)
No longer blocks: 385065
Depends on: 385065
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.
Target Milestone: --- → Firefox 3 M9
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.)
Component: General → File Handling
QA Contact: general → file.handling
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!
Assignee: nobody → dmose
I'll take this one - you work on the hard ones :)
Assignee: dmose → mark.finkle
moving out bugs that don't need to block b1
Target Milestone: Firefox 3 M9 → Firefox 3 M10
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
Attachment #283597 - Flags: review?(dmose)
Attached image 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 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).
Attachment #283597 - Flags: review?(dmose) → review-
(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.
No longer blocks: 396635
Depends on: 396635
Priority: -- → P3
Whiteboard: [proto]
Priority: P3 → P2
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.
Whiteboard: [proto] → [proto][needs new patch]
Same as previous patch except:
* dropped any changes for dialog resizing (those were fixed in another bug)
* added CSS changes for Mac
Attachment #283597 - Attachment is obsolete: true
Attachment #289499 - Flags: review?(dmose)
bug 404546 added so we can get a decent icon for the default handler
Attachment #289499 - Flags: review?(dmose)
(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)
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)
Attachment #289499 - Attachment is obsolete: true
Attachment #291231 - Flags: review?(dmose)
Attachment #291231 - Flags: review?(dmose) → review?(gavin.sharp)
Whiteboard: [proto][needs new patch] → [proto][needs review gavin]
Attachment #291231 - Flags: review?(gavin.sharp) → review+
Keywords: checkin-needed
Whiteboard: [proto][needs review gavin] → [proto][needs checkin]
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
Status: NEW → RESOLVED
Closed: 15 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [proto][needs checkin] → [proto]
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.