588266 - Firefox add-on installation dialog should use doorhanger notification

Status: RESOLVED WONTFIX

(Toolkit :: Add-ons Manager, defect)

Description

[Justin Scott [:fligtar]]

Attachment: mock v1 by Boriss

Let's move the add-on installation dialog to using the doorhanger notification API, as shown in the attached mock.

This change should only affect Firefox (not Fennec or other apps). It should function the same as other doorhanger notifications, and clicking off of it closes the pop-up but the icon remains for later. The add-on install "allow once" doorhanger is a good model to follow as it already uses the puzzle icon.

Notes on the mock:
* Add-on name and icon should come from install.rdf, which will already be available as we now download the add-on before showing the install dialog

* The "By" line should only show up if the add-on is signed and display the name of the organization

* There should be a 1 second delay before Add to Firefox button is enabled.

* The icon is using the new default add-on size of 48x48. If the icon is smaller than that it should be top-right-aligned.

* Add-on name and icon should still be passed to InstallTrigger, as these will be used to display in the Downloads Manager while the add-on is downloading. I don't think this affects this bug; I'm just noting it.

* The large puzzle piece isn't the final icon, so don't worry about that quite yet.

* The warning text should have a second line: "Malicious software can damage your computer or violate your privacy."

* Please use the existing doorhanger API in Firefox 4.

Feature freeze is on August 27th and this doesn't block, so this needs to be done very quickly if we are getting it in Firefox 4, and I really hope we are.

Thanks!

Comment 1

[Brian King [:kinger]]

Some initial observations / questions.

1. The doorhanger API (PopupNotifications.jsm) seems to only support simple text notifications. It will need to be extended to handle other UI, in this case a list of add-on(s).

2. Does the new Add-ons API support installing multiple add-ons? I seem to recall a bug, but all I can find is bug 497306 which does not seem like the right one.

3. Does this notification all under site based notifications (bug 588240) or Home Tab Snippets (bug 588230)? I think the latter as installs can come not just from sites, but locally as well.

Comment 2

[Brian King [:kinger]]

Jetpack add-ons don't currently have icons to show, see bug 588119. So we'll have to show the generic one.

Comment 3

[Dave Townsend [:mossop]]

