1317363 - Implement the new sideloading flow

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect, P1)

Description

[Andrew Swan [:aswan]]

Attachment: sideload indicator mock

The permissions UX design calls for an overhaul of the process for handling sideloaded extensions.  The newaddon page is replaced by a badge on the hamburger menu and an item in the menu notifying the user about the new extension and asking them to approve any needed permissions.
This bug is to implement that flow.

Comment 1

[Andrew Swan [:aswan]]

Attachment: sideload menu item mock

Comment 2

[Andrew Swan [:aswan]]

Markus, can you provide the badge that we should use as an svg (or redirect to the right person to provide it)?  (existing badges used for app updates for reference are in browser/themes/shared/update-badge.svg and update-badge-failed.svg.
Also, in the event that we have an app update available plus sideloaded extensions (plus extension updates), in what order should those items appear in the menu?

Comment 3

[Andrew Swan [:aswan]]

And one more question Markus, the illustrations at https://www.figma.com/file/HrLiKUwoLQZsIUIVBKM8Wnnu/Install-Flow-showing-Permissions still show a modal dialog box, but I thought this was going to be revised to jump to about:addons and show a doorhanger attached to the puzzle icon in the location bar?

Comment 4

[:shell escalante]

hi markus - do you have details on aswans' questions?

Comment 5

[Markus Jaritz [:designakt] (UX)]

(In reply to Andrew Swan [:aswan] from comment #2)
> Markus, can you provide the badge that we should use as an svg (or redirect
> to the right person to provide it)?  (existing badges used for app updates
> for reference are in browser/themes/shared/update-badge.svg and
> update-badge-failed.svg.

Emanuela & Phil should be able to help with the svg.
We are working on a collection of all those icons but this is not part of it yet. (https://firefoxux.github.io/StyleGuide/#/glyphs)
Emanuala is working on the visual UI detailing. Ping here for any assets you need.

> Also, in the event that we have an app update available plus sideloaded
> extensions (plus extension updates), in what order should those items appear
> in the menu?
Firefox Update on the bottom, then sideloaded and updates on top.
(I hope this is not the case too often.)

(In reply to Andrew Swan [:aswan] from comment #3)
> And one more question Markus, the illustrations at
> https://www.figma.com/file/HrLiKUwoLQZsIUIVBKM8Wnnu/Install-Flow-showing-
> Permissions still show a modal dialog box, but I thought this was going to
> be revised to jump to about:addons and show a doorhanger attached to the
> puzzle icon in the location bar?

I thought so too. (The file contains Permissions v1 - Panel, and also screens for v2 - Modal.)
Use this link to be sure to see only v1:
https://www.figma.com/file/HrLiKUwoLQZsIUIVBKM8Wnnu/Install-Flow-showing-Permissions?node-id=321%3A78

Comment 6

[Andrew Swan [:aswan]]

I talked with Markus about how to handle the case where the name of the extension is too long to fit into the menu.
His request is that in that case we break the menu item into two lines with the name of the extension on the first line and the "was added to Firefox" fixed part on the second line.
I'm not sure how this would work with localization, i.e., can we impose a localization constraint that the add-on name must always come first in the localized string?

Comment 7

[Andy McKay]

Gijs, could you please point us to someone who might know how edge cases in the UI like comment 6 are normally handled?

Comment 8

[:Gijs (out for now; he/him)]

(In reply to Andy McKay [:andym] from comment #7)
> Gijs, could you please point us to someone who might know how edge cases in
> the UI like comment 6 are normally handled?

(In reply to Andrew Swan [:aswan] from comment #6)
> His request is that in that case we break the menu item into two lines with
> the name of the extension on the first line and the "was added to Firefox"
> fixed part on the second line.
> I'm not sure how this would work with localization, i.e., can we impose a
> localization constraint that the add-on name must always come first in the
> localized string?

I don't think this is possible. In some languages it will be literally impossible to have no content before the name of the extension. If you're manually detecting the case where the extension name is too long, I guess there should be a separate string for that case with a hardcoded newline. You can have a localization comment indicating the constraints, to help localizers.

:flod is probably the best person to ask other l10n questions, and :shorlander or :phlsa might have other ideas if they've faced this issue before.

Comment 9

[emanuela [ux]]

My suggestion here is to simplify. 

We can identify the name of the web extension. If the name is longer than 26 character, truck it and add an ellipsis "…"
I don't see any advantages to split the message and the name of the add-on in different lines by default.

Comment 10

[emanuela [ux]]

Attachment: addon-badge.svg

(In reply to Andrew Swan [:aswan] from comment #2)
> Markus, can you provide the badge that we should use as an svg (or redirect
> to the right person to provide it)?  (existing badges used for app updates
> for reference are in browser/themes/shared/update-badge.svg and
> update-badge-failed.svg.


Let's use the add-on icon. I optimised the icon in the version attached in this comment. 

Badge color: #bb3817
Svg fill: white.

Comment 11

[Andrew Swan [:aswan]]

Attachment: Screen Shot 2017-01-01 at 6.20.18 PM.png

Emanuela, I attached a screenshot of what the toolbar menu button looks like with the badge as you specified it, is this what you were expecting?

Comment 12

[emanuela [ux]]

Attachment: Screen-Shot-2017-01-03-at-09.29.43.png

(In reply to Andrew Swan [:aswan] from comment #11)
> Created attachment 8823063 [details]
> Screen Shot 2017-01-01 at 6.20.18 PM.png
> 
> Emanuela, I attached a screenshot of what the toolbar menu button looks like
> with the badge as you specified it, is this what you were expecting?


Close! I attached the right version

Comment 13

[Andrew Swan [:aswan]]

Attachment: Bug 1317363 Implement the new sideloading flow

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

Comment 14

[Andrew Swan [:aswan]]

Comment on attachment 8824588 [details]
Bug 1317363 Implement the new sideloading flow

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

Comment 15

[Andrew Swan [:aswan]]

Comment on attachment 8824588 [details]
Bug 1317363 Implement the new sideloading flow

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

Comment 16

[Florian Quèze [:florian]]

Comment on attachment 8824588 [details]
Bug 1317363 Implement the new sideloading flow

https://reviewboard.mozilla.org/r/103020/#review105624

If I understood the test correctly, if a user clicks the badge (which opens the panel), then clicks the notification at the bottom of the panel, then cancels the doorhanger that opens once the add-on manager is loaded, the notification in the panel is still there. Is this intended? Isn't this pushing users to accept add-ons whatever they are just to get rid of the notification?

::: browser/base/content/browser-addons.js:495
(Diff revision 3)
> +      container.firstChild.remove();
> +    }
> +
> +    // Strings below to be properly localized in bug 1316996
> +    const DEFAULT_EXTENSION_ICON
> +      = "chrome://mozapps/skin/extensions/extensionGeneric.svg";

nit: '=' on the previous line, right before the line break.

::: browser/base/content/browser-addons.js:507
(Diff revision 3)
> +
> +      button.addEventListener("click", evt => {
> +        ExtensionsUI.showSideloaded(gBrowser, addon);
> +      });
> +
> +      container.appendChild(button);

If sideloaded.size is large, does the UI break, or is that part of the UI already designed to handle large numbers of alerts gracefully?

::: browser/base/content/test/general/browser_extension_permissions.js:47
(Diff revision 3)
>    let uls = panel.firstChild.getElementsByTagNameNS("http://www.w3.org/1999/xhtml", "ul");
>    is(uls.length, 1, "Found the permissions list");
>    let ul = uls[0];
>  
> -  let headers = panel.firstChild.getElementsByClassName("addon-webext-perm-text");
> -  is(headers.length, 1, "Found the header");
> +  let elements = panel.firstChild.getElementsByClassName("addon-webext-perm-text");
> +  is (elements.length, 2, "Found two text elements in notification");

Is this test relevant? Should we instead filter out all the hidden elements, and check we still have only one header?

::: browser/base/content/test/general/browser_extension_sideloading.js:1
(Diff revision 3)
> +

nit: remove empty line at start of file

::: browser/base/content/test/general/browser_extension_sideloading.js:75
(Diff revision 3)
> +
> +function promiseViewLoaded(tab, viewid) {
> +  let win = tab.linkedBrowser.contentWindow;
> +  if (win.gViewController && !win.gViewController.isLoading &&
> +      win.gViewController.currentViewId == viewid) {
> +        return Promise.resolve();

nit: indent

::: browser/base/content/test/general/browser_extension_sideloading.js:94
(Diff revision 3)
> +
> +function promisePopupNotificationShown(name) {
> +  return new Promise(resolve => {
> +    function popupshown() {
> +      let notification = PopupNotifications.getNotification(name);
> +      if (!notification) { return; }

nit: line breaks after { and before }

::: browser/base/content/test/general/browser_extension_sideloading.js:161
(Diff revision 3)
> +
> +  // Ugh, _checkForSideloaded emits an event and the front-end code needs
> +  // to respond to that event before the UI will be updated.  Take a spin
> +  // through the event loop to make that happens before we start verifying
> +  // the front-end UI bits.
> +  yield new Promise(resolve => setTimeout(resolve, 0));

Can we instead listen for the specific event we care about?

If not, is executeSoon() available here? (If so, I would prefer yield new Promise(resolve => executeSoon(resolve)); to something using a timer)

::: browser/components/nsBrowserGlue.js:1053
(Diff revision 3)
>      }
>  
> -    // For any add-ons that were installed disabled and can be enabled offer
> -    // them to the user.
>      let win = RecentWindow.getMostRecentBrowserWindow();
> -    AddonManager.getAllAddons(addons => {
> +    ExtensionsUI.init(win);

The 'win' parameter doesn't seem a good idea here, as the ExtensionsUI.init call applies to one window specifically.

I think that getMostRecentBrowserWindow call can be moved inside the _checkForSideloaded method, right before the for loop where it'll be used.

::: browser/modules/ExtensionsUI.jsm:41
(Diff revision 3)
> +      // Check for any side-loaded addons that the user is allowed
> +      // to enable.
> +      let sideloaded = addons.filter(
> +        addon => addon.seen === false && (addon.permissions & AddonManager.PERM_CAN_ENABLE));
> +
> +      if (sideloaded.length > 0) {

if (!sideloaded.length) {
  return;
}

::: browser/modules/ExtensionsUI.jsm:49
(Diff revision 3)
> +            this.sideloaded.add(addon);
> +          }
> +          this.emit("change");
> +        } else {
> +          for (let addon of sideloaded) {
> +            win.openUILinkIn(`about:newaddon?id=${addon.id}`, "tab");

Is there a bug on file to cleanup this old code path and remove the newaddon about page?

::: browser/themes/shared/addons/addon-badge.svg:3
(Diff revision 3)
> +<?xml version="1.0" encoding="utf-8"?>
> +<svg version="1.1" id="Layer_1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" x="0px" y="0px"
> +     viewBox="0 0 10 10" style="enable-background:new 0 0 10 10;" xml:space="preserve">

Some of the attributes can be removed here.

::: browser/themes/shared/addons/addon-badge.svg:11
(Diff revision 3)
> +  stroke: #bb3817;
> +  fill: #FFFFFF;
> +}
> +</style>
> +<path d="M6.6,9c0.3,0,0.5-0.3,0.6-0.6C7,7.7,7,6.9,7.1,6.2c0.1-0.3,0.3-0.4,0.6-0.4c0.3,0,0.3,0.4,1,0.4
> +	c0.3,0,0.8-0.1,0.8-1.1S9,3.9,8.7,3.9c-0.6,0-0.7,0.5-1,0.5c-0.3,0-0.5-0.2-0.6-0.5c0-0.3,0-0.7,0-1c0-0.3-0.2-0.5-0.5-0.6

avoid tabs for the indent

::: browser/themes/shared/customizableui/panelUI.inc.css:609
(Diff revision 3)
> +#PanelUI-footer-addons > toolbarbutton::after {
> +  content: "";
> +  width: 14px;
> +  height: 14px;
> +  margin-inline-end: 16.5px;
> +  box-shadow: 0px 1px 0px rgba(255,255,255,.2) inset, 0px -1px 0px rgba(0,0,0,.1) inset, 0px 1px 0px rgba(12,27,38,.2);

Could we somehow use a class to avoid duplicating all these rules with #PanelUI-update-status[update-status]::after ?

::: toolkit/mozapps/extensions/internal/XPIProviderUtils.js:93
(Diff revision 3)
>                            "skinnable", "size", "sourceURI", "releaseNotesURI",
>                            "softDisabled", "foreignInstall", "hasBinaryComponents",
>                            "strictCompatibility", "locales", "targetApplications",
>                            "targetPlatforms", "multiprocessCompatible", "signedState",
> -                          "seen", "dependencies", "hasEmbeddedWebExtension", "mpcOptedOut"];
> +                          "seen", "dependencies", "hasEmbeddedWebExtension", "mpcOptedOut",
> +                          "userPermissions"];

Is this change related to the rest of the patch?

Comment 17

[Andrew Swan [:aswan]]

Comment on attachment 8824588 [details]
Bug 1317363 Implement the new sideloading flow

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

Comment 18

[Andrew Swan [:aswan]]

Comment on attachment 8824588 [details]
Bug 1317363 Implement the new sideloading flow

https://reviewboard.mozilla.org/r/103018/#review105834

Comment 19

[Andrew Swan [:aswan]]

Comment on attachment 8824588 [details]
Bug 1317363 Implement the new sideloading flow

https://reviewboard.mozilla.org/r/103020/#review105624

> If sideloaded.size is large, does the UI break, or is that part of the UI already designed to handle large numbers of alerts gracefully?

I guess it depends what you mean by "break".  The menu would get a lot of entries, but that's such an unlikely scenario that I'm not inclined to add more code to handle it.

> Is this test relevant? Should we instead filter out all the hidden elements, and check we still have only one header?

We actually do have two elements with the addon-perm-webext-text class in some of the dialogs, there is the text element ("Another program on your computer...") and the intro to the list ("It can...")
The test is mostly a check to ensure that the next line (grabbing the list intro item) is sane.

> if (!sideloaded.length) {
>   return;
> }

Well that's not right and this is exactly why I don't like using the boolean shorthand for integer comparisons but nevertheless I've changed it to `if (sideloaded.length) {...`

> Is there a bug on file to cleanup this old code path and remove the newaddon about page?

Filed bug 1331521

> Could we somehow use a class to avoid duplicating all these rules with #PanelUI-update-status[update-status]::after ?

I'm not sure how to do that on the ::after pseudo element?

> Is this change related to the rest of the patch?

Yes, we only read the extension contents once and then store the information that the add-ons manager cares about in extensions.json.  This change probably should have been part of an earlier patch but it didn't actually break anything until now.
I'll add rhelmer to the review though, Rob can you sign off on this change?

Comment 20

[Andrew Swan [:aswan]]

Comment on attachment 8824588 [details]
Bug 1317363 Implement the new sideloading flow

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

Comment 21

[Andrew Swan [:aswan]]

Comment on attachment 8824588 [details]
Bug 1317363 Implement the new sideloading flow

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/103020/diff/5-6/

Comment 22

[Andrew Swan [:aswan]]

Comment on attachment 8824588 [details]
Bug 1317363 Implement the new sideloading flow

https://reviewboard.mozilla.org/r/103020/#review105624

Ah good catch, this wasn't explicitly covered in the UX specs but the old about:newaddon based behavior was that you only got notified once.  I've changed it to match this behavior and updated the test accordingly.

Comment 23

[Florian Quèze [:florian]]

Comment on attachment 8824588 [details]
Bug 1317363 Implement the new sideloading flow

https://reviewboard.mozilla.org/r/103020/#review106100

::: browser/modules/ExtensionsUI.jsm:41
(Diff revisions 3 - 6)
>        // Check for any side-loaded addons that the user is allowed
>        // to enable.
>        let sideloaded = addons.filter(
>          addon => addon.seen === false && (addon.permissions & AddonManager.PERM_CAN_ENABLE));
>  
> -      if (sideloaded.length > 0) {
> +      if (sideloaded.length) {

In my previous review I meant this should become an early return. Sorry if that wasn't explicit.

::: browser/modules/ExtensionsUI.jsm:48
(Diff revisions 3 - 6)
>            for (let addon of sideloaded) {
>              this.sideloaded.add(addon);
>            }
>            this.emit("change");
>          } else {
> +          let win = RecentWindow.getMostRecentBrowserWindow();

Is RecentWindow already imported in this scope?

::: browser/themes/shared/addons/addon-badge.svg:3
(Diff revisions 3 - 6)
>  <?xml version="1.0" encoding="utf-8"?>
> -<svg version="1.1" id="Layer_1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" x="0px" y="0px"
> -     viewBox="0 0 10 10" style="enable-background:new 0 0 10 10;" xml:space="preserve">
> +<svg version="1.1" xmlns="http://www.w3.org/2000/svg"
> +     x="0px" y="0px" viewBox="0 0 10 10">

I'm not sure what x="0px" y="0px" means here, but other svg files in the same folder would have something like width="10" height="10" instead. Not sure if that makes a difference, but if not then let's be consistent.

Comment 24

[Florian Quèze [:florian]]

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

> > If sideloaded.size is large, does the UI break, or is that part of the UI already designed to handle large numbers of alerts gracefully?
> 
> I guess it depends what you mean by "break".

I mean something unusable where scrolling wouldn't be possible and that would prevent accessing the bottom of the panel. What I had in mind was the brokenness we had in bug 1113747 (see attachment 8539412 [details]).

> > Is there a bug on file to cleanup this old code path and remove the newaddon about page?
> 
> Filed bug 1331521

Include this in a comment please?

> > Could we somehow use a class to avoid duplicating all these rules with #PanelUI-update-status[update-status]::after ?
> 
> I'm not sure how to do that on the ::after pseudo element?

Actually, we don't need a class here, even simpler: at line 569, just do:

#PanelUI-update-status[update-status]::after,
#PanelUI-footer-addons > toolbarbutton::after {
  keep all the shared rules here instead of duplicating

And then, just:
#PanelUI-footer-addons > toolbarbutton::after {
  background-image: url(chrome://browser/skin/addons/addon-badge.svg);
}

Comment 25

[Florian Quèze [:florian]]

Comment on attachment 8824588 [details]
Bug 1317363 Implement the new sideloading flow

https://reviewboard.mozilla.org/r/103020/#review106106

::: browser/base/content/test/general/browser_extension_permissions.js:48
(Diff revision 6)
>    is(uls.length, 1, "Found the permissions list");
>    let ul = uls[0];
>  
> -  let headers = panel.firstChild.getElementsByClassName("addon-webext-perm-text");
> -  is(headers.length, 1, "Found the header");
> -  let header = headers[0];
> +  let elements = panel.firstChild.getElementsByClassName("addon-webext-perm-text");
> +  is (elements.length, 2, "Found two text elements in notification");
> +  let header = elements[1];

From our irc discussion, it seems you intended to use getElementById here.

Comment 26

[Andrew Swan [:aswan]]

(In reply to Florian Quèze [:florian] [:flo] from comment #24)
> (In reply to Andrew Swan [:aswan] from comment #19)
> > > If sideloaded.size is large, does the UI break, or is that part of the UI already designed to handle large numbers of alerts gracefully?
> > 
> > I guess it depends what you mean by "break".
> 
> I mean something unusable where scrolling wouldn't be possible and that
> would prevent accessing the bottom of the panel. What I had in mind was the
> brokenness we had in bug 1113747 (see attachment 8539412 [details]).

Markus, what do you think about this?  Given how uncommon sideloading is (or how uncommon we expect it is), it shouldn't be a problem to only display the first few.

Comment 27

[Andrew Swan [:aswan]]

Comment on attachment 8824588 [details]
Bug 1317363 Implement the new sideloading flow

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/103020/diff/6-7/

Comment 28

[Robert Helmer [:rhelmer]]

Comment on attachment 8824588 [details]
Bug 1317363 Implement the new sideloading flow

https://reviewboard.mozilla.org/r/103020/#review106174

Comment 29

[Andrew Swan [:aswan]]

Comment on attachment 8824588 [details]
Bug 1317363 Implement the new sideloading flow

https://reviewboard.mozilla.org/r/103020/#review105624

> I guess it depends what you mean by "break".  The menu would get a lot of entries, but that's such an unlikely scenario that I'm not inclined to add more code to handle it.

Okay, limited the number of menu items to 4 (which was chosen arbitrarily).  If UX wants something different, I propose we do it in a follow-up.

Comment 30

[Andrew Swan [:aswan]]

Comment on attachment 8824588 [details]
Bug 1317363 Implement the new sideloading flow

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/103020/diff/7-8/

Comment 31

[Andrew Swan [:aswan]]

Comment on attachment 8824588 [details]
Bug 1317363 Implement the new sideloading flow

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/103020/diff/8-9/

Comment 32

[Florian Quèze [:florian]]

Comment on attachment 8824588 [details]
Bug 1317363 Implement the new sideloading flow

https://reviewboard.mozilla.org/r/103020/#review106260

Looks good to me now, thanks!

Comment 33

[emanuela [ux]]

(In reply to Andrew Swan [:aswan] from comment #29)
> Comment on attachment 8824588 [details]
> Bug 1317363 Implement the new sideloading flow
> 
> https://reviewboard.mozilla.org/r/103020/#review105624
> 
> > I guess it depends what you mean by "break". The menu would get a lot of entries, but that's such an unlikely scenario that I'm not inclined to add more code to handle it.
> 
> Okay, limited the number of menu items to 4 (which was chosen arbitrarily). 
> If UX wants something different, I propose we do it in a follow-up.


I think is more a general question here. For which API we want to display permissions and how. Currently in Chrome is remarkably hard to find an extension with more than three permissions listed. I agree, let's currently set the number to five or four and let's see how it works in action.

Comment 34

[emanuela [ux]]

About the number of simultaneously shown sideload notifications shown in the firefox menu, I think the scenario is quite rare. We can display 4 menu items per time. We can track scenarios with more than 4 sideloading extensions and/or update in another bug.

Comment 35

[Andrew Swan [:aswan]]

(In reply to emanuela [ux team] [PTO until Jan 3rd] from comment #33)
> (In reply to Andrew Swan [:aswan] from comment #29)
> > Comment on attachment 8824588 [details]
> > Bug 1317363 Implement the new sideloading flow
> > 
> > https://reviewboard.mozilla.org/r/103020/#review105624
> > 
> > > I guess it depends what you mean by "break". The menu would get a lot of entries, but that's such an unlikely scenario that I'm not inclined to add more code to handle it.
> > 
> > Okay, limited the number of menu items to 4 (which was chosen arbitrarily). 
> > If UX wants something different, I propose we do it in a follow-up.
> 
> I think is more a general question here. For which API we want to display
> permissions and how. Currently in Chrome is remarkably hard to find an
> extension with more than three permissions listed. I agree, let's currently
> set the number to five or four and let's see how it works in action.

Wait we're talking about two completely different things here, though it looks like comment 34 addresses the question I was trying to ask about a large number of sideloaded (and/or updated) extensions.
For actual permissions, no truncation takes place, though the permissions copy proposal specifies that some permissions don't trigger an entry in the permissions dialog.  That's probably better discussed over at bug 1316996.

Comment 36

[Andrew Swan [:aswan]]

Comment on attachment 8824588 [details]
Bug 1317363 Implement the new sideloading flow

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/103020/diff/9-10/

Comment 37

[Andrew Swan [:aswan]]

Comment on attachment 8824588 [details]
Bug 1317363 Implement the new sideloading flow

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/103020/diff/10-11/

Comment 38

[Andrew Swan [:aswan]]

Comment on attachment 8824588 [details]
Bug 1317363 Implement the new sideloading flow

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/103020/diff/11-12/

Comment 39

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7aa76c31395d

Comment 40

[Vasilica Tamas, QA [:vtamas]]

Tested a bit around this issue on Firefox 54.0a1 (2017-01-25/26) and Firefox 53.0a2 (2017-01-25-/26) under Windows 10 64-bit, Ubuntu 16.04 32-bit and Mac OS X 10.11 and noticed the following issues:

   - There is not the correct add-on icon displayed in top right corner of the humberger menu: https://www.screencast.com/t/arXucNlO4CwM - Andrew already covered this issue in Bug 1334010

   - There is no hover effect while hovering over the Sideloaded button from Panel Menu - this is also tracked in Bug 1334010

   - The sideloading button text is centered and not left alligned: https://www.screencast.com/t/tsy8GV2jDRyr - tracked by Bug 1334085

   - No puzzle icon displayed in installation pop-up if the webextension has no icon specified in manifest https://www.screencast.com/t/NCDRThBT - tracked by Bug 1334076

   - The sideloading button does not disappear from Panel Menu if the webextension was enabled from Add-ons Manager by skipping the sideloading flow - tracked by Bug 1334096 


Questions: 
   - Which colour should have the sideloading installation button? Blue as in https://www.figma.com/file/HrLiKUwoLQZsIUIVBKM8Wnnu/Install-Flow-showing-Permissions or red as in https://bug1317363.bmoattachments.org/attachment.cgi?id=8810447 ?

   - The add-on icon is displayed on the sideloading installation button (  https://www.screencast.com/t/jeYNu82H ) and not the exclamation mark as in the mock-up:  https://bug1317363.bmoattachments.org/attachment.cgi?id=8810447 . Which is the expected icon?

Please let me know which are the correct answers.

Comment 41

[Merike Sell (:merike)]

What info will the user be able to see before deciding to allow an extension? Anything in addition to the icon and extension name? Are author and description visible? It can be quite confusing to only see extension name and icon in some cases.

Comment 42

[Andy McKay]

Author and description are not included. If you feel they should, please file a seperate bug for that.

Please see the example shown in the blog post here: https://blog.mozilla.org/addons/2017/01/25/webextensions-in-firefox-53/, you can try it in nightly following these instructions: https://wiki.mozilla.org/WebExtensions/Permissions#Status.

Comment 43

[Vasilica Tamas, QA [:vtamas]]

Confirm that sideloading installation is successfully implemented in Firefox 54.0a1 (2017-02-19) and Firefox 53.0a2 (2017-02-20) under Windows 10 64-bit and Ubuntu 16.04 32-bit. During this testing I also encountered a few sideloading issues but they are already tracked in separate bugs.