Closed Bug 1360777 Opened 6 years ago Closed 6 years ago

about:addons indications for disabled legacy extensions

Categories

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

Product:
Toolkit
Component:
Add-ons Manager
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: aswan, Assigned: aswan)

References

Details

(Whiteboard: triaged)

Attachments

(4 files)

What is new tab for me
138.15 KB, image/png
Details
legacy.svg
927 bytes, image/svg+xml
Details
Bug 1360777 Move legacy extension to a separate pane in about:addons
59 bytes, text/x-review-board-request
Details
Bug 1360777 - remove double spaces and remove hardcoded Firefox for legacy extensions in about:addons
59 bytes, text/x-review-board-request
aswan
: review+
Details 
This bug is to cover the UX in about:addons for disabled legacy extensions.
Markus is this the latest set of mock-ups?
https://mozilla.invisionapp.com/share/HUAUGBGWZ

Assuming it is, can you confirm that:
- disabled legacy extension should not appear at all in the default list of extensions
- the "missing something?" banner should only appear if there are legacy extensions in the profile
- if there are both unsigned and legacy extensions in the profile, we should have both the "one more more installed add-ons cannot be verified..." message at the top of the page and the "missing something?" message right below that.  And unsigned extensions should be visible with the error icon but legacy extensions should not be visible (until clicking through on the "missing something?" banner)
Flags: needinfo?(mjaritz)
Forwarding to Emanuela as she worked on those mocks.
Flags: needinfo?(mjaritz) → needinfo?(emanuela)
(In reply to Andrew Swan [:aswan] from comment #1)

> Assuming it is, can you confirm that:
> - disabled legacy extension should not appear at all in the default list of
> extensions
True.
All disabled legacy extensions are in a separated tab.

> - the "missing something?" banner should only appear if there are legacy
> extensions in the profile

True.

> - if there are both unsigned and legacy extensions in the profile, we should
> have both the "one more more installed add-ons cannot be verified..."
> message at the top of the page and the "missing something?" message right
> below that. And unsigned extensions should be visible with the error icon
> but legacy extensions should not be visible (until clicking through on the
> "missing something?" banner)

Almost true :) Yes, we should display both banners. Ideally, those banners are display one next to another with some space between them. Cliking on the "Show me the extensions" brings the user in a separated tab where all the disabled legacy extensions are listed. 

Do we have an estimate of how many people are going to see both message? If not, can we have it?
Flags: needinfo?(emanuela)
(In reply to emanuela [ux team] from comment #3)
> > - if there are both unsigned and legacy extensions in the profile, we should
> > have both the "one more more installed add-ons cannot be verified..."
> > message at the top of the page and the "missing something?" message right
> > below that. And unsigned extensions should be visible with the error icon
> > but legacy extensions should not be visible (until clicking through on the
> > "missing something?" banner)
> 
> Almost true :) Yes, we should display both banners. Ideally, those banners
> are display one next to another with some space between them. Cliking on the
> "Show me the extensions" brings the user in a separated tab where all the
> disabled legacy extensions are listed. 

I'm not sure we have enough space to put them next to each other.  Can you create a mock to show how you expect it to look?

> Do we have an estimate of how many people are going to see both message? If
> not, can we have it?

I don't think we have the data to get an accurate number but maybe we could make a guess.  Also this number is a moving target as extension authors convert to webextensions.
Flags: needinfo?(emanuela)
Thinking about this more, this would be less engineering/maintenance work in the long run if we treat unsigned and legacy extensions the same way -- is it intentional that they are handled differently (ie that unsigned extensions show up in the default extensions list and legacy extensions do not) and if so why?

Also, this may become moot based on the previous issue but I still can't reconcile the part of comment 3 that says the two banners should be next to each other with the mocks that show the banner for legacy extensions below the row that has the gear icon and search box.

