1308310 - Display "your add-on is ready" popup after installing with mozAddonManager

Status: VERIFIED FIXED

(WebExtensions :: General, defect, P3)

Description

[Andrew Swan [:aswan]]

This is the last stage in the disco pane flows detailed at https://www.figma.com/file/HrLiKUwoLQZsIUIVBKM8Wnnu/Install-Flow-showing-Permissions

This will need to be coordinated with the AMO side, we currently generate an in-content popup after installs finish and we want to make sure we stop doing that when this bug lands so that we display exactly 1 popup and not 0 or 2.

Comment 1

[Andy McKay]

In bug 1308296 I mentioned the interaction with the disco pane and maybe won't fixing bug 1264239. I think that comment might have been more relevant to this bug.

Comment 2

[Andrew Swan [:aswan]]

Attachment: mock up of confirmation dialog

Comment 3

[Andrew Swan [:aswan]]

AMO is currently generating this notification in content.  We'll need to to coordinate the rollout of having this happen inside the browser so that we don't get both notifications (and so that users with older browser do get a notification).
Stuart, how should we handle this?  I think the browser currently sends its version in a query parameter when it loads the disco pane inside about:addons, do you just want to do a version check on the server side when we know which version this will land in?  Or would you prefer a way to feature-detect whether the browser will display the notification?
Also cc chuck for a heads up that this will impact the flow for Test Pilot installs (that is, for the install of the Test Pilot extension, not for installs of individual experiments)

Comment 4

[Chuck Harmston [:chuck]]

@aswan Thanks for the heads-up! Do I understand correctly that the "Your add-on is ready" notification will be injected by use of mozAddonManager's installation methods? If so, is it something that we could opt-out of?

Comment 5

[Andrew Swan [:aswan]]

That's correct, but note this is after the installation is complete and non-modal.
Are you concerned that it will be too disruptive?  I'd really like to keep it as simple as possible by just displaying it unconditionally.

Comment 6

[Chuck Harmston [:chuck]]

Yeah, that's exactly it. We already have post-installation modals in place for both the parent add-on installation and each experiment one (which will soon use mozAddonManager as well). I don't think we want to give up that control, but multiple notifications are also pretty bad UX.

CC John, our product manager, for his thoughts.

Comment 7

[Andrew Swan [:aswan]]

Can we let the browser built-in prompt replace the one you're currently generating from content?  We'll need to coordinate the rollout just like for AMO of course...

Comment 8

[Chuck Harmston [:chuck]]

Attachment: Screen Shot 2016-10-12 at 4.56.34 PM.png

The UI we have right now pretty tailored and task-specific to Test Pilot (see this and next attachment), so I imagine that this would be a hit to our conversions, but I'll let John weigh in on that.

Comment 9

[Chuck Harmston [:chuck]]

Attachment: Screen Shot 2016-10-12 at 4.56.12 PM.png

Comment 10

[John Gruen [:jgruen]]

Andrew Swan. We'd really prefer to maintain control over the Test Pilot install flow. Would it be possible to simply check for the Test Pilot add-on ID and skip this confirmation UI?

Comment 11

[John Gruen [:jgruen]]

We'd really prefer to maintain control over the Test Pilot install flow. Would it be possible to simply check for the Test Pilot add-on ID and skip this confirmation UI?

Comment 12

[[e-mail]]

(In reply to Andrew Swan [:aswan] from comment #3)
> AMO is currently generating this notification in content.  We'll need to to
> coordinate the rollout of having this happen inside the browser so that we
> don't get both notifications (and so that users with older browser do get a
> notification).
> Stuart, how should we handle this?  I think the browser currently sends its
> version in a query parameter when it loads the disco pane inside
> about:addons, do you just want to do a version check on the server side when
> we know which version this will land in?  Or would you prefer a way to
> feature-detect whether the browser will display the notification?
> Also cc chuck for a heads up that this will impact the flow for Test Pilot
> installs (that is, for the install of the Test Pilot extension, not for
> installs of individual experiments)

