1201896 - Update popup notifications for add-on installation

Status: VERIFIED FIXED

(Firefox :: Site Identity, defect, P1)

Description

[Ash Grigas]

Attachment: add-on notification wireframe.png

Attaching an initial wireframe of the basic components of the new design. Please review for any additional states needed specific to the add-on installation flow from a direct url.

Comment 1

[Ash Grigas]

Think we should consider whether this is really a permission or not given our new design handling permissions since the likelihood a users wants to 'allow always' the installation of add-ons from a specific url is quite an edge case and could be left as is in the more info modal versus having to bring it into the control center.

Comment 2

[Markus Jaritz [:designakt] (UX)]

As I understand this is about 2 topics:

1) Redesigning the add-on install flow to match the new notification style. -> I think we should do that, even if we just redesign them. Having doorhangers be too different will create a feeling of some parts being outdated.


2) Deciding if the doorhanger asking for permission to ask to install and add-on should be handled like other permissions.
I agree that an "allow always" is a very specific edge case. When reconsidering permissions it is indeed a question whether this permission is needed or really is a permission. With other permissions on allowing, one will immediately share private data (location, cam, mic) or execute some code (flash). With this message one will simply allow to be ask, if one wants to install an add-on. So it is not a permission to install add-ons but a permission to ask to install. Considering this I feel this question could be omitted completely, as  but I would like Daniels input on that.


If Daniel has security concerns I think we should make it a permission similar to the others.
https://mozilla.invisionapp.com/share/PC3Y0QSRK#/screens/98827154

Comment 3

[Ash Grigas]

(In reply to Markus Jaritz [:maritz] (UX) from comment #2)
> As I understand this is about 2 topics:
> 
> 1) Redesigning the add-on install flow to match the new notification style.
> -> I think we should do that, even if we just redesign them. Having
> doorhangers be too different will create a feeling of some parts being
> outdated.
> 
> 
> 2) Deciding if the doorhanger asking for permission to ask to install and
> add-on should be handled like other permissions.
> I agree that an "allow always" is a very specific edge case. When
> reconsidering permissions it is indeed a question whether this permission is
> needed or really is a permission. With other permissions on allowing, one
> will immediately share private data (location, cam, mic) or execute some
> code (flash). With this message one will simply allow to be ask, if one
> wants to install an add-on. So it is not a permission to install add-ons but
> a permission to ask to install. Considering this I feel this question could
> be omitted completely, as  but I would like Daniels input on that.
> 
> 
> If Daniel has security concerns I think we should make it a permission
> similar to the others.
> https://mozilla.invisionapp.com/share/PC3Y0QSRK#/screens/98827154

Correct, 1 is already agreed upon and just need an answer as to which team will implement it. Does your team have scope to do work on it during 44 release with launch in 45? If not, our team can take it.

Per item 2, just to clarify - if the user changes the default to 'Allow' from the rarely used more info dialogue, my understanding is that it would never ask them and just install the Add-on right away. So from a security perspective, uplifting access to do this seems risky. The proposal would keep the user's decision for the session only and would re-ask on subsequent visit to that direct install url. 

Please review the updates to the other permissions. You will see they do not have directly editable settings anymore in the control center, they merely have a 'clear setting control' by clicking the 'X' which will prompt an ask the next time the site requests that permission.

Comment 4

[Daniel Veditz [:dveditz]]

CC'ing Dave since he's the module owner for this code and may have different ideas.

There are two install-related door-hangers. The one in attachment 8657119 [details] is what sites get if the user has left the default setting for "Warn me when sites try to install add-ons" on the Security preference pane, unless the site has been added to the Exceptions list (default: AMO and Marketplace). Even if you uncheck that or add a site to the exceptions you still always get a second door-hanger showing download progress and the results of the signaure check which then gives the user an option to cancel or install.

I don't want to make it any easier for users to add sites to the exception list--no "always remember" checkbox as shown in the invisionapp.com link in comment 2, nor showing up as a normal "Permission" to fiddle with from the site permissions list in the door hanger. Even though there's always the second dialog, when it's too easy users make completely unwarranted assumptions about add-ons being "safe" because it's part of the browser. ADD-ONS ARE NOT SAFE. Even reviewed and signed add-ons may do powerful and damaging things if used in the wrong context.

My original intent for the pref behind the "warn" checkbox (which was buried in about:config) was that installing would not be allowed AT ALL from any site not on the exception list, forcing users to download the file and "launch" it from the local disk as they do for random "setup.exe" files--the (in)security is the same for both. The front end folks rejected that approach early on in favor of the "allow?" dialog. I still think it's too easy, but at least the signing requirement will take the edge off the worst ones.