Finally, comment 3 also says that "Show me the extensions" should open a new tab but in fact we go to some effort to try to push users toward only having one copy of about:addons open at a time.  Opening a new tab would be a big reversal, are we sure we want to do that?

I'd like to make progress on this bug this week so getting these questions resolved quickly would be very helpful.
Adding Markus as well in case he wants to weigh in.
Flags: needinfo?(mjaritz)
Attached image What is new tab for me — 
Hey Andrew, as you may remember one of the first solutions we explored was to treat Legacy extensions as unsigned extensions. What you’re proposing, unfortunately, does not work for this case.
With Quantum we’re giving to our users a better, new and faster Firefox. However, shipping the code, it’s not enough, and besides code, we must ship a better experience.
For this reason, we’re applying a progressive disclosure pattern here. We reveal only essential information (here: the banner) and then help to manage complexity by disclosing information and options progressively (the new tab and the Learn more within it). 
We’re not showing the Legacy extensions in the same tab to avoid an overwhelmed and annoying experience to not-power-users. 
Advanced users will benefit too. We save them time because they don’t have to scan a large list of extensions to check which ones are off.

Plus, how likely is that unsigned extensions are going to be Web Extensions as well? If they aren't Web Extensions, we should treat them as Legacy extensions. Doing this should leave us with very few unsigned extensions.

