402252 - application details should be accessible from prefs UI

Status: VERIFIED FIXED

(Firefox :: File Handling, defect)

Description

[Dan Mosedale (:dmosedale, :dmose)]

At the security/design review, it was observed that it's not possible to see the hostname or URI of a web-based protocol handler from the preferences UI, nor is it possible to edit the title.  I just noticed that it's also not possible to see the path of a local application either.

Nominating as blocking-firefox? because it might actually be wanted-firefox3.

Comment 1

[Alex Faaborg [:faaborg] (Firefox UX)]

Why would the use want to edit the title?  They can't do this for local apps.  I don't think the path to local applications will mean much to mainstream users, but we should provide the path for Web applications due to security.

Comment 2

[Mike Beltzner [:beltzner, not reading bugmail]]

Not sure why we believe that adding the path would do anything other than clutter the display; people who know what the difference is between online and client-side apps will know them by their brand names, people who don't won't be helped by adding a URL.

The trust decision should be made at install-time.

That said, if we wanted to segment Web Apps out into their own section of the drop-down, I'd support that, I guess. Prism confuses this, of course, as it could be a client-side app that's actually a web-app. :)

Comment 3

[Alex Faaborg [:faaborg] (Firefox UX)]

>The trust decision should be made at install-time.

yeah, I agree.

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #1)
> Why would the use want to edit the title?  They can't do this for local apps. 

In case sites give insufficiently unique titles in their registration ("RSS Reader").

> I don't think the path to local applications will mean much to mainstream
> users

It is possible to have an identically named app in two different directories.  Without a way to see the path, you won't be able to tell which is which.  This is, of course, quite edge-casy.

