Closed Bug 1308296 Opened 3 years ago Closed 3 years ago

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

Categories

(WebExtensions :: General, defect, P5)

51 Branch
defect

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: aswan, Assigned: aswan)

References

Details

(Whiteboard: triaged[permissions])

Attachments

(2 files, 1 obsolete file)

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
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.
Attached image Screenshot 2016-10-06 10.03.57.png (obsolete) —
Priority: -- → P3
Whiteboard: triaged
Priority: P3 → P5
Whiteboard: triaged → triaged[permissions]
Attached image mock
Attachment #8798659 - Attachment is obsolete: true
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...
Attachment #8828648 - Flags: review?(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.
Attachment #8828648 - Flags: review?(florian) → review-
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 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.
Attachment #8828648 - Flags: review?(florian) → review+
Pushed by aswan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/84923aad3402
Show post-install notification r=florian
Assignee: nobody → aswan
Keywords: leave-open
Depends on: 1333071
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 ?
Flags: needinfo?(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.
Flags: needinfo?(aswan) → needinfo?(sdevaney)
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.
Status: NEW → RESOLVED
Closed: 3 years ago
Keywords: leave-open
Resolution: --- → FIXED
Flags: needinfo?(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.
Depends on: 1334404
Depends on: 1334479
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 ?
Flags: needinfo?(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.
Flags: needinfo?(aswan)
No longer depends on: 1334404
No longer depends on: 1334479
(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.
Status: RESOLVED → VERIFIED
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.