Extra: I attached a screenshot because maybe we're having some terminology issue here. With the 'new tab' a mean a new entry in the sidebar. I thought the prototype was giving you the right context, but I just want to be sure.
Flags: needinfo?(mjaritz)
Flags: needinfo?(emanuela)
Thank you for clarifying what was meant by "new tab".  Can/should we also move unsigned extensions into the "Legacy Extensions" section?  And where can I find the icon that should be next to "Legacy Extensions"?
Finally, as unlikely as it is to happen in reality, we still need to deal with the case that users have both unsigned and legacy extensions in their profile.  If we can lump them together then perhaps we can get away with a single banner.  If we're going to have two banners, please clarify where they should be displayed.
Flags: needinfo?(emanuela)
Priority: -- → P1
Whiteboard: triaged
Talked to Markus and Emanuela, the invision mocks have now been updated.
A few more questions:
1. Can you provide the icon that should appear next to "Legacy Extensions" on the left side of about:addons
2. Where should Legacy Extensions appear in the list on the left side?  From the mocks it looks like it should go below all the individual types of add-ons (Extensions, Plugins, etc) but should it be above or below available and recent updates, which can appear at the bottom of that list?
3. On the last page of the mocks, all the legacy extensions have the same icon, but I assume that is meant to be each extension's individual icon (or the default), just shown in grey?
(In reply to Andrew Swan [:aswan] from comment #8)
> Talked to Markus and Emanuela, the invision mocks have now been updated.
Yes all updated, now both legacy and unsigned extensions both go into the newly added category. This category is labeled "Legacy Extnsions" if it only contains legacy extensions. And "Unsupported" if it contains unsigned extensions.
Unsigned extensions are only labeled as such if they are not legacy, or legacy is prefed on.

> A few more questions:
> 1. Can you provide the icon that should appear next to "Legacy Extensions"
> on the left side of about:addons
Emanuela will get back to you with an icon next week. If you need one now, please use a grey-scale version of the extensions puzzle piece. (Might need to be updated to Photon icons soon. Emanuela is working on those.)

> 2. Where should Legacy Extensions appear in the list on the left side?  From
> the mocks it looks like it should go below all the individual types of
> add-ons (Extensions, Plugins, etc) but should it be above or below available
> and recent updates, which can appear at the bottom of that list?
Please place the category above updates, and below everything else.

> 3. On the last page of the mocks, all the legacy extensions have the same
> icon, but I assume that is meant to be each extension's individual icon (or
> the default), just shown in grey?
Yes, please treat them as regular disabled extensions with their individual icon in grey.
The mocks have a "Find a Replacement" button next to legacy extensions.  I could swear we talked before about where that should I link but I can't seem to find it now.  Andy, where shoudl it go?
(see previous comment)
Flags: needinfo?(amckay)
Jorge should be able to provide a URL-pattern for those as we agreed to resolve what we show for each extension on AMO.
Flags: needinfo?(jorge)
In the mocks, the "Legacy Extensions" panel has a link at the top with the text "Learn about the changes to Firefox add-ons".  Should that go to the same page the "legacy" badges currently point at?  That page is reachable from various urls but among others it is:
https://support.mozilla.org/en-US/kb/firefox-add-technology-modernizing
Flags: needinfo?(mjaritz)
(In reply to Andrew Swan [:aswan] from comment #14)
> In the mocks, the "Legacy Extensions" panel has a link at the top with the
> text "Learn about the changes to Firefox add-ons".  Should that go to the
> same page the "legacy" badges currently point at?  That page is reachable
> from various urls but among others it is:
> https://support.mozilla.org/en-US/kb/firefox-add-technology-modernizing

It should use the same links we're using for the legacy badges.
Flags: needinfo?(mjaritz)
Flags: needinfo?(emanuela)
Reminder, I am still waiting on the icon for the Legacy Extensions pane
Flags: needinfo?(emanuela)
Attached image legacy.svg — 
Here the icon for legacy extensions.
Flags: needinfo?(emanuela)
Here are all the strings I have in my in-progress patches.  Scott, can you either rewrite or approve these?
Taking a look at pages 10-15 of the mocks linked in comment 1 will probably be helpful too.

Title for the new pane in about:addons when there are only legacy extensions: "Legacy Extensions"
Title for the new pane when there are also unsigned extensions: "Unsupported"
(the two above also have tooltips with the exact same text)
Text for the new button next to legacy addons: "Find a Replacement"
Text in the banner at the top of the extensions list if there are legacy or unsigned extensions: "Missing something?  Some extensions are no longer supported by Firefox."
Text for the link in the above banner: "Show unsupported extensions"
Text at the top of the pane for unsupported extensions: "These extensions do not meet current Firefox standards so we deactivated them."
Text for the link immediately following the above text: "Learn about the changes to Firefox add-ons"

Finally, many occurrences of "Firefox" are replaced with eg "Nightly" if the application has a different name, but I think in at least the last two strings we should just use "Firefox" everywhere?
Flags: needinfo?(sdevaney)
Hey aswan, the copy was already checked by mheubusch!

I'm putting her in cc but the copy was ok last time :)
Flags: needinfo?(sdevaney) → needinfo?(mheubusch)
Do legacy extensions that get shipped in C:\Program Files\Nightly\browser\features don't get deactivated?
I thought they must be webextensions to don't be legacy so I filled bug 1366488. But bug 1366486 maybe also fits.
(In reply to emanuela [ux team] [off until 6/2] from comment #19)
> Hey aswan, the copy was already checked by mheubusch!
> 
> I'm putting her in cc but the copy was ok last time :)

Yep - these look good.
Copy looks good!
Flags: needinfo?(mheubusch)
Depends on: 1359203
No longer depends on: 1359023
Comment on attachment 8874640 [details]
Bug 1360777 Move legacy extension to a separate pane in about:addons

https://reviewboard.mozilla.org/r/145984/#review149928

lgtm overall - a few comments which you can take or leave. Should probably remove that `XXX` comment at least (or make it work)

::: toolkit/mozapps/extensions/content/extensions.js:75
(Diff revision 1)
>  const UPDATES_RECENT_TIMESPAN = 2 * 24 * 3600000; // 2 days (in milliseconds)
>  const UPDATES_RELEASENOTES_TRANSFORMFILE = "chrome://mozapps/content/extensions/updateinfo.xsl";
>  
>  const XMLURI_PARSE_ERROR = "http://www.mozilla.org/newlayout/xml/parsererror.xml"
>  
> +const KEY_TEMPORARY = "app-temporary";

We should consider sharing things like this so they don't need to be copy/pasted... not sure if we have any good options outside of importing an `AppConstants`-like class.

::: toolkit/mozapps/extensions/content/extensions.js:2847
(Diff revision 1)
> +  initialize() {
> +    this.node = document.getElementById("legacy-view");
> +    this._listBox = document.getElementById("legacy-list");
> +    this._categoryItem = gCategories.get("addons://legacy/");
> +
> +    document.getElementById("legacy-learnmore").href = SUPPORT_URL + "webextensions";

I know it's done like this elsewhere but is there any reason not to construct URLs using https://developer.mozilla.org/en-US/docs/Web/API/URL instead of as strings?

It'd at least throw on an invalid URL and less clunky to make sure it's constructed properly.

::: toolkit/mozapps/extensions/content/extensions.xml:1650
(Diff revision 1)
>          ]]></body>
>        </method>
>  
> +      <method name="findReplacement">
> +        <body><![CDATA[
> +          openURL(`https://addons.mozilla.org/find-replacement/?guid=${this.mAddon.id}`);

Should this be a variable like the way `SUPPORT_URL` and others work? I see we have a raw wiki.m.o link but that seems to be the exception...

How long will this feature need to live? If we're going to pull it out in the next few releases it doesn't seem like a big deal, but if so please file a bug and put it in a comment (or something)

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:908
(Diff revision 1)
>      if (aAddon.dependencies.some(id => !isActive(id)))
>        return false;
>    }
>  
>    if (!AddonSettings.ALLOW_LEGACY_EXTENSIONS &&
> -      aAddon.type == "extension" && !aAddon.isSystem &&
> +      aAddon.type == "extension" && !aAddon._installLocation.isSystem &&

Why is this necessary? I can't think offhand why `isSystem` on the Addon object wouldn't work - all the `isSystem` getter does is this.

::: toolkit/mozapps/extensions/test/browser/browser_legacy.js:40
(Diff revision 1)
>      },
>      {
> -      id: "new-theme@tests.mozilla.org",
> -      name: NAMES.newTheme,
> -      type: "theme",
> -      isWebExtension: true,
> +      id: "mozilla@tests.mozilla.org",
> +      name: "Mozilla signed extension",
> +      type: "extension",
> +      isWebExtension: false,

Is it worth testing too that Mozilla-signed web extensions aren't marked as legacy?

::: toolkit/mozapps/extensions/test/browser/browser_legacy.js:116
(Diff revision 2)
> +  // The legacy category does not watch for new installs since new
> +  // legacy extensions cannot be installed while legacy extensions
> +  // are disabled, so manually refresh it here.
> +  await mgrWin.gLegacyView.refresh();
> +
> +  /* XXX why doesn't this work

I don't think this listener survives document reload.
Attachment #8874640 - Flags: review+
Comment on attachment 8874640 [details]
Bug 1360777 Move legacy extension to a separate pane in about:addons

https://reviewboard.mozilla.org/r/145984/#review149928

> We should consider sharing things like this so they don't need to be copy/pasted... not sure if we have any good options outside of importing an `AppConstants`-like class.

Yeah, though this was actually unused and is now gone :)

> Why is this necessary? I can't think offhand why `isSystem` on the Addon object wouldn't work - all the `isSystem` getter does is this.

because the isSystem getter is on AddonWrapper and `aAddon` here is an actual AddonInternal

> I don't think this listener survives document reload.

Indeed, will update...
Comment on attachment 8874640 [details]
Bug 1360777 Move legacy extension to a separate pane in about:addons

https://reviewboard.mozilla.org/r/145984/#review149928

> I know it's done like this elsewhere but is there any reason not to construct URLs using https://developer.mozilla.org/en-US/docs/Web/API/URL instead of as strings?
> 
> It'd at least throw on an invalid URL and less clunky to make sure it's constructed properly.

You mean run it through `new URL()` to catch invalid urls at runtime?  I'm not sure what we would do if we caught an exception here...

> Should this be a variable like the way `SUPPORT_URL` and others work? I see we have a raw wiki.m.o link but that seems to be the exception...
> 
> How long will this feature need to live? If we're going to pull it out in the next few releases it doesn't seem like a big deal, but if so please file a bug and put it in a comment (or something)

I'm not sure how long we're expecting to keep this around.  I thought that using prefs was generally for toolkit code that is used in different applications, I don't see an immediate need for that level of generality here...

> Is it worth testing too that Mozilla-signed web extensions aren't marked as legacy?

That doesn't strike me as a particularly likely regression (at least not without also breaking either Mozilla-signed bootstrapped or webextensions at the same time...)
Comment on attachment 8874640 [details]
Bug 1360777 Move legacy extension to a separate pane in about:addons

https://reviewboard.mozilla.org/r/145984/#review149928

> You mean run it through `new URL()` to catch invalid urls at runtime?  I'm not sure what we would do if we caught an exception here...

Other than catching invalid URLs (which I agree would not be something we'd be able to do much about) I was thinking more about things like assuming `SUPPORT_URL` ends with a slash and such, it's easier to do `url.pathname =` or as part of the `new URL` constructor etc.

Anyway doing this as strings is how it's done elsewhere and we control the input so I don't think it really matters.

> I'm not sure how long we're expecting to keep this around.  I thought that using prefs was generally for toolkit code that is used in different applications, I don't see an immediate need for that level of generality here...

Yeah it'd be more for folks who repackage or rebuild like Tor etc (although looking I notice they point to AMO and such)
Comment on attachment 8874640 [details]
Bug 1360777 Move legacy extension to a separate pane in about:addons

https://reviewboard.mozilla.org/r/145984/#review149928

> Other than catching invalid URLs (which I agree would not be something we'd be able to do much about) I was thinking more about things like assuming `SUPPORT_URL` ends with a slash and such, it's easier to do `url.pathname =` or as part of the `new URL` constructor etc.
> 
> Anyway doing this as strings is how it's done elsewhere and we control the input so I don't think it really matters.

Right, but to be clear, the pathname contains some elements that come from SUPPORT_URL plus the bit that we're adding here.  So I don't think the URL constructor would help us at all with the "who's including the /" issue... :/
Pushed by aswan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a1759d29a2cd
Move legacy extension to a separate pane in about:addons r=rhelmer
Comment on attachment 8874640 [details]
Bug 1360777 Move legacy extension to a separate pane in about:addons

https://reviewboard.mozilla.org/r/145984/#review151316

::: toolkit/locales/en-US/chrome/mozapps/extensions/extensions.dtd:261
(Diff revision 3)
>  <!ENTITY disabledUnsigned.devInfo.end ".">
>  
>  <!ENTITY pluginDeprecation.description "Missing something? Some plugins are no longer supported by &brandShortName;.">
>  <!ENTITY pluginDeprecation.learnMore "Learn More.">
> +
> +<!ENTITY legacyWarning.description "Missing something?  Some extensions are no longer supported by Firefox.">

We don't use two spaces after punctuation in Firefox. It might be collapsed when displayed, but still

::: toolkit/locales/en-US/chrome/mozapps/extensions/extensions.dtd:263
(Diff revision 3)
>  <!ENTITY pluginDeprecation.description "Missing something? Some plugins are no longer supported by &brandShortName;.">
>  <!ENTITY pluginDeprecation.learnMore "Learn More.">
> +
> +<!ENTITY legacyWarning.description "Missing something?  Some extensions are no longer supported by Firefox.">
> +<!ENTITY legacyWarning.showLegacy "Show legacy extensions">
> +<!ENTITY legacyExtensions.description "These extensions do not meet current Firefox standards so we deactivated them.">

Do you want to have Firefox hard-coded here? I assume you really want &brandShortName;

Same question for the next string
Sorry for the double NI, but if this needs fixing, it needs to happen very quickly given that we're 4 days from merge. In case, this is also going to block me from exposing new strings to localizers, to it's a blocker.
Flags: needinfo?(rhelmer)
Flags: needinfo?(aswan)
https://hg.mozilla.org/mozilla-central/rev/a1759d29a2cd
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Getting a patch ready for this now, thanks flod for catching that!
Flags: needinfo?(rhelmer)
Flags: needinfo?(aswan)
In this specific case is fine to land without a new string ID.
Comment on attachment 8875747 [details]
Bug 1360777 - remove double spaces and remove hardcoded Firefox for legacy extensions in about:addons

https://reviewboard.mozilla.org/r/147170/#review151670
Attachment #8875747 - Flags: review?(aswan) → review+
Pushed by aswan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7bc19dddce5d
remove double spaces and remove hardcoded Firefox for legacy extensions in about:addons r=aswan
Build-ID 20170609100251 (x64 on Debian Testing)

1.) regression: Manually imported webextensions (which don't have a legacy badge) are additionally(!) shown in the Legacy extensions tab (which doesn't have any label in the german Nightly = icon-only vertical tab).

I have set
extensions.legacy.enabled;false
extensions.legacy.exceptions;
extensions.interposition.enabled;false
extensions.webextensions.remote;true (still not possible with WebRender on Linux, but the bubble of IPvFoo (imported with Chrome Store Foxified) located inside the address bar is invisible with this pref)
xpinstall.signatures.required;false

and manually imported my favorite webextensions:
uMatrix, uBlock Origin: download the latest uBlock0.webext.xpi from https://github.com/gorhill/uBlock/releases, unzip, go into the webextension folder zip the contents as webextension.zip, import to Nightly. Since I reported https://github.com/gorhill/uBlock/issues/2576 I want and do webext-only.

https://addons.mozilla.org/firefox/addon/redirector/ (webextension) is only shown in the extensions tab (as expected).

2.) The legacy-labeled Default Theme is now gone from the Theme list. Only both Compact themes are left. I could activate the (invisible) Default Theme by deactivating the currently activated "Compact Dark" theme which hasn't a legacy badge.
Days ago I thought about opening a bug asking for prohibiting the Default theme (completely) if extensions.legacy.enabled is false. I look forward to a bleeding edge Photon non-legacy theme.
Depends on: 1371788
(In reply to Darkspirit from comment #42)
I should've called it just a bug.
In addition to 2.): "Missing something? Some extensions are no longer supported by Firefox. Show legacy extensions" is only shown inside the Themes tab. As the (now hidden) Default Theme is not shown in the new legacy extensions tab, there is no need for a legacy extensions tab if extensions.legacy.enabled is false.
Item #1 from comment 42 is bug 1371752
Item #2 is due to the fact that you changed the extensions.legacy.exceptions preference, what were you hoping to achieve by changing that?
(In reply to Andrew Swan [:aswan] from comment #44)
> Item #1 from comment 42 is bug 1371752
Thank you
> Item #2 is due to the fact that you changed the extensions.legacy.exceptions preference, what were you hoping to achieve by changing that?
To prevent any legacy theme/system-addon/addon (reduce XUL) to get rid of them (and NPAPI plugin support) as soon as possible.
At first I wondered why the legacy default theme didn't get disabled (together with the other legacy extensions) and replaced with the non-legacy Photon-like Compact light for now. But I can imagine why.
The extensions.legacy.exceptions preference just controls how things are presented in the UI, it doesn't actually change what is allowed to load/run.
Depends on: 1374584
Depends on: 1374607
Depends on: 1374637
Depends on: 1379952
Depends on: 1380598
Depends on: 1384608
Depends on: 1384610
Depends on: 1384919
Depends on: 1384940
Depends on: 1385251
Depends on: 1389559
Depends on: 1390132
Depends on: 1395892
You need to log in before you can comment on or make changes to this bug.