(In reply to comment #2)
> Not sure why we believe that adding the path would do anything other than
> clutter the display;

I don't know that they need to be in primary UI, a tooltip or something might suffice.

> The trust decision should be made at install-time.

Absolutely.  But when people are using this pref UI, they're potentially re-evaluating that trust decision, perhaps in the context of what other apps they now have installed or new information or education.  But now they no longer have all the information that was available to them when they initially made the decision.

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

We pretty much decided over in bug 402245 not to make this trust decision at install time.  As a result, I think we need this for Fx3.  Renominating for reconsideration.

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

Note that the tentative plan for bug 402620 (per faaborg) is to do the same thing (provide ancillary info, in particular at least the hostname) in the dialog as well to prevent web-based handlers from being able to spoof local handlers.

Comment 7

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #5)
> We pretty much decided over in bug 402245 not to make this trust decision at
> install time.  As a result, I think we need this for Fx3.  Renominating for
> reconsideration.

I don't think that's actually the case, though I've been using the terminology loosely.

The user makes the trust decision at the time they click on the link asking to install the application handler, and then again trusts them when they choose the application as a handler for a specific file.

What I said was that if we know that handler shouldn't be trusted, we should caution users; but they've already *made* their trust decision then, and will again.

That said, I'd support some way of verifying that "Gmail" is actually from "www.google.com" when you're running. I think that bug 406620 is the blocker here, and this one is just wanted until we figure out how to do it in a way that doesn't make the design ugly.

Ideas for that would be:

 [Y] Yahoo! Mail (www.yahoo.com)
 [G] GMail  (www.gmail.com)

with the URL part in smaller text in the drop-down.

Comment 8

[Florian Quèze [:florian]]

Pasting here the content of an email from beltzner:


I think we need to make the following changes:

- switch the "Applications" column header to say "Action"
- for files and protocol handlers, always order the list as follows:

  Always ask
  [HandleWithFirefoxOption]
  -----------------------
  [Use System Default]
  Use [app1]
  Use [app2]
  Use other...
  Application details...

 where [HandleWithFirefoxOption] is "Save File" or "Add Live Bookmark" or nothing, depending

- selecting "Application details..." would open a dialog that looks like

 You have indicated that the following applications can be used
 to handle the itms protocol:

  .-------------------------------------------.
  |=App1==(web)============================== | (Remove)
  | App2                                      |
  | App3                                      |
  |                                           |
  '-------------------------------------------'
        App1 is a web application hosted at
        http://foo.bar.baz/biff/baff/boff

                                      ( Cancel ) (( OK ))

Comment 9

[Florian Quèze [:florian]]

Following the UI mockup pasted in the previous comment would also fix bug 402153.

Comment 10

[Florian Quèze [:florian]]

(In reply to comment #7)

> [...] this one is just wanted until we figure out how to do it in a way
> that doesn't make the design ugly.
> 

We have the UI design now.  Renominating, though if blocking it would probably be P4 or P5.

Comment 11

[Florian Quèze [:florian]]

Attachment: patch v1

This was ui-reviewed by beltzner over IRC.

Comment 12

[Asaf Romano (gone)]

Comment on attachment 299332 [details] [diff] [review]
patch v1

nits, mostly.

>Index: browser/components/preferences/applicationManager.js
>===================================================================
>+# Portions created by the Initial Developer are Copyright (C) 2005

that cannot be right.

>+    var defaultApp = this.handlerInfo.preferredApplicationHandler;
>+    var handlers = this.handlerInfo.possibleApplicationHandlers;
>+    for (var i = 0; i < this._removed.length; ++i) {
>+      var app = this._removed[i];
>+      if (defaultApp && app.equals(defaultApp)) {
>+        // If the app we remove was the default app, we must make sure
>+        // it won't be used anymore
>+        this.handlerInfo.alwaysAskBeforeHandling = true;
>+        this.handlerInfo.preferredApplicationHandler = null;

Please file a follow up to automate this.

>+  remove: function appManager_remove() {
>+    var list = document.getElementById("appList");
>+    this._removed.push(list.selectedItem.app);
>+    var index = list.selectedIndex;
>+    list.removeItemAt(index);
>+    if (!list.getRowCount()) {
>+      // The list is now empty, make the bottom part disappear
>+      document.getElementById("appType").hidden = true;
>+      document.getElementById("appLocation").hidden = true;

maybe put the "bottom part" in a box?

>Index: browser/components/preferences/applicationManager.xul
>===================================================================
>+<?xml-stylesheet href="chrome://global/skin/"?>
>+
>+<!DOCTYPE dialog SYSTEM "chrome://browser/locale/preferences/applicationManager.dtd">
>+
>+<dialog id="appManager"
...
>+        windowtype="Browser:AppManager">

this is a modal dialog...

>+  <script type="application/x-javascript"
>+          src="chrome://browser/content/preferences/applicationManager.js"/>
>+  <script type="application/x-javascript"
>+          src="chrome://browser/content/preferences/applications.js"/>
>+

application/javascript

>+  <commandset id="appManagerCommandSet">
>+    <command id="cmd_remove"
>+             oncommand="gAppManagerDialog.remove();"
>+             disabled="true"/>
>+  </commandset>
>+
>+  <keyset id="appManagerKeyset">
>+    <key id="delete" keycode="VK_DELETE" command="cmd_remove"/>
>+  </keyset>

Any chance you could implement a controller for cmd_delete instead? We should also [try to] support backspace on OS X.


>+    </vbox>
>+  </hbox>
>+  <separator class="thin"/>
>+  <description id="appType"/>

label, i think.

>Index: browser/components/preferences/applications.js
>=================================================================== 
>+    // Create a menu item for saving to disk.
>+    // Note: this option isn't available to protocol types, since we don't know
>+    // what it means to save a URL having a certain scheme to disk, nor is it
>+    // available to feeds, since the feed code doesn't implement the capability.
>+    if ((handlerInfo.wrappedHandlerInfo instanceof Ci.nsIMIMEInfo) &&
>+        handlerInfo.type != TYPE_MAYBE_FEED) {
>+      var saveMenuItem = document.createElementNS(kXULNS, "menuitem");

just createElement, feel free to fix other places in this file :)

ditto for other instances in this patch, feel free to fix other places in this file :

* the textNode->textContent change.

Comment 13

[Florian Quèze [:florian]]

Attachment: patch v2

Comment 14

[Florian Quèze [:florian]]

(In reply to comment #12)

> >+    var defaultApp = this.handlerInfo.preferredApplicationHandler;
> >+    var handlers = this.handlerInfo.possibleApplicationHandlers;
> >+    for (var i = 0; i < this._removed.length; ++i) {
> >+      var app = this._removed[i];
> >+      if (defaultApp && app.equals(defaultApp)) {
> >+        // If the app we remove was the default app, we must make sure
> >+        // it won't be used anymore
> >+        this.handlerInfo.alwaysAskBeforeHandling = true;
> >+        this.handlerInfo.preferredApplicationHandler = null;
> 
> Please file a follow up to automate this.
> 

I added a removePossibleApplicationHandler method to HandlerInfoWrapper in applications.js

> >+  <commandset id="appManagerCommandSet">
> >+    <command id="cmd_remove"
> >+             oncommand="gAppManagerDialog.remove();"
> >+             disabled="true"/>
> >+  </commandset>
> >+
> >+  <keyset id="appManagerKeyset">
> >+    <key id="delete" keycode="VK_DELETE" command="cmd_remove"/>
> >+  </keyset>
> 
> Any chance you could implement a controller for cmd_delete instead? We should
> also [try to] support backspace on OS X.

I would prefer doing this in a follow up bug.

Fixed the other nits.

Comment 15

[Asaf Romano (gone)]

Comment on attachment 299666 [details] [diff] [review]
patch v2

I don't like the vanishing details area much. I think it should include the separator if it says.

>+  onOK: function appManager_onOK() {
>+    for (var i = 0; i < this._removed.length; ++i)
>+      this.handlerInfo.removePossibleApplicationHandler(this._removed[i]);
>+
>+    this.handlerInfo.store();

<florian>	I should probably add back the if(!this._removed.lenght) return; at the begining of onOK

>+  onCancel: function appManager_onCancel() {
>+  },
>+

Just remove it (or add a "be gentle, do nothing..

>+  remove: function appManager_remove() {
>+    var list = document.getElementById("appList");
>+    this._removed.push(list.selectedItem.app);
>+    var index = list.selectedIndex;
>+    list.removeItemAt(index);
>+    if (!list.getRowCount()) {

I prefer a |== 0| check here.

>Index: browser/locales/en-US/chrome/browser/preferences/applicationManager.dtd
>===================================================================

>+<!ENTITY appManager.title     "Application details">

Application Details.

r=mano otherwise.

Comment 16

[Florian Quèze [:florian]]

Attachment: patch v3

Comment 17

[Damon Sicore (:damons)]

Comment on attachment 299693 [details] [diff] [review]
patch v3

a1.9+=damons

Comment 18

[Florian Quèze [:florian]]

Fixed the bitrot from bug 398627:
  saveMenuItem.setAttribute("image", ICON_URL_SAVE);
replaced by:
  saveMenuItem.setAttribute(APP_ICON_ATTR_NAME, "save");

and checked in.

Comment 19

[Florian Quèze [:florian]]

Backed out the applications.js changes to fix unit test failure on tinderbox until I recover the lost string changes.

Comment 20

[Florian Quèze [:florian]]

Attachment: patch v3 - missing strings

Parts of the patch that I missed in the previous diff...

Comment 21

[Florian Quèze [:florian]]

Relanded the changes in applications.js, along with attachment 300046 [details] [diff] [review].

Comment 22

[Rimas Kudelis [use needinfo to ping]]

Localizer rant follows.

Quoting applicationManager.properties:
> descriptionApplications=The following applications can be used to handle %S.
> 
> handleProtocol=%S links
> handleWebFeeds=Web Feeds
> handleFile=%S content

Please avoid dynamically composing sentences as often as you can. Too often they become a headache for localizers. Thanks! :)

Comment 23

[Ronny Perinke]

If I open the application details for Web-Feed, I get the following two errors.

Fehler: redeclaration of const Cc
Quelldatei: chrome://browser/content/preferences/applications.js
Zeile: 11

Fehler: gApplicationsPane is not defined
Quelldatei: chrome://browser/content/preferences/applicationManager.js
Zeile: 34


It seems that applicationManager.js is loaded before applications.js, because it defines the constants Ci and Cc. And because of this redeclaration-error, the 2nd error occurs causing gAppManagerDialog.init to fail.

After commenting out the definitions of const Ci and const Cc applicationManager.js everything is ok.

My OS is Windows XP. I guess Ci and Cc are not needed at all in applicationManager.js or it needs an ifdef XP_MACOSX, so that they are only defined, if they are not in applications.js (note the #ifndef XP_MACOSX there).

Comment 24

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Verified with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b3) Gecko/2008020511 Firefox/3.0b3 ID:2008020511.

The Application Details dialog is shown but doesn't list any of the available handlers with the above version. This is fixed (in another bug) when running a latest nightly build e.g. Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b4pre) Gecko/2008021804 Minefield/3.0b4pre ID:2008021804