1308296 - Display "your add-on is ready" popup after installing a webextension

Status: VERIFIED FIXED

(WebExtensions :: General, defect, P5)

Description

[Andrew Swan [:aswan]]

This is the final step in the third party flow (and other flows) detailed at https://www.figma.com/file/HrLiKUwoLQZsIUIVBKM8Wnnu/Install-Flow-showing-Permissions

Comment 1

[Andy McKay]

Bug 1264239 on exposing the UITour to the disco pane. The requirement for showing this popup means that we need to show it all over the place. As such that bug is probably pointless and should be replaced with this one.

Comment 2

[Andy McKay]

Attachment: Screenshot 2016-10-06 10.03.57.png

Comment 3

[Andrew Swan [:aswan]]

Attachment: Bug 1308296 Show post-install notification

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

Comment 4

[Andrew Swan [:aswan]]

Attachment: mock

Comment 5

[Andrew Swan [:aswan]]

This is the last big piece for permissions.  It is missing a test, but I'd like to get it landed like this with the leave-open keyword until the test lands, so that the strings are in central in time for the 53 uplift...

Comment 6

[Florian Quèze [:florian]]

Comment on attachment 8828648 [details]
Bug 1308296 Show post-install notification

https://reviewboard.mozilla.org/r/105992/#review107028

Looks mostly good, but there are enough small issues that I should have another look at the updated patch before it lands.

::: browser/locales/en-US/chrome/browser/browser.properties:124
(Diff revision 1)
>  # for which this webextension is requesting permission.
>  webextPerms.hostDescription.tooManySites=Access your data on #1 other site;Access your data on #1 other sites
>  
> +# LOCALIZATION NOTE (addonPostInstall.message)
> +# %1$S is replaced with the localized named of the extension that was
> +# just installed.  %2$S is replaced with the localized name of the application.

nit: this double space feels weird here, maybe put a line break?

::: browser/locales/en-US/chrome/browser/browser.properties:133
(Diff revision 1)
> +# %1$S is replaced with the localized name of the extension.
> +# %2$S is replaced with the icon for the add-ons menu.
> +# %3$S is replaced with the icon for the toolbar menu.
> +# Note, this string will be used as raw markup. Avoid characters like <, >, &
> +addonPostInstall.message2=Manage %1$S by clicking %2$S in the %3$S menu.
> +addonPostInstall.okay.label=OK!

None of the existing OK button has a '!', so I would suggest removing it here to be consistent.

http://searchfox.org/mozilla-central/search?q=%3DOK&path=.properties

::: browser/modules/ExtensionsUI.jsm:25
(Diff revision 1)
>                                    "resource://gre/modules/Services.jsm");
>  
>  XPCOMUtils.defineLazyPreferenceGetter(this, "WEBEXT_PERMISSION_PROMPTS",
>                                        "extensions.webextPermissionPrompts", false);
>  
> -const DEFAULT_EXENSION_ICON = "chrome://mozapps/skin/extensions/extensionGeneric.svg";
> +const DEFAULT_EXTENSION_ICON = "chrome://mozapps/skin/extensions/extensionGeneric.svg";