I don't understand comment 3, what is the 'rarely used more info dialog'? Install exceptions are not a permission, my preference would be to keep it tucked away on the Security preference pane.

Comment 5

[Ash Grigas]

(In reply to Daniel Veditz [:dveditz] from comment #4)
> CC'ing Dave since he's the module owner for this code and may have different
> ideas.
> 
> There are two install-related door-hangers. The one in attachment 8657119 [details]
> [details] is what sites get if the user has left the default setting for
> "Warn me when sites try to install add-ons" on the Security preference pane,
> unless the site has been added to the Exceptions list (default: AMO and
> Marketplace). Even if you uncheck that or add a site to the exceptions you
> still always get a second door-hanger showing download progress and the
> results of the signaure check which then gives the user an option to cancel
> or install.
> 
> I don't want to make it any easier for users to add sites to the exception
> list--no "always remember" checkbox as shown in the invisionapp.com link in
> comment 2, nor showing up as a normal "Permission" to fiddle with from the
> site permissions list in the door hanger. Even though there's always the
> second dialog, when it's too easy users make completely unwarranted
> assumptions about add-ons being "safe" because it's part of the browser.
> ADD-ONS ARE NOT SAFE. Even reviewed and signed add-ons may do powerful and
> damaging things if used in the wrong context.
> 
> My original intent for the pref behind the "warn" checkbox (which was buried
> in about:config) was that installing would not be allowed AT ALL from any
> site not on the exception list, forcing users to download the file and
> "launch" it from the local disk as they do for random "setup.exe" files--the
> (in)security is the same for both. The front end folks rejected that
> approach early on in favor of the "allow?" dialog. I still think it's too
> easy, but at least the signing requirement will take the edge off the worst
> ones.
> 
> I don't understand comment 3, what is the 'rarely used more info dialog'?
> Install exceptions are not a permission, my preference would be to keep it
> tucked away on the Security preference pane.

Thanks for the context Dan. The more info modal is here: https://www.dropbox.com/s/1kek5xp1xcqvx6z/Screenshot%202015-09-11%2012.18.05.png?dl=0

In it you can see is a permissions pref to Allow or Block. Default is block. Based on your feedback, we will no bring this permission into the control center panel since its not likely a user would change it to 'allow' nor that we would want them to. That said, should it be removed from the modal as well? 

For urls (not including AMO/Marketplace) it seems like the default should actually be reflected as 'Ask' not 'Block' since we don't proactively block install we ask users if they want to Allow it or Block it.

Are you ok going forward with the following:
1) changing default label to Ask (if we keep this as a permission in the Tools>Page info modal)
2) updating the initial 'ask' notification style to match the notification design (wireframe above) we're rolling out in our control center updates? (no checkbox to remember choice since we don't want to have the selection persist beyond the sessions)

Last thing - if our eng team updates the 'ask' notification - could you apply the style to the other doorhangers once they click 'allow' in the install flow?

Comment 6

[Ash Grigas]

(In reply to agrigas from comment #5)
> (In reply to Daniel Veditz [:dveditz] from comment #4)
> > CC'ing Dave since he's the module owner for this code and may have different
> > ideas.
> > 
> > There are two install-related door-hangers. The one in attachment 8657119 [details]
> > [details] is what sites get if the user has left the default setting for
> > "Warn me when sites try to install add-ons" on the Security preference pane,
> > unless the site has been added to the Exceptions list (default: AMO and
> > Marketplace). Even if you uncheck that or add a site to the exceptions you
> > still always get a second door-hanger showing download progress and the
> > results of the signaure check which then gives the user an option to cancel
> > or install.
> > 
> > I don't want to make it any easier for users to add sites to the exception
> > list--no "always remember" checkbox as shown in the invisionapp.com link in
> > comment 2, nor showing up as a normal "Permission" to fiddle with from the
> > site permissions list in the door hanger. Even though there's always the
> > second dialog, when it's too easy users make completely unwarranted
> > assumptions about add-ons being "safe" because it's part of the browser.
> > ADD-ONS ARE NOT SAFE. Even reviewed and signed add-ons may do powerful and
> > damaging things if used in the wrong context.
> > 
> > My original intent for the pref behind the "warn" checkbox (which was buried
> > in about:config) was that installing would not be allowed AT ALL from any
> > site not on the exception list, forcing users to download the file and
> > "launch" it from the local disk as they do for random "setup.exe" files--the
> > (in)security is the same for both. The front end folks rejected that
> > approach early on in favor of the "allow?" dialog. I still think it's too
> > easy, but at least the signing requirement will take the edge off the worst
> > ones.
> > 
> > I don't understand comment 3, what is the 'rarely used more info dialog'?
> > Install exceptions are not a permission, my preference would be to keep it
> > tucked away on the Security preference pane.
> 
> Thanks for the context Dan. The more info modal is here:
> https://www.dropbox.com/s/1kek5xp1xcqvx6z/Screenshot%202015-09-11%2012.18.05.
> png?dl=0
> 
> In it you can see is a permissions pref to Allow or Block. Default is block.
> Based on your feedback, we will no bring this permission into the control
> center panel since its not likely a user would change it to 'allow' nor that
> we would want them to. That said, should it be removed from the modal as
> well? 
> 
> For urls (not including AMO/Marketplace) it seems like the default should
> actually be reflected as 'Ask' not 'Block' since we don't proactively block
> install we ask users if they want to Allow it or Block it.
> 
> Are you ok going forward with the following:
> 1) changing default label to Ask (if we keep this as a permission in the
> Tools>Page info modal)
> 2) updating the initial 'ask' notification style to match the notification
> design (wireframe above) we're rolling out in our control center updates?
> (no checkbox to remember choice since we don't want to have the selection
> persist beyond the sessions)
> 
> Last thing - if our eng team updates the 'ask' notification - could you
> apply the style to the other doorhangers once they click 'allow' in the
> install flow?