We are going to need to start looking at versions anyway (for other reasons). Since that data is already exposed in the disco pane URL we can use that and change behaviour accordingly - all I'll need to know is the version that we'll need to make the change once we know it.

Comment 13

[Andrew Swan [:aswan]]

(In reply to [:jgruen] from comment #11)
> We'd really prefer to maintain control over the Test Pilot install flow.
> Would it be possible to simply check for the Test Pilot add-on ID and skip
> this confirmation UI?

I don't think that's a good solution, this is code that runs in the browser (and it theoretically part of toolkit and independent of Firefox)

Comment 14

[Mark Striemer [:mstriemer]]

I like the idea of this modal always popping up when the mozAddonManager API is used since having add-ons silently installed in the case of an XSS seems really bad. Of course not having the XSS would be better but I feel we should notify the user when something is installed.

Perhaps there could be a callback that is triggered once this modal is closed so that the TestPilot specific modal can come up after the user has been informed of where to find their add-on?

Comment 15

[John Gruen [:jgruen]]

The Test Pilot use case is pretty distinct from the Disco Pane use case. We're installing 1st party rather than 3rd party software, and building a service that has an on-boarding flow distinct from the one designed for the Disco Pane (for example we solicit user emails on install).

I like :mstriemer's idea of keying Test Pilot onboarding off of a callback from the installed modal. Seems like a good compromise and also simplifies things on our end since we are doing a version of this UI here: https://testpilot.firefox.com/onboarding 

This may be out of scope for this bug, but I'll mention it anyway. If we can avoid it, I would prefer not to force users to dismiss a pre-install dialog (seen in the mock-ups linked above), since our Terms and Conditions are presented to the user on our landing page (unlike add-ons in the Disco Pane) and (again) we're first party software.

Comment 16

[Andrew Swan [:aswan]]

(In reply to [:jgruen] from comment #15)
> I like :mstriemer's idea of keying Test Pilot onboarding off of a callback
> from the installed modal. Seems like a good compromise and also simplifies
> things on our end since we are doing a version of this UI here:
> https://testpilot.firefox.com/onboarding 

Great, in the interest of keeping this simple, I can make the install promise resolve only after the notification has been dismissed.  Mark, Chuck, will that work for you?
We also need to figure out the rollout for testpilot.  Chuck can you do verion detection a la comment 12?

> This may be out of scope for this bug, but I'll mention it anyway. If we can
> avoid it, I would prefer not to force users to dismiss a pre-install dialog
> (seen in the mock-ups linked above), since our Terms and Conditions are
> presented to the user on our landing page (unlike add-ons in the Disco Pane)
> and (again) we're first party software.

The permission prompts for mozAddonManager (bug 1308309) are only for webextensions.  Since testpilot is an SDK add-on, those won't be displayed.  Discussion about making testpilot a webextension is in bug 1280233 but there are so many other barriers to that happening that I don't think its worth being concerned about now.  If something changes over there we can revisit.

Comment 17

[[e-mail]]

:aswan what does this do in the case where a restart is required?

How is the UI represented on FFAndroid?

Comment 18

[Andrew Swan [:aswan]]

(In reply to Stuart Colville [:scolville] [:muffinresearch] from comment #17)
> :aswan what does this do in the case where a restart is required?

Good questions, I hadn't thought about the restart case, can we finesse around this by only featuring restartless multi-process capable extensions in the disco pane?  (And I assume test pilot will want to do the same)

> How is the UI represented on FFAndroid?

Another good question, I'm hoping to use PopupNotifications.jsm.  Existing notifications (eg permissions) created with that library on Android already look a lot like the mocks for this bug.  So hopefully, pretty much the same.

Comment 19

[[e-mail]]

(In reply to Andrew Swan [:aswan] from comment #18)
> (In reply to Stuart Colville [:scolville] [:muffinresearch] from comment #17)
> > :aswan what does this do in the case where a restart is required?
> 
> Good questions, I hadn't thought about the restart case, can we finesse
> around this by only featuring restartless multi-process capable extensions
> in the disco pane?  (And I assume test pilot will want to do the same)

We're starting to use this API in the AMO mobile pages which is the start of replacing the amo frontend with a new UI. Since that will be surfacing all the add-ons on AMO (albeit limited to android initially), we will need to make sure this can handle the restart case when relevant.

> 
> > How is the UI represented on FFAndroid?
> 
> Another good question, I'm hoping to use PopupNotifications.jsm.  Existing
> notifications (eg permissions) created with that library on Android already
> look a lot like the mocks for this bug.  So hopefully, pretty much the same.

Ok, sounds good.

Comment 20

[Mark Striemer [:mstriemer]]

> I can make the install promise resolve only after the notification has been dismissed

We're currently using the events to see when install is done rather than the `installObj.install()` promise [1]. This should work for us though as long as the events still trigger the same way we'd expect so we can update the button state and just move the install notice to after that promise.

[1] https://github.com/mozilla/addons-frontend/blob/d9bd5cf1cc614b2bee01ac549cb5a65757b740ca/src/disco/addonManager.js#L38-L43

Comment 21

[Andrew Swan [:aswan]]

Attachment: screen shot of work-in-progress

I've cobbled together a bunch of yet-unlanded patches from other bugs and managed to hack up something that produces the attached screenshot.  It raises a few questions though.  Separate from the questions below, I'll try to deal with the strange appearance of the OK button and get the formatting fixed for the "Your add-on is ready" header.

But on to the questions: this is all done using PopupNotifications.jsm which just does doorhanger-style dialogs with an arrow on the top of the window that must be anchored to something in the notification box (the space immediately to the left of the url).  In the attached screenshot you can see I attached the notification to the generic addon icon (the puzzle piece).  I'd like to get confirmation that that's okay (anything else is going to entail an unknown amount of additional work...)
The second question is about the detailed text in the notification.  The mock says "Now you can access {addon name} from the toolbar."  But many addons don't actually put something into the toolbar, so I think that text isn't really appropriate.  What should it be?

Comment 22

[Chuck Harmston [:chuck]]

> Great, in the interest of keeping this simple, I can make the install
> promise resolve only after the notification has been dismissed.  Mark,
> Chuck, will that work for you?
> We also need to figure out the rollout for testpilot.  Chuck can you do
> verion detection a la comment 12?

Having the promise resolve on the dismissal of the prompt is fine.

If it's not too much trouble, I'd prefer to feature-detect. We don't typically closely follow platform changes, and may not notice and adapt to e.g. a rollback that affects the version number.

Comment 23

[Markus Jaritz [:designakt] (UX)]

(In reply to Andrew Swan [:aswan] from comment #21)
> ...this is all done using PopupNotifications.jsm which
> just does doorhanger-style dialogs with an arrow on the top of the window
> that must be anchored to something in the notification box (the space
> immediately to the left of the url). In the attached screenshot you can see
> I attached the notification to the generic addon icon (the puzzle piece). 
> I'd like to get confirmation that that's okay (anything else is going to
> entail an unknown amount of additional work...)

The goal of the confirmation is to introduce users to the add-on they just added. It can do that best by hanging off of the extensions toolbar button if there is any. (If not the menu button as a fallback)
Work on this once started here (https://github.com/mozilla/addons-frontend/issues/565) but was limited to in-content because we couldn't get the changes needed into Firefox on time.

We will have to detail the messaging for all possible uses-cases after updating the install flow for WebExtension permissions. The current designs where built with focus on disco pane. So there are some cases still missing:
- extension with no toolbar icon
- extension update (with permission change)
- extension removal (something we learned doing user tests on disco pane)
- can we provide additional info (e.g. how to find the addons manager)
- can the developer extend the messaging in this dialog (to not need to force open a tab on install) (https://github.com/mozilla/addons/issues/112#issuecomment-223350744)
- theme confirmation (currently a notification bar)
- ...

(In reply to Andrew Swan [:aswan] from comment #21)
> The second question is about the detailed text in the notification.  The
> mock says "Now you can access {addon name} from the toolbar."  But many
> addons don't actually put something into the toolbar, so I think that text
> isn't really appropriate.  What should it be?

I will need to check with Scott once we have the different states. (We might have multiple messages)

Comment 24

[Andrew Swan [:aswan]]

(In reply to Markus Jaritz [:maritz] (UX) from comment #23)
> The goal of the confirmation is to introduce users to the add-on they just
> added. It can do that best by hanging off of the extensions toolbar button
> if there is any. (If not the menu button as a fallback)

I think that hanging the confirmation off the toolbar button if there is one is a good idea but why the menu button as the fallback?  Is the idea that the user will find their way to the add-ons manager from there?  I would think that a link inside the notification to the add-ons manager (or even to the details for the new add-on within the add-ons manager) would be better way to accomplish that.  The location in the screenshot from comment 21 is considerably simpler to implement, how about putting it there and including a link in the notification?

> - can the developer extend the messaging in this dialog (to not need to
> force open a tab on install)
> (https://github.com/mozilla/addons/issues/112#issuecomment-223350744)

This is a nice ideas but I think outside of the scope of the current project to get the basic permissions-for-webextension-installation flow in place.  Can you file a new bug so we can prioritize and plan the implementation work?  (putting the content into the notification is easy enough, I think the sticking point here will be defining a way for the extension author to provide the content in a way that's consistent with the current cross-browser extension standardization efforts)

Comment 25

[Andrew Swan [:aswan]]

Attachment: Bug 1308310 Post-install notification for mozAddonManager installs

Review commit: https://reviewboard.mozilla.org/r/88250/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/88250/

Comment 26

[Andrew Swan [:aswan]]

Comment on attachment 8804125 [details]
Bug 1308310 Post-install notification for mozAddonManager installs

Panos, I think AndyM has talked to you about our webextensions permissions project.  This is my first time really doing anything with XUL and my first time using PopupNotifications.jsm so I was hoping to get some feedback about the attached patch, particularly if there are things that could be done more simply.

Its kind of a pain to actually run this code, I'm running off a local frankenstein branch that consists of the contents of the elm branch plus your in-review patch for bug 1282768 snagged from reviewboard and cherry-picked patches for bugs 1091924 and 1311180 since those landed in central since the last central->elm merge.

No urgency on the feedback if you've got other things to wrap up before the 52 cutoff.

Comment 27

[Panos Astithas (he/him) [:past] (please ni?)]

Comment on attachment 8804125 [details]
Bug 1308310 Post-install notification for mozAddonManager installs

I didn't take a very thorough look, but I have some comments that should hopefully help you get things moving. And sorry for the long patch queue you have to deal with, I'm hoping to merge m-c to elm some time next week.

The main feedback I have is that for a simple prompt with just a header, a description and a couple of buttons, you don't need to add markup to popup-notifications.inc. Just using PopupNotifications.show() should be enough (see _showE10sAccessibilityWarning in nsBrowserGlue.js for a simple example). Also a new API is now available in PermissionUI.jsm that may better fit your needs. Check out the comments at the top of that file for more. I think we should even remove addon-install-confirmation-notification, but that's for bug 1201896.

If on the other hand you intend to add more markup in the prompt in the next iterations of this patch (perhaps checkboxes for individual permissions?), then a new <popupnotification> element makes sense.

Another thing I'd like to call out is that I see you are using a persistent prompt, meaning the user has to explicitly click the single button to dismiss. If that is by design, then it's fine, but in case you were just trying to be consistent with my patch, perhaps you folks should make an explicit decision about the tradeoff between ensuring the user sees the message and interrupting their workflow.

Comment 28

[Andrew Swan [:aswan]]

Attachment: mock.png

Updated mock of confirmation dialog

Comment 29

[Wil Clouser [:clouserw]]

Another note about Test Pilot support:  we're working on the ability to install the Test Pilot add-on and an experiment with one click (so, the user can just "enable tab center" instead of installing test pilot first).  If we have a bunch of pop-ups that may get confusing.

Comment 30

[John Gruen [:jgruen]]

Piggy backing off of :clouserw

We're working to implement an install flow to install test pilot and one of the experiments with a single click, and this proposal would cause some issues for us. 

We have a fairly distinct use case, and I'd really appreciate revisiting the notion that we could special case Test Pilot that we can install our test pilot and experiment add-ons without this notification.

Comment 31

[Andy McKay]

We spoke about this in a Vidyo meeting and my recollection of it was, we'll add in a callback so that you can know when the user has clicked OK. This would prevent multiple notifications to a user.

Has that plan changed?

Comment 32

[Markus Jaritz [:designakt] (UX)]

Sorry for the late reply, but we talked about this already.
So here the summary of what we agreed on:

(In reply to Andrew Swan [:aswan] from comment #24)
> (In reply to Markus Jaritz [:maritz] (UX) from comment #23)
> > The goal of the confirmation is to introduce users to the add-on they just
> > added. It can do that best by hanging off of the extensions toolbar button
> > if there is any. (If not the menu button as a fallback)
> 
> I think that hanging the confirmation off the toolbar button if there is one
> is a good idea but why the menu button as the fallback?  Is the idea that
> the user will find their way to the add-ons manager from there?  I would
> think that a link inside the notification to the add-ons manager (or even to
> the details for the new add-on within the add-ons manager) would be better
> way to accomplish that.  The location in the screenshot from comment 21 is
> considerably simpler to implement, how about putting it there and including
> a link in the notification?

For now we will keep the confirmation where it is ( hanging from the left side of the url bar), and treat hanging it from the extensions button in the toolbar as a follow-up.
The reason for moving it away from the url-bar is to be able to have it directly point to the button in the toolbar that got added (to help the users notice it), and then provide a fallback for extensions without a button. I chose the menu button as a fallback as it is closest to where confirmations will appear for extensions with a toolbar-button. (And because it indicates where to find the add-on manager.)

> 
> > - can the developer extend the messaging in this dialog (to not need to
> > force open a tab on install)
> > (https://github.com/mozilla/addons/issues/112#issuecomment-223350744)
> 
> This is a nice ideas but I think outside of the scope of the current project
> to get the basic permissions-for-webextension-installation flow in place. 
> Can you file a new bug so we can prioritize and plan the implementation
> work?  (putting the content into the notification is easy enough, I think
> the sticking point here will be defining a way for the extension author to
> provide the content in a way that's consistent with the current
> cross-browser extension standardization efforts)

Yes, definitely a follow-up.

Comment 33

[Jonathan Kingston [:jkt] he/him]

Attachment: Selection_429.png

Will this be landing before Aurora? The final prompt from the install addon pane currently has a blank call to action that doesn't do anything.

Comment 34

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Jonathan Kingston [:jkt] from comment #33)
> Will this be landing before Aurora? The final prompt from the install addon
> pane currently has a blank call to action that doesn't do anything.

This is bug 1315236.

Comment 35

[Andrew Swan [:aswan]]

Comment on attachment 8804125 [details]
Bug 1308310 Post-install notification for mozAddonManager installs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/88250/diff/1-2/

Comment 36

[Robert Helmer [:rhelmer]]

Comment on attachment 8804125 [details]
Bug 1308310 Post-install notification for mozAddonManager installs

https://reviewboard.mozilla.org/r/88250/#review107564

::: toolkit/mozapps/extensions/AddonManager.jsm:3004
(Diff revision 2)
> -          AddonManager.webAPI.copyProps(install, data);
> +            AddonManager.webAPI.copyProps(install, data);
> -          this.sendEvent(mm, data);
> +            this.sendEvent(mm, data);
> +            if (event == "onInstallEnded") {
> +              resolve(addon);
> +            } else if (event == "onInstallFailed") {
> +              reject({message: "XXX"});

Did you mean to replace this "XXX" message?

::: toolkit/mozapps/extensions/AddonManager.jsm:3007
(Diff revision 2)
> +              resolve(addon);
> +            } else if (event == "onInstallFailed") {
> +              reject({message: "XXX"});
> +            } else if (event == "onInstallCancelled") {
> +              reject({message: "install cancelled"});
> -        }
> +            }

Is it worth having an `else` here in case something unexpected happens? I guess the test will hang if that happens so it's not fatal but a little more annoying to debug.

Comment 37

[Andrew Swan [:aswan]]

Comment on attachment 8804125 [details]
Bug 1308310 Post-install notification for mozAddonManager installs

https://reviewboard.mozilla.org/r/88250/#review107564

> Did you mean to replace this "XXX" message?

whoops, sure did.  fix coming.

> Is it worth having an `else` here in case something unexpected happens? I guess the test will hang if that happens so it's not fatal but a little more annoying to debug.

Some of those events aren't final so we don't want to settle the promise (onDownloadProgress, onDownloadEnded for instance) but yeah I should probably catch some of the other terminal events.  Will push an update.

Comment 38

[Andrew Swan [:aswan]]

Comment on attachment 8804125 [details]
Bug 1308310 Post-install notification for mozAddonManager installs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/88250/diff/2-3/

Comment 39

[Andrew Swan [:aswan]]

Comment on attachment 8804125 [details]
Bug 1308310 Post-install notification for mozAddonManager installs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/88250/diff/3-4/

Comment 40

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f8ee44e0d4c6
Post-install notification for mozAddonManager installs r=rhelmer

Comment 41

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/f8ee44e0d4c6

Comment 42

[Vasilica Tamas, QA [:vtamas]]

Confirm that permission confirmation doorhanger is successfully displayed while installing from disco pane on Firefox 54.0a1 (2017-01-30/31) under Windows 10 64-bit and Ubuntu 16.04 32-bit.

During testing I also noticed the following: 
  - The old Disco Panel confirmation (Your add-on is ready) is still displayed - filed a github bug for this issue: https://github.com/mozilla/addons-frontend/issues/1679

  - The new confirmation pop-up is not so visible, because after installation, the focus is automatically changed to add-on confirmation tab. After returning back, the doorhanger is dismissed and I am not so sure how noticeable is the puzzle icon from toolbar: https://www.screencast.com/t/eVuSPqrecsG 
 
Could this confirmation tab be somehow delayed in order to give enough time for permission doorhangers to be observed by users? Or is there a way to maintain displayed the pop-up though the focus is switched to another tab?

Comment 43

[Andrew Swan [:aswan]]

(In reply to Vasilica Mihasca, QA [:vasilica_mihasca] from comment #42)
>   - The new confirmation pop-up is not so visible, because after
> installation, the focus is automatically changed to add-on confirmation tab.

This is tracked in bug 1334354

Comment 44

[Vasilica Tamas, QA [:vtamas]]

(In reply to Andrew Swan [:aswan] from comment #43)
> This is tracked in bug 1334354

Add-on confirmation page differs from contribution page and it is displayed in a new tab. For example DownThemAll! add-on displays both contribution and confirmation pages: https://www.screencast.com/t/iLb09iziR . Could both pages be tracked in the same bug?

Comment 45

[Andrew Swan [:aswan]]

Sure, feel free to update bug 1334354 to mention that.

Comment 46

[Vasilica Tamas, QA [:vtamas]]

Based on Comment 42 and Comment 45, I am marking this bug as Verified since the other issues are tracked separately.