Ouch, I should have caught this before :-(

::: browser/modules/ExtensionsUI.jsm:324
(Diff revision 1)
> +  showInstallNotification(target, addon) {
> +    let win = target.ownerGlobal;
> +    let popups = win.PopupNotifications;
> +
> +    let addonLabel = `<label class="addon-webext-name">${addon.name}</label>`;
> +    let addonIcon = `<image class="addon-addon-icon"/>`;

Let's use backquotes only if there's a variable to replace. If you are avoiding the usual double quotes here to not have to escape the " characters within the string, you can use '.
Applies to the next line too.

::: browser/modules/ExtensionsUI.jsm:330
(Diff revision 1)
> +    let toolbarIcon = `<image class="addon-toolbar-icon"/>`;
> +
> +    let brandBundle = win.document.getElementById("bundle_brand");
> +    let appName = brandBundle.getString("brandShortName");
> +
> +    let msg1 = win.gNavigatorBundle.getFormattedString("addonPostInstall.message1",

let bundle = win.gNavigatorBundle;

::: browser/modules/ExtensionsUI.jsm:335
(Diff revision 1)
> +    let msg1 = win.gNavigatorBundle.getFormattedString("addonPostInstall.message1",
> +                                                       [addonLabel, appName]);
> +    let msg2 = win.gNavigatorBundle.getFormattedString("addonPostInstall.message2",
> +                                                       [addonLabel, addonIcon, toolbarIcon]);
> +    let okay = win.gNavigatorBundle.getString("addonPostInstall.okay.label");
> +    let okayKey = win.gNavigatorBundle.getString("addonPostInstall.okay.key");

I don't find these 2 variables very helpful, I think it would be more readable to have:

let action = {
  label: bundle.getString(...)
  accessKey: ...
  callback: () => {}
};

::: browser/modules/ExtensionsUI.jsm:345
(Diff revision 1)
> +                  label: okay,
> +                  accessKey: okayKey,
> +                  callback: () => {},
> +                },
> +                null,
> +                {

I would also recommend putting this in an options variable to reduce the indent level, but I guess it's mostly a matter of style.

::: browser/modules/ExtensionsUI.jsm:349
(Diff revision 1)
> +                null,
> +                {
> +                  hideClose: true,
> +                  popupIconURL: addon.iconURL || DEFAULT_EXTENSION_ICON,
> +                  eventCallback(topic) {
> +                    if (topic == "shown") {

I think code doing these kind of manipulations on the panel usually do it on the "showing" event rather than on the "shown" one.

That said, I can't really make a convincing argument for why using 'shown' would cause issues. I suspect it may be more likely to cause flickering if you do the changes once the panel is already visible on screen; but I'm not sure that actually happens.

::: browser/modules/ExtensionsUI.jsm:350
(Diff revision 1)
> +                {
> +                  hideClose: true,
> +                  popupIconURL: addon.iconURL || DEFAULT_EXTENSION_ICON,
> +                  eventCallback(topic) {
> +                    if (topic == "shown") {
> +                      win.document

Use a 'doc' variable to slightly reduce duplication:
let doc = win.document;

Also, it doesn't matter in this case because the notification isn't swappable to another window, but I would have a slight preference for:
let doc = this.browser.ownerDocument;

(mostly for the sake of avoiding issues if someone copy/pastes this code to then write the code of a notification that will persist across window changes)

::: browser/themes/osx/browser.css:3104
(Diff revision 1)
>    font-weight: bold;
>    margin: 0;
>  }
>  
> +.addon-addon-icon {
> +    width: 14px;

Please fix the indent in your additions to this file.

Also, you'll need similar changes for Windows and Linux.

::: toolkit/mozapps/extensions/AddonManager.jsm:2114
(Diff revision 1)
>              install.addon.appDisabled == false) {
>                install.addon.userDisabled = false;
>          }
> +
> +        if (WEBEXT_PERMISSION_PROMPTS) {
> +          let subject  = {wrappedJSObject: {target: browser, addon: install.addon}};

nit: remove duplicated space.

Comment 7

[Andrew Swan [:aswan]]

Comment on attachment 8828648 [details]
Bug 1308296 Show post-install notification

https://reviewboard.mozilla.org/r/105992/#review107028

> None of the existing OK button has a '!', so I would suggest removing it here to be consistent.
> 
> http://searchfox.org/mozilla-central/search?q=%3DOK&path=.properties

Hm, I'm not even sure where that came from.  This is a very old patch that I dusted off, it must have been in some earlier mock-up from UX.  But yeah its not even in the current mocks, removing it.

> I think code doing these kind of manipulations on the panel usually do it on the "showing" event rather than on the "shown" one.
> 
> That said, I can't really make a convincing argument for why using 'shown' would cause issues. I suspect it may be more likely to cause flickering if you do the changes once the panel is already visible on screen; but I'm not sure that actually happens.

Ah, that wasn't a deliberate choice, fixing.

Comment 8

[Andrew Swan [:aswan]]

Comment on attachment 8828648 [details]
Bug 1308296 Show post-install notification

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

Comment 9

[Florian Quèze [:florian]]

Comment on attachment 8828648 [details]
Bug 1308296 Show post-install notification

https://reviewboard.mozilla.org/r/105992/#review107122

::: browser/modules/ExtensionsUI.jsm:348
(Diff revision 2)
> +             .innerHTML = msg1;
> +          doc.getElementById("addon-installed-notification-message")
> +             .innerHTML = msg2;
> +        }
> +      }
> +    }

nit: missing ;

::: browser/modules/ExtensionsUI.jsm:352
(Diff revision 2)
> +      }
> +    }
> +
> +    popups.show(target, "addon-installed", "",
> +                "addons-notification-icon",
> +                {

I still think it would be slightly more readable to put this in a separate variable.

Comment 10

[Andrew Swan [:aswan]]

Comment on attachment 8828648 [details]
Bug 1308296 Show post-install notification

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

Comment 11

[Andrew Swan [:aswan]]

Comment on attachment 8828648 [details]
Bug 1308296 Show post-install notification

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

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/84923aad3402
Show post-install notification r=florian

Comment 13

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/84923aad3402

Comment 14

[Vasilica Tamas, QA [:vtamas]]

The webextension name is mentioned twice in the confirmation pop-up and for some of the webextenions the doorhanger seems to be very crowded such in the following case: https://www.screencast.com/t/S8R17ggx2

Checking the mock-ups, the second paragraph should not contain the webextension name and it should be replaced by “your extension” expression: https://www.screencast.com/t/g1ftVkdiofBc

But, I’m not sure which of the following confirmation pop-ups will be finally kept? This one https://bug1308296.bmoattachments.org/attachment.cgi?id=8828649 or this one https://bug1308296.bmoattachments.org/attachment.cgi?id=8798659 ?

Comment 15

[Andrew Swan [:aswan]]

The layout issues are covered in bug 1333168.  The [name of add-on] appearing twice comes from the latest copy doc attached to bug 1316996, redirecting the comment on that to Scott.

Comment 16

[Andrew Swan [:aswan]]

This was still open pending a test getting added, which happened in bug 1308310 so resolving.
If we decide to change the copy in the dialog, that can happen in a follow-up.

Comment 17

[sdevaney]

(In reply to Vasilica Mihasca, QA [:vasilica_mihasca] from comment #14)
> The webextension name is mentioned twice in the confirmation pop-up and for
> some of the webextenions the doorhanger seems to be very crowded such in the
> following case: https://www.screencast.com/t/S8R17ggx2
> 
> Checking the mock-ups, the second paragraph should not contain the
> webextension name and it should be replaced by “your extension” expression:
> https://www.screencast.com/t/g1ftVkdiofBc
> 
> But, I’m not sure which of the following confirmation pop-ups will be
> finally kept? This one
> https://bug1308296.bmoattachments.org/attachment.cgi?id=8828649 or this one
> https://bug1308296.bmoattachments.org/attachment.cgi?id=8798659 ?

I suggest this copy:

[name of add-on] has been added to Nightly. 

Manage your add-ons by clicking [symbol] in the [symbol] menu.

Comment 18

[Vasilica Tamas, QA [:vtamas]]

So,from what I understand the "your add-on is ready" popup changed its design and text along the way from https://bug1308296.bmoattachments.org/attachment.cgi?id=8798659 into https://bug1308296.bmoattachments.org/attachment.cgi?id=8828649 ?

Comment 19

[Andrew Swan [:aswan]]

(In reply to Vasilica Mihasca, QA [:vasilica_mihasca] from comment #18)
> So,from what I understand the "your add-on is ready" popup changed its
> design and text along the way from
> https://bug1308296.bmoattachments.org/attachment.cgi?id=8798659 into
> https://bug1308296.bmoattachments.org/attachment.cgi?id=8828649 ?

The screenshots are of mocks for the visual appearance that were created by the UX team, but the actual text in the dialogs is written by Scott.  Which means we don't have a single source to compare against unfortunately.  But in any case, this is getting picked up in bug 1334404.

Comment 20

[Vasilica Tamas, QA [:vtamas]]

(In reply to Andrew Swan [:aswan] from comment #19)

> The screenshots are of mocks for the visual appearance that were created by
> the UX team, but the actual text in the dialogs is written by Scott.  Which
> means we don't have a single source to compare against unfortunately.  But
> in any case, this is getting picked up in bug 1334404.

Thanks. 

This bug can be marked as verified since the confirmation doorhanger is successfully displayed while installing webextensions on Firefox 54.0a1 (2017-01-31) and Firefox 53.0a1 (2017-01-31) under Windows 10 64-bit, Ubuntu 16.04 32-bit and Mac OS X 10.10.4. The other implementation parts will be tracked in Bug 1334404.