One more thought - we could also leave the styling as is and just change the copy and button labels to match formatting for the new notifications. If add-ons arent considered a permission and still have the same url bar behavior, may be simpler to just detach them from our work.

Comment 7

[Markus Jaritz [:designakt] (UX)]

So we will leave the dialogues as they are and not have the question for self-hosted add-ons be part of regular permissions.

(In reply to agrigas from comment #3)
> Correct, 1 is already agreed upon and just need an answer as to which team
> will implement it. Does your team have scope to do work on it during 44
> release with launch in 45? If not, our team can take it.

Dave, when all permission doorhangers will be redesigned it would be great to update the add-on install flow to the new design as well. Dave, do you think there is time for updating our design if the privacy team provides code for a template-doorhanger of their new doorhanger?

Dan, if self-hosted add-ons might be as harmful as you mention, we should consider to make it more difficult for users to install them (or block them completely), as users will probably go a long way to install something if they really want it (and the remotely trust the site prompting them to).
Maybe setting the default action to "Do not Allow" might be a step towards that. Or stronger measurements. But I can not estimate the threat-level of this.

Comment 8

[Dave Townsend [:mossop]]

(In reply to Markus Jaritz [:maritz] (UX) from comment #7)
> So we will leave the dialogues as they are and not have the question for
> self-hosted add-ons be part of regular permissions.
> 
> (In reply to agrigas from comment #3)
> > Correct, 1 is already agreed upon and just need an answer as to which team
> > will implement it. Does your team have scope to do work on it during 44
> > release with launch in 45? If not, our team can take it.
> 
> Dave, when all permission doorhangers will be redesigned it would be great
> to update the add-on install flow to the new design as well. Dave, do you
> think there is time for updating our design if the privacy team provides
> code for a template-doorhanger of their new doorhanger?

I've got not time available for this right now but this might be something that falls under the remit of the new add-ons team, Kev or Andy might be able to comment.

Comment 9

[Andy McKay]

It sure does.

Comment 10

[Daniel Veditz [:dveditz]]

(In reply to Markus Jaritz [:maritz] (UX) from comment #7):

I've raised my concerns in comment 4, but for actual product decisions and how to weigh competing concerns I feel I should defer to Andy and Kev.

Comment 11

[:Gijs (he/him)]

Move control center bugs, filter on "de-generalize-control-center"

Comment 12

[Johann Hofmann [:johannh]]

We should probably track this bug in fxprivacy. I'm not sure what the latest status on add-on prompts was. I think we transitioned them to new layout but some states like the progress bar could use some fresh designs. Is that correct or can we close this bug?

Comment 13

[Andy McKay]

:elan, judging by the last comment it looks like someone is working on this?

A few things have changed since this started, primarily the new discovery pane API and secondly we are planning on working on WebExtensions permissions. Might be good to chat about where this one is going.

Comment 14

[Erin Lancaster [:elan]]

:andym, sounds like your team has this covered so I'll leave this as work external to the project I'm tracking. let me know if this doesn't sound correct. thank you.

Comment 15

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

Erin I was wrong yesterday about this bug, I believe it actually is part of the work we need to do in our project's MVP. I was confused after my discussion with Andy and assumed this was the bug that was tracking the WebExtensions permission work, which is in fact tracked in bug 1197420. So we need to work on this immediately after bug 1282768, which updates the rest of the doorhangers.

Comment 16

[Erin Lancaster [:elan]]

Note: this will have to happen before we merge elm to m-c.

Comment 17

[:Paolo Amadini]

We're simply updating the add-on code to use the built-in buttons instead of the custom ones.

Comment 18

[:Paolo Amadini]

Attachment: Bug 1201896 - Update popup notifications for add-on installation.

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

Comment 19

[:Paolo Amadini]

Is one of you the right reviewer for the attached patch?

Comment 20

[Andy McKay]

I think aswan might be the best person to help on this one.

Comment 21

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

I took a quick look at the changes related to the refactoring I worked on and they look good to me. aswan should be a better reviewer for the rest, but I'm happy to review the entire thing if he is swamped.

Comment 22

[Andrew Swan [:aswan]]

The changes in this patch appear to be all front-end related which I'm really not qualified to give meaningful feedback on.

Comment 23

[:Paolo Amadini]

Comment on attachment 8806755 [details]
Bug 1201896 - Update popup notifications for add-on installation.

https://reviewboard.mozilla.org/r/90084/#review90030

Panos, I guess you won this review!

I have some questions that are probably best answered by someone familiar with the add-ons installation flow, though.

::: browser/base/content/browser-addons.js:316
(Diff revision 1)
> +              if (Preferences.get("xpinstall.customConfirmationUI", false)) {
> +                element.setAttribute("mainactiondisabled", "true");
> +              } else {
> +                element.button.hidden = true;
> +              }

Should I test non-default values of xpinstall.customConfirmationUI?

::: browser/base/content/browser-addons.js:338
(Diff revision 1)
> +          for (let install of installInfo.installs) {
> +            if (install.state != AddonManager.STATE_CANCELLED) {
> +              install.cancel();
> +            }
> +          }

Is this correct as a replacement...

::: browser/base/content/popup-notifications.inc
(Diff revision 1)
> -      <button id="addon-progress-cancel"
> -              oncommand="this.parentNode.cancel();"/>

...for this?

Comment 24

[:Paolo Amadini]

Dave, maybe you can help with the questions in comment 23?

Comment 25

[Dave Townsend [:mossop]]

(In reply to :Paolo Amadini from comment #23)
> Comment on attachment 8806755 [details]
> Bug 1201896 - Update popup notifications for add-on installation.
> 
> https://reviewboard.mozilla.org/r/90084/#review90030
> 
> Panos, I guess you won this review!
> 
> I have some questions that are probably best answered by someone familiar
> with the add-ons installation flow, though.
> 
> ::: browser/base/content/browser-addons.js:316
> (Diff revision 1)
> > +              if (Preferences.get("xpinstall.customConfirmationUI", false)) {
> > +                element.setAttribute("mainactiondisabled", "true");
> > +              } else {
> > +                element.button.hidden = true;
> > +              }
> 
> Should I test non-default values of xpinstall.customConfirmationUI?

No I think we can say that non-default values aren't supported in Firefox.

> ::: browser/base/content/browser-addons.js:338
> (Diff revision 1)
> > +          for (let install of installInfo.installs) {
> > +            if (install.state != AddonManager.STATE_CANCELLED) {
> > +              install.cancel();
> > +            }
> > +          }
> 
> Is this correct as a replacement...
> 
> ::: browser/base/content/popup-notifications.inc
> (Diff revision 1)
> > -      <button id="addon-progress-cancel"
> > -              oncommand="this.parentNode.cancel();"/>
> 
> ...for this?

Looks like it.

Comment 26

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

Paolo, could you push to try so that I can play with the patch on macOS and Windows?

Comment 27

[:Paolo Amadini]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c898bad61be1f7e8eddc0b83d10662207e0d4f21

Comment 28

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

Comment on attachment 8806755 [details]
Bug 1201896 - Update popup notifications for add-on installation.

https://reviewboard.mozilla.org/r/90084/#review90312

This looks good to me and works as advertised. There are two issues with it that can be dealt with in a followup bug.

The first one is the behavior of the last notification in the installation flow that doesn't have any buttons or actions. In that particular case the empty button row seems wrong, so we should just hide it entirely. Another approach would be to add a Dimsiss/OK button to provide a means of closing the prompt apart from the "x" (in that case maybe also remove the "x" for consistency with other prompts?).

The second is the appearance when only a single button/action is present (not two buttons where one of them is disabled). There is no mention in the spec for this case, but a separate spec from the add-ons UX team that I saw yesterday indicates that it should stretch to consume the entire row.

::: browser/base/content/browser-addons.js:195
(Diff revision 1)
> -    let acceptButton = document.getElementById("addon-install-confirmation-accept");
> -    acceptButton.label = gNavigatorBundle.getString("addonInstall.acceptButton.label");
> -    acceptButton.accessKey = gNavigatorBundle.getString("addonInstall.acceptButton.accesskey");
> +    };
> +
> +    let secondaryAction = {
> +      label: gNavigatorBundle.getString("addonInstall.cancelButton.label"),
> +      accessKey: gNavigatorBundle.getString("addonInstall.cancelButton.accesskey"),
> +      callback: () => {},

Can't you just use cancelInstallation here for consistency and remove the event handler for "removed"?

::: browser/base/content/browser-addons.js:313
(Diff revision 1)
>        options.contentWindow = browser.contentWindow;
>        options.sourceURI = browser.currentURI;
> -      options.eventCallback = (aEvent) => {
> +      options.eventCallback = function (aEvent) {
>          switch (aEvent) {
> +          case "shown":
> +            let element = [...this.owner.panel.childNodes]

I found myself having to read the code closely to figure out which element this was. How about renaming it to |notification|? This way |notification.button.hidden| is straightforward to read and consistent with other places.

Comment 29

[:Paolo Amadini]

(In reply to Panos Astithas [:past] from comment #28)
> The first one is the behavior of the last notification in the installation
> flow that doesn't have any buttons or actions.

Ah, that was a case I didn't test manually, because by chance all the random add-ons I tried required a restart. I've filed bug 1315236 where we can figure out what to do for all the consumers in this case.

> The second is the appearance when only a single button/action is present
> (not two buttons where one of them is disabled). There is no mention in the
> spec for this case, but a separate spec from the add-ons UX team that I saw
> yesterday indicates that it should stretch to consume the entire row.

The current look is clearer to me, but we can easily switch to what we need.

> Can't you just use cancelInstallation here for consistency and remove the
> event handler for "removed"?

I believe the purpose was to clean up in case the tab was closed, so I left the code alone. Instead, if the user already accepted the installation, removing the progress notification shouldn't stop the download.

> I found myself having to read the code closely to figure out which element
> this was. How about renaming it to |notification|? This way
> |notification.button.hidden| is straightforward to read and consistent with
> other places.

Renamed to notificationElement, because in the rest of the code "notification" refers to the Notification object.

Comment 30

[:Paolo Amadini]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=549a69164489fc17e91ee3720bff3b96b2a60374

Comment 31

[Markus Jaritz [:designakt] (UX)]

Using the latest try build on Win10, I noticed that the install button is active while downloading. In the current flow it is disabled while the download is still in progress.
Please use the disabled state in the new implementation to keep the button behavior as it is. Thanks.

Comment 32

[:Paolo Amadini]

(In reply to Markus Jaritz [:maritz] (UX) from comment #31)
> Using the latest try build on Win10, I noticed that the install button is
> active while downloading. In the current flow it is disabled while the
> download is still in progress.

Thanks for noticing this bug! I've introduced it accidentally when renaming a variable. I've now added a regression test that checks that the button is indeed disabled.

Comment 33

[:Paolo Amadini]

Comment on attachment 8806755 [details]
Bug 1201896 - Update popup notifications for add-on installation.

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

Comment 34

[:Paolo Amadini]

Comment on attachment 8806755 [details]
Bug 1201896 - Update popup notifications for add-on installation.

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

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e3bef583561
Update popup notifications for add-on installation. r=past

Comment 36

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2e3bef583561

Comment 38

[magicp]

Attachment: adobe-flash.png

Hello,
Is this notification out of scope? If not, I will file this. Thanks.

Comment 39

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

Plugin prompts will be handled in bug 1290912.

Comment 40

[Mohammad Maruf Rahman]

I have reproduced this bug with Nightly 43.0a1 (2015-09-04) on Windows 7 , 64 Bit !

This bug's fix is verified with latest Nightly 

Build   ID   20161130030206
User Agent   Mozilla/5.0 (WindowsNT 6.1; Win64; x64; rv:53.0) Gecko/20100101 Firefox/53.0
[bugday-20161130]

Comment 41

[Md. Majedul islam]

I have reproduced this bug with Nightly Nightly 43.0a1 (2015-09-04) (64-bit) on Ubuntu 16.10, 64 Bit !

This bug's fix is verified with latest Nightly

Build   ID   20161130030206

User Agent   Mozilla/5.0 (X11; Linux x86_64; rv:53.0) Gecko/20100101 Firefox/53.0

[bugday-20161130]