(In reply to comment #1)
> 2. Does the new Add-ons API support installing multiple add-ons? I seem to
> recall a bug, but all I can find is bug 497306 which does not seem like the
> right one.

Yes, we support both installing multiple XPIs through a list of them passed to InstallTrigger.install and multi-package XPIs.

> 3. Does this notification all under site based notifications (bug 588240) or
> Home Tab Snippets (bug 588230)? I think the latter as installs can come not
> just from sites, but locally as well.

The former really since it needs to show up in the browser regardless of whether the user has the home tab displayed.

(In reply to comment #2)
> Jetpack add-ons don't currently have icons to show, see bug 588119. So we'll
> have to show the generic one.

We should be showing the icon specified by the InstallTrigger.install call.

Comment 5

[Justin Scott [:fligtar]]

A few more notes:

* As Mossop said, for the icon we should first try the icon in the package, followed by the icon in InstallTrigger, followed by the platform's default add-on icon

* The styling of the doorhanger notification bubble itself will be done by whoever implements the theme for the rest of them, so don't worry about that

* Dolske prefers that you do separate patches for each main thing, e.g. one that extends the doorhanger api, one that disables the current xpinstall dialog, etc.

Comment 6

[Justin Scott [:fligtar]]

Oh, and:

* I said that the add-on icons were 48x48 because that's the new default size. Boriss wants to use 32x32 add-on icons for this dialog, so we'd have to scale down the 48x48. I suppose we can try this and see how it looks.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #1)
> 1. The doorhanger API (PopupNotifications.jsm) seems to only support simple
> text notifications. It will need to be extended to handle other UI, in this
> case a list of add-on(s).

I don't think you actually need to modify the API for this to work. You can extend the popupnotification binding and override based on ID to have the notification popup itself behave differently, and you can pass in the relevant information you need (addons to install, etc.) by using a property of the "options" parameter to PopupNotifications.show() (that object is passed directly to the popupnotification via popupnotification.notification.options). Mardak is doing similar stuff for account manager, as well, so you might want to ping either/both of us on IRC to figure out how exactly we want to do this.

Comment 8

[Brian King [:kinger]]

I wonder if this notification should also handle the progress indication when the add-on is downloading. Right now the dialog only appears after download. See bug 570012.

Comment 9

[Dave Townsend [:mossop]]

(In reply to comment #8)
> I wonder if this notification should also handle the progress indication when
> the add-on is downloading. Right now the dialog only appears after download.
> See bug 570012.

I don't think that's a good idea as we don't want to have something hovering over the page while the user is downloading the add-on

Comment 10

[Sylvain Pasche]

Hi, I'm helping Brian on this bug.

(In reply to comment #7)
> (In reply to comment #1)
> I don't think you actually need to modify the API for this to work. You can
> extend the popupnotification binding and override based on ID to have the
> notification popup itself behave differently, and you can pass in the relevant
> information you need (addons to install, etc.) by using a property of the
> "options" parameter to PopupNotifications.show() (that object is passed
> directly to the popupnotification via popupnotification.notification.options).
> Mardak is doing similar stuff for account manager, as well, so you might want
> to ping either/both of us on IRC to figure out how exactly we want to do this.

I found bug 571409 which should give a good example how to do that.

Do you happen to know where to hook for running code when the notification is dismissed? (in order to do the equivalent of http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/xpinstall/content/xpinstallConfirm.js#178).

(In reply to comment #5)
> * As Mossop said, for the icon we should first try the icon in the package,
> followed by the icon in InstallTrigger, followed by the platform's default
> add-on icon

It looks like the icon from InstallTrigger is currently not used when the addons are ready to be installed: http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/XPIProvider.jsm#4341. I suggest to look at this in a separate bug.


Here's how I propose to implement this:
Add a component implementing amIWebInstallPrompt in browser/components/nsBrowserGlue.js in order to get the notification when addons are ready to be installed (this way we still fallback to using xpinstallConfirm.xul for other apps).

That component would open a PopupNotification which is overridden by a binding in browser/base/content/xpinstallConfirm.xml. That binding would duplicate some of the logic that is currently in toolkit/mozapps/xpinstall/content/xpinstallConfirm.{js,xul}.

Does this plan look ok?

Comment 11

[Dave Townsend [:mossop]]

(In reply to comment #10)
> Hi, I'm helping Brian on this bug.
> 
> (In reply to comment #7)
> > (In reply to comment #1)
> > I don't think you actually need to modify the API for this to work. You can
> > extend the popupnotification binding and override based on ID to have the
> > notification popup itself behave differently, and you can pass in the relevant
> > information you need (addons to install, etc.) by using a property of the
> > "options" parameter to PopupNotifications.show() (that object is passed
> > directly to the popupnotification via popupnotification.notification.options).
> > Mardak is doing similar stuff for account manager, as well, so you might want
> > to ping either/both of us on IRC to figure out how exactly we want to do this.
> 
> I found bug 571409 which should give a good example how to do that.
> 
> Do you happen to know where to hook for running code when the notification is
> dismissed? (in order to do the equivalent of
> http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/xpinstall/content/xpinstallConfirm.js#178).
> 
> (In reply to comment #5)
> > * As Mossop said, for the icon we should first try the icon in the package,
> > followed by the icon in InstallTrigger, followed by the platform's default
> > add-on icon
> 
> It looks like the icon from InstallTrigger is currently not used when the
> addons are ready to be installed:
> http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/XPIProvider.jsm#4341.
> I suggest to look at this in a separate bug.

I forgot about that but actually that code means we always use the InstallTrigger icon for add-ons ready to be installed. It would be nice to use the icon from the xpi if possible, but that would require extracting it somewhere since otherwise we lock the xpi on windows. And extracting it has the problem that you need to be very careful to clean it up after, so yes I'd suggest sticking with the InstallTrigger icon for now.

> Here's how I propose to implement this:
> Add a component implementing amIWebInstallPrompt in
> browser/components/nsBrowserGlue.js in order to get the notification when
> addons are ready to be installed (this way we still fallback to using
> xpinstallConfirm.xul for other apps).
> 
> That component would open a PopupNotification which is overridden by a binding
> in browser/base/content/xpinstallConfirm.xml. That binding would duplicate some
> of the logic that is currently in
> toolkit/mozapps/xpinstall/content/xpinstallConfirm.{js,xul}.
> 
> Does this plan look ok?

That sounds like the way to go to me.

Comment 12

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #10)
> Do you happen to know where to hook for running code when the notification is
> dismissed? (in order to do the equivalent of
> http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/xpinstall/content/xpinstallConfirm.js#178).

Popup notifications don't currently support being "dismissed" - they just stick around until they are cleared by navigation (exact behavior depends on the timeout/persistence arguments to show()). We could easily re-add the "dismissalHandler" logic that I had in the earlier patches for bug 398776 (e.g. attachment 448336 [details] [diff] [review]), though - we probably should.

Comment 13

[Sylvain Pasche]

Attachment: WIP, v1.0

I changed my mind from the implementation proposal and put the xpinstallConfirm.xml binding in toolkit/mozapps/extensions (it should be application agnostic and is kind of related to the extensions code).

This is a first work in progress for feedback. What remains: cancel the add-on installation when the popup is dismissed and the tests will need to be updated/added.

Comment 14

[:Gavin Sharp [email: [e-mail]]]

BTW, given that you're moving getChromeWindow, you might as well take the opportunity to update it to this simpler version: http://hg.mozilla.org/mozilla-central/annotate/0aefd9d484f1/toolkit/components/console/hudservice/HUDService.jsm#l1563

Comment 15

[Sylvain Pasche]

Attachment: Refactoring of the mozapps/exensions browser tests (only one test updated)

Here's how I propose to change the test harness. Please let me know if that seems good and I'll refactor all the tests.

The approach here is to listen for the "popupshown" popup event. An alternative would be to replace the "@mozilla.org/addons/web-install-prompt;1" component for the duration of the tests in order to get install events.

Comment 16

[Sylvain Pasche]

(In reply to comment #11)
> I forgot about that but actually that code means we always use the
> InstallTrigger icon for add-ons ready to be installed. It would be nice to use
> the icon from the xpi if possible, but that would require extracting it
> somewhere since otherwise we lock the xpi on windows. And extracting it has the
> problem that you need to be very careful to clean it up after, so yes I'd
> suggest sticking with the InstallTrigger icon for now.

I filed bug 590798 for this.

Comment 17

[Sylvain Pasche]

Attachment: patch, v2 (without tests).

This should be applied on top of bug 590794. That should be feature complete, but is missing the tests.

When the notification is dismissed, I cancel the add-on installation. If the notification is not removed from the NotificationPanel, the user could display it again by clicking on the notification icon and try to install the add-ons again. That wouldn't work because they have been cancelled. To prevent that issue, I remove the notification when dismissed.

A better behavior might be not to remove the notification when the popup is dismissed, but instead cancel the add-ons installation only when the notification is removed. However we don't have a "notification was removed" callback yet (maybe that could be added in addition to or instead of the dismissed callback?).

Comment 18

[Dave Townsend [:mossop]]

Comment on attachment 469293 [details] [diff] [review]
Refactoring of the mozapps/exensions browser tests (only one test updated)

This looks like it will break tests on Seamonkey because they haven't implemented the popup notifications yet, please keep both paths working.

Comment 19

[Dave Townsend [:mossop]]

Comment on attachment 469578 [details] [diff] [review]
patch, v2 (without tests).

>diff --git a/toolkit/locales/en-US/chrome/mozapps/extensions/xpinstallConfirm.properties b/toolkit/locales/en-US/chrome/mozapps/extensions/xpinstallConfirm.properties
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/locales/en-US/chrome/mozapps/extensions/xpinstallConfirm.properties
>@@ -0,0 +1,6 @@
>+itemWarnIntroMultiple=Would you like to install the following %S add-ons?
>+itemWarnIntroSingle=Would you like to install the following add-on?

You need to use the plural form code to make this work right.

>diff --git a/toolkit/themes/pinstripe/mozapps/extensions/xpinstallConfirm.css b/toolkit/themes/pinstripe/mozapps/extensions/xpinstallConfirm.css

Doesn't look like this file is packaged at all.

>diff --git a/toolkit/themes/winstripe/mozapps/jar.mn b/toolkit/themes/winstripe/mozapps/jar.mn

>         skin/classic/mozapps/extensions/pause.png                  (extensions/pause.png)
>         skin/classic/mozapps/extensions/utilities.png              (extensions/utilities.png)
>         skin/classic/mozapps/extensions/eula.css                   (extensions/eula.css)
>+*       skin/classic/mozapps/extensions/xpinstallConfirm.css       (extensions/xpinstallConfirm.css)

Is there a need to preprocess this file?

Comment 20

[Sylvain Pasche]

(In reply to comment #18)
> Comment on attachment 469293 [details] [diff] [review]
> Refactoring of the mozapps/exensions browser tests (only one test updated)
> 
> This looks like it will break tests on Seamonkey because they haven't
> implemented the popup notifications yet, please keep both paths working.

The properties in the bindings of the dialog and popup are slightly different, so if we want both paths working I suggest mocking amIWebInstallPrompt to make the tests independent from the UI.

Comment 21

[Dave Townsend [:mossop]]

(In reply to comment #20)
> (In reply to comment #18)
> > Comment on attachment 469293 [details] [diff] [review] [details]
> > Refactoring of the mozapps/exensions browser tests (only one test updated)
> > 
> > This looks like it will break tests on Seamonkey because they haven't
> > implemented the popup notifications yet, please keep both paths working.
> 
> The properties in the bindings of the dialog and popup are slightly different,
> so if we want both paths working I suggest mocking amIWebInstallPrompt to make
> the tests independent from the UI.

Then the tests wouldn't be testing hte UI. That isn't necessarily a problem, it just means that UI specific tests would also have to be written.

Comment 22

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

[strings]: probably new strings for the direct question and action button

Comment 23

[Sylvain Pasche]

(In reply to comment #19)
> Comment on attachment 469578 [details] [diff] [review]
> patch, v2 (without tests).
> 
> >diff --git a/toolkit/locales/en-US/chrome/mozapps/extensions/xpinstallConfirm.properties b/toolkit/locales/en-US/chrome/mozapps/extensions/xpinstallConfirm.properties
> >new file mode 100644
> >--- /dev/null
> >+++ b/toolkit/locales/en-US/chrome/mozapps/extensions/xpinstallConfirm.properties
> >@@ -0,0 +1,6 @@
> >+itemWarnIntroMultiple=Would you like to install the following %S add-ons?
> >+itemWarnIntroSingle=Would you like to install the following add-on?
> 
> You need to use the plural form code to make this work right.

Done. Should I also update xpinstallConfirm.xul? (I copied strings from it).

> >diff --git a/toolkit/themes/pinstripe/mozapps/extensions/xpinstallConfirm.css b/toolkit/themes/pinstripe/mozapps/extensions/xpinstallConfirm.css
> 
> Doesn't look like this file is packaged at all.

Fixed

> >diff --git a/toolkit/themes/winstripe/mozapps/jar.mn b/toolkit/themes/winstripe/mozapps/jar.mn
> 
> >         skin/classic/mozapps/extensions/pause.png                  (extensions/pause.png)
> >         skin/classic/mozapps/extensions/utilities.png              (extensions/utilities.png)
> >         skin/classic/mozapps/extensions/eula.css                   (extensions/eula.css)
> >+*       skin/classic/mozapps/extensions/xpinstallConfirm.css       (extensions/xpinstallConfirm.css)
> 
> Is there a need to preprocess this file?

There is an ifdef in the css I copied from mozapps/xpinstall. I didn't look at updating the theme for now, but after the theme update that might not be needed any more.

Comment 24

[Sylvain Pasche]

Attachment: part1 (change without tests), v3

Updated from review comments. Also fixed a few things that broke the tests.

Comment 25

[Brian King [:kinger]]

Attachment: part1 (change without tests), v4

The patch bit-rotted a bit already (!), so here's an updated version.

Comment 26

[Sylvain Pasche]

Attachment: part1 (change without tests), v5

Add a .dtd file which was missing in the previous patch refresh. Fix missing add-on icon on the left of the doorhanger.

Comment 27

[Sylvain Pasche]

(In reply to comment #21)
> (In reply to comment #20)
> > (In reply to comment #18)
> > > Comment on attachment 469293 [details] [diff] [review] [details] [details]
> > > Refactoring of the mozapps/exensions browser tests (only one test updated)
> > > 
> > > This looks like it will break tests on Seamonkey because they haven't
> > > implemented the popup notifications yet, please keep both paths working.
> > 
> > The properties in the bindings of the dialog and popup are slightly different,
> > so if we want both paths working I suggest mocking amIWebInstallPrompt to make
> > the tests independent from the UI.
> 
> Then the tests wouldn't be testing hte UI. That isn't necessarily a problem, it
> just means that UI specific tests would also have to be written.

I encountered some issues when trying to override the amIWebInstallPrompt component during the tests. Finally, I made both paths working by detecting if the popup notification is used or not.

Comment 28

[Sylvain Pasche]

Attachment: part2: tests, v1

Tests. I created a xpinstallConfirm_utils.js utility file to share common code between the extensions browser tests and xpinstall tests.

Comment 29

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

Sylvain: could you post a screen shot of the notification for UI review?

Comment 30

[Sylvain Pasche]

Attachment: Screenshot of notification on Windows

Here's how it looks on Windows. The screenshot shows a single add-on installation on top and with two add-ons below (one of which is signed).

Note that I haven't looked at styling the notification to make it match the mockup exactly, as comment 5 implies it would be done separately.

Comment 31

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

Comment on attachment 473184 [details]
Screenshot of notification on Windows

Overall good, but let's use a 16x16 icon for the warning, and a single line of normal sized text:

"Only install add-ons if you trust the author. Malicious software can violate your privacy."

With the current layout the warning seems to be a third add-on instead of providing context to the action button.

Also, this notification should anchor to the site identity block, and not it's own 16x16 parent (the puzzle piece icon to the left of the site identity block).

Comment 32

[Justin Scott [:fligtar]]

(In reply to comment #31)
> Also, this notification should anchor to the site identity block, and not it's
> own 16x16 parent (the puzzle piece icon to the left of the site identity
> block).

Having its own icon is consistent with the implementation of the notification that replaced the yellow bar for add-on installation allowance. This allows the user to click it again while on the same page if they accidentally dismissed the dialog. Will clicking the site identity button bring the add-on installation dialog back?

Comment 33

[Brian King [:kinger]]

(In reply to comment #30)
> Note that I haven't looked at styling the notification to make it match the
> mockup exactly, as comment 5 implies it would be done separately.

Is there a bug for this or should we open a new one?

Comment 34

[Brian King [:kinger]]

Attachment: Screenshot of notification on Mac

Here is what it is looking like on Mac, and similar to Windows without styling.

On both platforms there seems to be too much space between the warning and the install button.

Comment 35

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

>This allows the
>user to click it again while on the same page if they accidentally dismissed
>the dialog. Will clicking the site identity button bring the add-on
>installation dialog back?

Bug 588613
mockup: https://bugzilla.mozilla.org/attachment.cgi?id=467307

A lot of people currently working on notifications that don't involve the user's personal identity are waiting on that change as well.

Comment 36

[Axel Hecht [:Pike]]

Comment on attachment 472403 [details] [diff] [review]
part1 (change without tests), v5

<!ENTITY warningPrimary.label    "Only install add-ons only from authors whom you trust.">
is still changing anyway, right? "Only" too often.

Comment 37

[Dave Townsend [:mossop]]

Comment on attachment 472407 [details] [diff] [review]
part2: tests, v1

Nice work!

Comment 38

[Brian King [:kinger]]

Mossop, do you have time to review the other patch as well?

Comment 39

[Dave Townsend [:mossop]]

(In reply to comment #38)
> Mossop, do you have time to review the other patch as well?

Even if I did we are now past the point where this could land.

Comment 40

[Brian King [:kinger]]

(In reply to comment #39)
> Even if I did we are now past the point where this could land.

For beta 7 or final? Where does it say this (I don't see anything in mozilla.dev.planning)?

A review would still be nice so we could tidy up and nominate if a window opens up late features. I know this is a UX most wanted bug.

http://limi.net/articles/24-bugs-firefox-4/ -- #24

Comment 41

[Dave Townsend [:mossop]]

(In reply to comment #40)
> (In reply to comment #39)
> > Even if I did we are now past the point where this could land.
> 
> For beta 7 or final? Where does it say this (I don't see anything in
> mozilla.dev.planning)?

The tree is currently locked down for beta 7 blockers only and this isn't a beta 7 blocker, and beta 7 is the feature freeze so this normally wouldn't be able to land after then.

Comment 42

[Justin Dolske [:Dolske]]

Comment on attachment 473184 [details]
Screenshot of notification on Windows

First, a couple additional comments on the doorhanger's design:

The "source: https://...." line should go away. Since the notification is already tied to the site/urlbar, I think we should avoid duplicating it. But I suppose there's  the question of when goodsite.com has an (iframe/XSS/hacked) link to badsite.com/evil.xpi. Can we just block that case? Not sure how that's currently  handled.

I'm kind of ambivalent about having the addon's icon in the notification, seems distracting and muddles the purpose of the notification. Having a wall of text and icons makes this far more likely to just end up as a "whatever" notification.

Comment 43

[Jesse Ruderman]

We currently handle the cross-site case terribly. See bug 358266.

Comment 44

[Justin Dolske [:Dolske]]

Comment on attachment 472403 [details] [diff] [review]
part1 (change without tests), v5

>+function XPInstallPrompt() {}
...
>+  confirm: function(aWindow, aURL, aInstalls) {
...
>+    try {
>+      chromeWin = getChromeWindow(aWindow);
>+    } catch (e) {
>+      // That could happen if the window is closed.
>+      chromeWin = Services.wm.getMostRecentWindow("navigator:browser");
>+    }

Hmm, I'm not sure I understand how this case can happen.

If a page wants to install something but the window has since been closed, the install request should just be canceled, we shouldn't be picking a random (most recent) tab and claiming it wants to install an addon.

Do locally-triggered addon installs go through this path? (eg File -> Open or drag'n'drop) They also shouldn't be associated with the tab, though I'm not sure what the best place is... Open the addons manager and use a different prompt? Something like that would also take care of the weird case of triggering an addon install without a navigator:browser window open (eg, it would open one and load about:addons?).

>+    if (aWindow.document)
>+      browser = chromeWin.gBrowser.getBrowserForDocument(aWindow.document);
>+    else
>+      browser = chromeWin.gBrowser.selectedBrowser;

Similar as above, shouldn't be picking a random tab here.

>+      callback: function() {
>+        // Defer the addon installation to let the notification panel close.
>+        // Useful for tests relying on popupshown events.
>+        Services.tm.mainThread.dispatch({
>+          run: function() {
>+            aInstalls.forEach(function(install) {
>+              install.install();

I think this has partially been discussed elsewhere (maybe?), but what's supposed to happen UX/UI-wise in terms of showing download progress and handling install errors? Does the EM just handle all this?

>+      dismissalCallback: function() {
>+        // remove the notification, because add-ons can't be installed again
>+        // after they are cancelled.
>+        chromeWin.PopupNotifications.remove(notification);
>+        aInstalls.forEach(function(install) {
>+          install.cancel();
>+        });
>+      },

Hmm, this breaks the "click to dismiss, click again to bring back" semantics to these kind of notifications. Is there a reason for doing this?

>+    notification = chromeWin.PopupNotifications.show(browser,
>+                                                     "addon-install-downloaded",
>+                                                     "<ignored>",
>+                                                     "addons-notification-icon",
>+                                                     mainAction, null, options);


>+++ b/browser/locales/en-US/chrome/browser/browser.properties
...
>+xpinstallAddButton=Add to %S

How about just "Install"?


>+++ b/toolkit/mozapps/extensions/content/xpinstallConfirm.css
>@@ -0,0 +1,4 @@
>+installitem {
>+  -moz-binding: url("chrome://mozapps/content/extensions/xpinstallConfirm.xml#installitem");
>+  display: -moz-box;
>+}

display: -moz-box is the default already here, isn't it?


>+++ b/toolkit/mozapps/extensions/content/xpinstallConfirm.xml
>+# The Initial Developer of the Original Code is
>+# Ben Goodger.
>+# Portions created by the Initial Developer are Copyright (C) 2001
>+# the Initial Developer. All Rights Reserved.
>+#
>+# Contributor(s):
>+#   Ben Goodger <[e-mail]>

Really?! :) I'd suspect this is almost entirely new code, so the old attribution isn't needed.

>+      <field name="bundle" readonly="true">
>+        document.getAnonymousElementByAttribute(this, "anonid", "xpinstallConfirmStrings");
>+      </field>

I think we're generally avoiding <stringbundle> now, just grab this with Services in the constructor / init.

>+      <constructor><![CDATA[
>+        let numItemsToInstall = this.installs.length;
>+        for (let i = 0; i < numItemsToInstall; ++i) {
>+          let install = this.installs[i];
>+          let installItem = document.createElement("installitem");
>+          this.itemList.appendChild(installItem);
>+
>+          installItem.name = install.name;
>+          installItem.version = install.version;
>+          installItem.url = install.sourceURI.spec;
>+          if (install.iconURL)
>+            installItem.icon = install.iconURL;
>+          if (install.type)
>+            installItem.type = install.type;
>+          if (install.certName)
>+            installItem.cert = install.certName;
>+          installItem.signed = install.certName ? "true" : "false";

Are these sanity / length checked by EM before they get here? [For the bits the extension can control, at least.]

Should make sure a naughty addon can't play tricks with giant icons, or long description / version strings.

Comment 45

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #44)
> >+      dismissalCallback: function() {
> >+        // remove the notification, because add-ons can't be installed again
> >+        // after they are cancelled.
> >+        chromeWin.PopupNotifications.remove(notification);
> >+        aInstalls.forEach(function(install) {
> >+          install.cancel();
> >+        });

Fwiw, my patch for bug 587587 adds a much simpler "removeOnDismissal" flag that does this automatically. Also this patch looks like it hasn't been updated to take into account the changes in bug 597244.

Comment 46

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #44)
> I think this has partially been discussed elsewhere (maybe?), but what's
> supposed to happen UX/UI-wise in terms of showing download progress and
> handling install errors? Does the EM just handle all this?

I think that "elsewhere" is bug 570012, which probably also conflicts with this patch.

Comment 47

[Sylvain Pasche]

(In reply to comment #42)
> Comment on attachment 473184 [details]
> Screenshot of notification on Windows
>
> First, a couple additional comments on the doorhanger's design:
>
> The "source: https://...." line should go away. Since the notification is
> already tied to the site/urlbar, I think we should avoid duplicating it. But I
> suppose there's  the question of when goodsite.com has an (iframe/XSS/hacked)
> link to badsite.com/evil.xpi. Can we just block that case? Not sure how that's
> currently  handled.

Let's revisit that when bug 358266 is fixed?

>
> I'm kind of ambivalent about having the addon's icon in the notification, seems
> distracting and muddles the purpose of the notification. Having a wall of text
> and icons makes this far more likely to just end up as a "whatever"
> notification.

Do you mean the generic addon icon on the left of the notification, or the icon that the addon author can provide?

(In reply to comment #44)
> Comment on attachment 472403 [details] [diff] [review]
> part1 (change without tests), v5
>
> >+function XPInstallPrompt() {}
> ...
> >+  confirm: function(aWindow, aURL, aInstalls) {
> ...
> >+    try {
> >+      chromeWin = getChromeWindow(aWindow);
> >+    } catch (e) {
> >+      // That could happen if the window is closed.
> >+      chromeWin = Services.wm.getMostRecentWindow("navigator:browser");
> >+    }
>
> Hmm, I'm not sure I understand how this case can happen.

If the tab in which the addon was installed is closed, the following code throws an exception:
aWindow.QueryInterface(Ci.nsIInterfaceRequestor)
                .getInterface(Ci.nsIWebNavigation)

Error: Component returned failure code: 0x80004002 (NS_NOINTERFACE) [nsIInterfaceRequestor.getInterface]

>
> If a page wants to install something but the window has since been closed, the
> install request should just be canceled, we shouldn't be picking a random (most
> recent) tab and claiming it wants to install an addon.

Done.

I also cancel the installation if the user navigates to another URL. There's some waste, because the addons are still downloaded in the background until they are ready and are cancelled. Maybe that could be taken care of in bug 570012.

>
> Do locally-triggered addon installs go through this path? (eg File -> Open or
> drag'n'drop) They also shouldn't be associated with the tab, though I'm not
> sure what the best place is... Open the addons manager and use a different
> prompt?

Yes, the same path is used. It looks like the url argument to amIWebInstallPrompt::confirm is null in this case.

Opening the addon manager looks good to me. Do UX people have a suggestion how to handle this case?

> Something like that would also take care of the weird case of
> triggering an addon install without a navigator:browser window open (eg, it
> would open one and load about:addons?).

Apparently, you can only trigger addon installation (at least in a current nightly) by dropping an addon on a browser window (I tried dropping one on the preference or download window, but that doesn't work). So I guess such a weird case couldn't happen?

>
> >+    if (aWindow.document)
> >+      browser = chromeWin.gBrowser.getBrowserForDocument(aWindow.document);
> >+    else
> >+      browser = chromeWin.gBrowser.selectedBrowser;
>
> Similar as above, shouldn't be picking a random tab here.
>
> >+      callback: function() {
> >+        // Defer the addon installation to let the notification panel close.
> >+        // Useful for tests relying on popupshown events.
> >+        Services.tm.mainThread.dispatch({
> >+          run: function() {
> >+            aInstalls.forEach(function(install) {
> >+              install.install();
>
> I think this has partially been discussed elsewhere (maybe?), but what's
> supposed to happen UX/UI-wise in terms of showing download progress and
> handling install errors? Does the EM just handle all this?

As gavin said, download progress is bug 570012.
I'm not sure if it deals with install errors though.

>
> >+      dismissalCallback: function() {
> >+        // remove the notification, because add-ons can't be installed again
> >+        // after they are cancelled.
> >+        chromeWin.PopupNotifications.remove(notification);
> >+        aInstalls.forEach(function(install) {
> >+          install.cancel();
> >+        });
> >+      },
>
> Hmm, this breaks the "click to dismiss, click again to bring back" semantics to
> these kind of notifications. Is there a reason for doing this?

That's because the PopupNotifications module doesn't provide what we need for this semantic yet (see comment 17). I've filed bug 609804 to implement that.

>
> >+    notification = chromeWin.PopupNotifications.show(browser,
> >+                                                     "addon-install-downloaded",
> >+                                                     "<ignored>",
> >+                                                     "addons-notification-icon",
> >+                                                     mainAction, null, options);
>
>
> >+++ b/browser/locales/en-US/chrome/browser/browser.properties
> ...
> >+xpinstallAddButton=Add to %S
>
> How about just "Install"?

Sounds good to me. I also noticed that in this chart http://jboriss.files.wordpress.com/2010/11/pic9_flowchart.png the label is called "Allow". Should we use that?

> >+++ b/toolkit/mozapps/extensions/content/xpinstallConfirm.css
> >@@ -0,0 +1,4 @@
> >+installitem {
> >+  -moz-binding: url("chrome://mozapps/content/extensions/xpinstallConfirm.xml#installitem");
> >+  display: -moz-box;
> >+}
>
> display: -moz-box is the default already here, isn't it?
>

That's correct, rule removed.

> >+++ b/toolkit/mozapps/extensions/content/xpinstallConfirm.xml
> >+# The Initial Developer of the Original Code is
> >+# Ben Goodger.
> >+# Portions created by the Initial Developer are Copyright (C) 2001
> >+# the Initial Developer. All Rights Reserved.
> >+#
> >+# Contributor(s):
> >+#   Ben Goodger <[e-mail]>
>
> Really?! :) I'd suspect this is almost entirely new code, so the old
> attribution isn't needed.

Right, I started with a copy but now there isn't much original code.

>
> >+      <field name="bundle" readonly="true">
> >+        document.getAnonymousElementByAttribute(this, "anonid", "xpinstallConfirmStrings");
> >+      </field>
>
> I think we're generally avoiding <stringbundle> now, just grab this with
> Services in the constructor / init.

done.

>
> >+      <constructor><![CDATA[
> >+        let numItemsToInstall = this.installs.length;
> >+        for (let i = 0; i < numItemsToInstall; ++i) {
> >+          let install = this.installs[i];
> >+          let installItem = document.createElement("installitem");
> >+          this.itemList.appendChild(installItem);
> >+
> >+          installItem.name = install.name;
> >+          installItem.version = install.version;
> >+          installItem.url = install.sourceURI.spec;
> >+          if (install.iconURL)
> >+            installItem.icon = install.iconURL;
> >+          if (install.type)
> >+            installItem.type = install.type;
> >+          if (install.certName)
> >+            installItem.cert = install.certName;
> >+          installItem.signed = install.certName ? "true" : "false";
>
> Are these sanity / length checked by EM before they get here? [For the bits the
> extension can control, at least.]
>
> Should make sure a naughty addon can't play tricks with giant icons, or long
> description / version strings.

For the description/version strings: if they are very long, the popup can get very wide. I see that it also affects other notifications, like the "Restart Now" one.
Maybe we should put a max-width on the <popupnotification> to avoid all these issues?

For the icon, there's a max-width/height of 32x32, so hopefully that should be safe.

(In reply to comment #45)
> Fwiw, my patch for bug 587587 adds a much simpler "removeOnDismissal" flag that
> does this automatically. Also this patch looks like it hasn't been updated to
> take into account the changes in bug 597244.

Patch now depends on bug 587587 and uses that new flag (until bug 609804 is implemented).

Comment 48

[Sylvain Pasche]

Attachment: part1 (change without tests), v6

Updated patch. Still needs a few things discussed above. Note that the tests patch will require an update.

Comment 49

[Justin Dolske [:Dolske]]

(In reply to comment #47)
 
> Let's revisit that when bug 358266 is fixed?

Hmm. Maybe, but I think that's going to be relatively important.


> > I'm kind of ambivalent about having the addon's icon in the notification, 
> 
> Do you mean the generic addon icon on the left of the notification, or the icon
> that the addon author can provide?

The author-provided icon.


> That's because the PopupNotifications module doesn't provide what we need for
> this semantic yet (see comment 17). I've filed bug 609804 to implement that.

I think this bug should probably block on getting the callbacks added in bug 597244 (and followups) doing what you need.


> > >+xpinstallAddButton=Add to %S
> >
> > How about just "Install"?
> 
> Sounds good to me. I also noticed that in this chart
> http://jboriss.files.wordpress.com/2010/11/pic9_flowchart.png the label is
> called "Allow". Should we use that?

Either is fine with me, UX can have the final word on if it should be "Allow" or "Install".


> > Are these sanity / length checked by EM before they get here?
> 
> For the description/version strings: if they are very long, the popup can get
> very wide. I see that it also affects other notifications, like the "Restart
> Now" one.
> Maybe we should put a max-width on the <popupnotification> to avoid all these
> issues?

That might be a good idea. Just need to be extra-vigilant in this dialog, since there's more motivation (and potential damage) from a page tricking a user through this dialog.

Comment 50

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

>Either is fine with me, UX can have the final word on if it should be "Allow"
>or "Install".

Both of those are heading in the direction of being vague (allow what?) although not as bad as "ok."  If I remember correctly fligtar was pushing for the buttons to be consistently labeled "Add to Firefox."  That would also work well from the ux standpoint of the user reading the button first, and then moving on to the question.

Comment 51

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

Sorry, actually two different buttons in consideration here, these would be more self describing than the current proposals of "Allow" and "Allow"

"Allow add-on Installation"

and

"Add to Firefox"

Comment 52

[Sylvain Pasche]

Alex/Jennifer there were a few other ux questions raised previously, it would be great if you could give your opinion:

- from comment 42, what about not showing the author-provided icon in the notification, in order to make it less distracting?

- from comment 44, what should we do when a user drags a .xpi on the Firefox window, or uses File > Open to install a .xpi? One suggestion is to open the about:addons page (or switch to it if it is already opened on the dropped window) and show the doorhanger notification there.

Comment 53

[Justin Scott [:fligtar]]

(In reply to comment #52)
> Alex/Jennifer there were a few other ux questions raised previously, it would
> be great if you could give your opinion:
> 
> - from comment 42, what about not showing the author-provided icon in the
> notification, in order to make it less distracting?
> 

I think the icon is pretty important to show. We're starting to emphasize add-on icons more and more, which is why we increased their size in Firefox 4. Users really respond to the icons (think iPhone/app store) and we're encouraging developers to improve the quality of their icons and not use the default.

Showing the icon is not a security regression as we already show it and have been since I've been using Firefox. If we feel that 2 icons is too distracting, I'd much rather remove the static installation puzzle piece than the custom icon.

> - from comment 44, what should we do when a user drags a .xpi on the Firefox
> window, or uses File > Open to install a .xpi? One suggestion is to open the
> about:addons page (or switch to it if it is already opened on the dropped
> window) and show the doorhanger notification there.

That sounds like a good suggestion to me.

Boriss, Faaborg, thoughts?

Comment 54

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

We need the large puzzle piece icon in order to reference the anchor icon (when the user clicks outside to close and then wants to bring the notification back).

Comment 55

[Justin Dolske [:Dolske]]

(In reply to comment #53)

> I think the icon is pretty important to show.

In other places, sure. I don't think it's really needed here, and is even counterproductive. It's tied to the immediate action (clicking "Install" on the page), so users shouldn't be confused as to what's happening. Having a lightweight UI like other dialogs of this style makes the dialog less imposing, and avoids making the user think they might have done something wrong (or are making some monumental decision) because there's a big complex dialog.


> Users really respond to the icons (think iPhone/app store)

Sure, but when I click "Install" on my iPhone, I get a simple, generic "Enter your iTunes password" dialog. No icon, or even app name. :)


I should note here (although it may or may not be in scope for this bug) that I think we should move towards having 2 distinct addon install experiences: one for whitelisted sites (like AMO), where there's either no dialog (!) or a super simple confirmation, and one for non-whitelisted sites (or whitelisted but sandboxed?) that has all the scary warnings lest the site be doing something evil.

I don't know the exact numbers, but I would presume the vast, vast, overwhelming majority of users get addons through AMO. We should optimize that experience and not treat AMO as some if it might be some dodgy site run by Louisianian scammers.


> Showing the icon is not a security regression as we already show it and have
> been since I've been using Firefox.

Not a regression, but I think we can both improve the experience and security at the same time.

Comment 56

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

>where there's either no dialog (!) or a super
>simple confirmation, and one for non-whitelisted sites (or whitelisted but
>sandboxed?) that has all the scary warnings lest the site be doing something
>evil.

We are working on more significant warnings for local applications that are trying to install extensions.  We could potentially leverage that UI for non-AMO sites that are trying to install add-ons (after they get past the first level of getting permission to ask).

Comment 57

[Sylvain Pasche]

(In reply to comment #52)
> - from comment 44, what should we do when a user drags a .xpi on the Firefox
> window, or uses File > Open to install a .xpi? One suggestion is to open the
> about:addons page (or switch to it if it is already opened on the dropped
> window) and show the doorhanger notification there.

Now that the main browser chrome is hidden when viewing about:addons (bug 571970), that suggestion might be reconsidered. We could "attach" the PopupNotification to the about:addons favicon? Or does someone have another suggestion?

Comment 58

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

A locally installed extension should ideally trigger the same content area UI we are considering for extensions being pushed in by other local applications.  Details in bug 476430.  Mockup here: http://people.mozilla.com/~faaborg/files/firefox4Mockups/userAddonSelection/addOnLocalInstall-i2.png

Comment 59

[Brian King [:kinger]]

(In reply to comment #58)
> A locally installed extension should ideally trigger the same content area UI
> we are considering for extensions being pushed in by other local applications. 
> Details in bug 476430.  Mockup here:
> http://people.mozilla.com/~faaborg/files/firefox4Mockups/userAddonSelection/addOnLocalInstall-i2.png

This seems like overkill for a user-initiated action like File->Open or a D&D of an add-on. Can we try and detect those and open the doorhanger instead?

Comment 60

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

Doorhangers are designed to mediate communication between the user and a web site.  Since interacting with a local file has no relationship with the site the user is on, a doorhanger isn't the correct way to surface the notification.  For instance, let's say the user is on espn.com, having espn.com ask the question if they would like to install an extension on their hard drive is just kind of bizarre.

Additionally, considering that app tabs and the home tab are going to be chromeless, there are certain cases where the application won't even have an anchor on screen for a app-level doorhanger to originate from.

Comment 61

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

(In reply to comment #60)
> Doorhangers are designed to mediate communication between the user and a web
> site.  Since interacting with a local file has no relationship with the site
> the user is on, a doorhanger isn't the correct way to surface the notification.

Please take into account that an action can contain a couple of add-ons to install. Do we really want to open 10 different tabs if the user dropped 10 XPI files from the local filesystem? That looks bizarre too.

Comment 62

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

>Do we really want to open 10 different tabs if the user dropped 10 XPI
>files from the local filesystem? That looks bizarre too.

yes, we really want to do that.  users should be able to focus when they are making decisions on what to install, and separating the decisions out is better than 10 items stacked and all coming from espn.com.  Also, dragging 10 xpi onto Firefox is a pretty unusual case.

Comment 63

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

I honestly think we should just get rid of this "security check." Few add-ons are signed, we already ask users to confirm for XPI installs from websites that aren't on the whitelist. This ends up being a "Yes, I really really mean it" button.

IMO, useless, other than to get in the user's way.

Comment 64

[Dave Townsend [:mossop]]

After seeing the results of trying to use doorhangers for most of the install process I've decided that Alex is right and we shouldn't be using the site notification for this stuff. Instead we'll use an in-content install UI which will allow us to keep the install progress, failure and success information all in the same context.

Comment 65

[Jesse Ruderman]

Mossop, where is that work happening? I'd like to see the mockups so I can help make sure we're not creating new security holes ;)

Comment 66

[Justin Scott [:fligtar]]

I don't understand this WONTFIX. Alex's full-tab UI for a local add-on install makes sense, but we need something more lightweight for website installations, especially from AMO. I don't think Alex even argued against the doorhanger for such cases.

Comment 67

[Dave Townsend [:mossop]]

(In reply to comment #66)
> I don't understand this WONTFIX. Alex's full-tab UI for a local add-on install
> makes sense, but we need something more lightweight for website installations,
> especially from AMO. I don't think Alex even argued against the doorhanger for
> such cases.

It's based on a discussion I had with Alex last week where we agreed that it would make sense to use a common experience for installs and that the doorhangers was not right for it.

(In reply to comment #65)
> Mossop, where is that work happening? I'd like to see the mockups so I can help
> make sure we're not creating new security holes ;)

I've created bug 643020 to track the work, largely a placeholder with the initial mockups for now, not going to be commencing work on it immediately.

Comment 68

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

>It's based on a discussion I had with Alex last week where we agreed that it
>would make sense to use a common experience for installs and that the
>doorhangers was not right for it.

For context, this discussion was a spin off of the previous work on creating an interface for allowing users to opt-in to locally forced in third party installs (bug 476430).  We had to create an install flow for that case, and felt it worked pretty well for focusing the user and guiding them through the process.

Everything about the doorhanger's design was meant for lightweight, potentially unrequested, and ignorable transactions.  Actively installing software really isn't any of those three things.

Comment 69

[Justin Scott [:fligtar]]

To be perfectly clear, you are proposing the mock-up from comment #58 for all add-on installs?

Comment 70

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

Without the unchecked check-box trick that we inserted to avoid people hitting the whatever button for locally installed extensions.  A modified flow is here: https://bug643020.bugzilla.mozilla.org/attachment.cgi?id=520339

Comment 71

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

Hm, making that tab modal for some white listed sites (AMO) would be a much better experience.  One sec, new mockup coming.

Comment 72

[Brian King [:kinger]]

FYI, we've turned the work we did on this into an add-on, Pretty Install:

https://addons.mozilla.org/en-US/firefox/addon/pretty-install/