Flag a legacy add-on with "legacy" in Add-ons Manager

VERIFIED FIXED in Firefox 55

Status

()

Toolkit
Add-ons Manager
P2
normal
VERIFIED FIXED
5 months ago
3 months ago

People

(Reporter: andym, Assigned: aswan)

Tracking

(Blocks: 1 bug)

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 verified)

Details

(Whiteboard: triaged)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(8 attachments)

(Reporter)

Description

5 months ago
This is the start of the plan for showing users what add-ons won't make the cut for Firefox 57. It's about adding a flag "legacy" into add-ons manager on each add-on that is not a WebExtension.

You can see this in the invision mock here: https://mozilla.invisionapp.com/share/HUAUGBGWZ#/screens/227774581

Please note that this does not depend upon the pref being added in bug 1336576.  Because we'll want this to appear before the pref is enabled as a warning.

It should link over to the SUMO page.

Updated

4 months ago
Assignee: nobody → aswan
Priority: -- → P2
Whiteboard: triaged
(Assignee)

Comment 1

4 months ago
The invision mockups specify that this should happen in 56.  Is there any reason not to land this earlier (ie in 55)?
Flags: needinfo?(mjaritz)
Please land earlier if possible. Thanks.
Flags: needinfo?(mjaritz)
(Assignee)

Comment 3

4 months ago
Okay, invision also only shows the "Legacy" badge in the list view, I presume the detail view for an individual addon should just show the same badge next to the name?
Flags: needinfo?(mjaritz)
(Assignee)

Updated

4 months ago
Depends on: 1354684
forwarding to Emanuela as she created those mocks...
Flags: needinfo?(mjaritz) → needinfo?(emanuela)

Comment 5

4 months ago
Created attachment 8860956 [details]
Spec for the "Legacy" badge

Aswan: yes, the badge always follows the name, even on the details page.

Attached the spec for the badge.

Badge
Padding: 4px
Width: 100%
Bg color: #FFE900
hover bg color: #D7B600 

Badge typography
font-size: 10px
letter-spacing: 0.5px
line-height: 14px (1.4)
color: #3E2800
Flags: needinfo?(emanuela)
(Assignee)

Comment 6

4 months ago
All the other typography sizing is in em (rem actually) not px.  Is there a specific reason to use px here or can we convert these to rem?
Flags: needinfo?(emanuela)

Comment 7

4 months ago
:aswan sure you can convert it in rem (or rem) :)

If :root font-size is 16px, the font-size for the badge is 0.625rem.
Flags: needinfo?(emanuela)
(Assignee)

Comment 8

4 months ago
Okay, 1 rem is 11px so we can't actually do this precisely.  If I do 0.91rem that gives us 10.1px.
Also:

(In reply to emanuela [ux team] from comment #5)
> letter-spacing: 0.5px
> line-height: 14px (1.4)

When I apply these manually from the browser toolbox, neither of these appears to have any effect.  I guess maybe xul labels don't honor letter-spacing?  And I'm not sure what effect you meant to get with line-height.  But I'll omit these unless there's some reason I'm overlooking why they matter.
(Assignee)

Comment 9

4 months ago
Created attachment 8861042 [details]
sceen shot of badge
(Assignee)

Comment 10

4 months ago
Created attachment 8861056 [details]
sceen shot of badge in details

This doesn't look great to me, do you want to change the styling in details Emanuela?
Flags: needinfo?(emanuela)
(Assignee)

Updated

4 months ago
Blocks: 1359276

Comment 11

4 months ago
Hey Andrew, yeah. I agree. It doesn't look good to me either. Any changes you can upload the badge somewhere so I can play around with the numbers in the inspector?

Anyway, I think you need to add some padding on top. You should have the impression to have the same space all around the word.
Flags: needinfo?(emanuela)

Comment 12

4 months ago
I created this jsbin, I hope it may help :)

https://jsbin.com/reyuyozagi/edit?html,css,output
(Assignee)

Comment 13

4 months ago
(In reply to emanuela [ux team] from comment #11)
> Any changes
> you can upload the badge somewhere so I can play around with the numbers in
> the inspector?

Sure, if I put a patch up can you apply it locally?  Or should I do a build?  If so, which platform?  (Mac?)
Flags: needinfo?(emanuela)

Comment 14

4 months ago
(In reply to Andrew Swan [:aswan] from comment #13)
> (In reply to emanuela [ux team] from comment #11)
> > Any changes
> > you can upload the badge somewhere so I can play around with the numbers in
> > the inspector?
> 
> Sure, if I put a patch up can you apply it locally?  Or should I do a build?
> If so, which platform?  (Mac?)

A build for Mac works, thanks!
Flags: needinfo?(emanuela)
(Assignee)

Comment 15

4 months ago
Here you go:
https://queue.taskcluster.net/v1/task/RDjPH0iYRdiIO0OwmdvrJw/runs/0/artifacts/public/build/target.dmg

Comment 16

4 months ago
Created attachment 8861512 [details]
mockup for legacy badge

Thanks, Aswan.

So, here the style for the legacy warning

.legacy-warning {
  background-color: #FFE900;
  color: #3E2800;
  padding: 4px 5px 3px;
  font-size: 0.9rem;
  font-weight: 600;
}


In the detail page of an add-on, we need to use a container around the legacy-warning to align the badge nicely with the name. 

Here what I did, not sure if <hbox> is the right element.

<hbox id="legacy-warning-container" class="legacy-warning-container" align="start">
  <label class="legacy-warning" value="LEGACY"/>
</hbox>

.legacy-warning-container {
  margin-top: 0.78rem;
}

I attached a .jpg with the screenshot of the two screens.
Comment hidden (mozreview-request)
(Assignee)

Updated

4 months ago
Attachment #8862189 - Flags: review?(dtownsend)

Comment 18

4 months ago
mozreview-review
Comment on attachment 8862189 [details]
Bug 1354682 Add legacy badges in about:addons

https://reviewboard.mozilla.org/r/134172/#review137360

r+ as is but this will mark test pilot extensions as legacy and I suspect we don't want that. Let's figure that out before landing this.

::: toolkit/mozapps/extensions/content/extensions.js:3073
(Diff revision 1)
> +      // themes as legacy.  There's no explicit flag for complete
> +      // themes so the other types of themes are enumerated:
> +      //  1. new style themes for which isWebExtension is true
> +      //  2. the default theme
> +      //  3. lightweight themes, which have ids with @personas.m.o
> +      legacy = !(aAddon.isWebExtension || aAddon.name == "Default" ||

Use the actual ID for the default theme here please

::: toolkit/mozapps/extensions/content/extensions.xml:763
(Diff revision 1)
> +            // themes as legacy.  There's no explicit flag for complete
> +            // themes so the other types of themes are enumerated:
> +            //  1. new style themes for which isWebExtension is true
> +            //  2. the default theme
> +            //  3. lightweight themes, which have ids with @personas.m.o
> +            if (this.mAddon.isWebExtension || this.mAddon.name == "Default" ||

Same again
Attachment #8862189 - Flags: review?(dtownsend) → review+
(Assignee)

Comment 19

4 months ago
I'll revise this to also consult a list of exceptions that we can use for test pilot (and anything else that might come along, gecko profiler?)
Wil to provide the initial list of test pilot exceptions.
Flags: needinfo?(wclouser)
Existing experiments:

> testpilot@cliqz.com
> @testpilot-containers
> jid1-NeEaf3sAHdKHPA@jetpack
> @activity-streams
> pulse@mozilla.com
> @testpilot-addon
> @min-vid
> tabcentertest1@mozilla.com
> snoozetabs@mozilla.com

And two upcoming experiments:

> speaktome@mozilla.com
> hoverpad@mozilla.com
Flags: needinfo?(wclouser)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 24

4 months ago
Changes in the latest patches:
- extensions signed with "Mozilla Extensions" are not shown as legacy, but we don't have any such extensions yet so:
- extensions that appear in the extensions.legacy.exceptions pref are not shown as legacy
- we never show the legacy badge in search results since we don't have that information available
(Assignee)

Updated

4 months ago
Attachment #8862632 - Flags: review?(wclouser)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 29

4 months ago
mozreview-review
Comment on attachment 8862189 [details]
Bug 1354682 Add legacy badges in about:addons

https://reviewboard.mozilla.org/r/134172/#review137478

So I think the only thing that bugs me about this is that I think we should get rid of the pref once we have the signing infrastructure set up. That means at that point we'll need to hardcode the default theme somewhere. So just remember to do that later.

::: toolkit/mozapps/extensions/content/extensions.js:3075
(Diff revisions 1 - 3)
> -      legacy = true;
> +        legacy = true;
> -    }
> +      }
> -    if (aAddon.type == "theme") {
> +      if (aAddon.type == "theme") {
> -      // The logic here is kind of clunky but we want to mark complete
> +        // The logic here is kind of clunky but we want to mark complete
> -      // themes as legacy.  There's no explicit flag for complete
> +        // themes as legacy.  There's no explicit flag for complete
> -      // themes so the other types of themes are enumerated:
> +        // themes so explicitly check for new sytle themes (for which

s/sytle/style/

::: toolkit/mozapps/extensions/content/extensions.xml:763
(Diff revisions 1 - 3)
> -            return true;
> +            legacy = true;
>            }
>            if (this.mAddon.type == "theme") {
>              // The logic here is kind of clunky but we want to mark complete
>              // themes as legacy.  There's no explicit flag for complete
> -            // themes so the other types of themes are enumerated:
> +            // themes so explicitly check for new sytle themes (for which

s/sytle/style/
(Assignee)

Comment 30

4 months ago
mozreview-review-reply
Comment on attachment 8862189 [details]
Bug 1354682 Add legacy badges in about:addons

https://reviewboard.mozilla.org/r/134172/#review137478

I figured we would rip all of this out in 57 since either legacy addons will be fully disabled with corresponding UI indicators or a user on a non-release build has flipped a preference.  In either case, there's no reason to show this badge.  That's close enough that I propose just sticking with this until 57, but I could be persuaded otherwise...
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 33

4 months ago
mozreview-review
Comment on attachment 8862631 [details]
Bug 1354682 Add transform to XPCOMUtils.defineLazyPreferenceGetter

https://reviewboard.mozilla.org/r/134480/#review137728

::: js/xpconnect/loader/XPCOMUtils.jsm:331
(Diff revision 1)
>              let previous = this.value;
>  
>              // Fetch and cache value.
>              this.value = undefined;
>              let latest = lazyGetter();
> -            aOnUpdate(data, previous, latest);
> +            aOnUpdate.apply(aObject, data, previous, latest);

Why this change?

::: js/xpconnect/tests/unit/test_xpcomutils.js:165
(Diff revision 1)
> +    Preferences.set(PREF, "newValue");
> +    equal(obj.pref, "newValue-changed", "transform is applied to updated value");
> +
> +    Preferences.reset(PREF);
> +    equal(obj.pref, "defaultValue-changed", "transform is applied to reset default");
> +    

Nit: whitespace
(Assignee)

Comment 34

4 months ago
mozreview-review-reply
Comment on attachment 8862631 [details]
Bug 1354682 Add transform to XPCOMUtils.defineLazyPreferenceGetter

https://reviewboard.mozilla.org/r/134480/#review137728

> Why this change?

Kris requested it...

Comment 35

4 months ago
mozreview-review
Comment on attachment 8862632 [details]
Bug 1354682 Initial list of exceptions for the legacy addon warning

https://reviewboard.mozilla.org/r/134482/#review137778

r+ for the test pilot IDs.  I don't know what {972ce4c6-7e08-4474-a285-3208198ce6fd} is or if there are any other add-ons we should add
Attachment #8862632 - Flags: review?(wclouser) → review+

Comment 36

4 months ago
mozreview-review
Comment on attachment 8862631 [details]
Bug 1354682 Add transform to XPCOMUtils.defineLazyPreferenceGetter

https://reviewboard.mozilla.org/r/134480/#review137780

::: js/xpconnect/loader/XPCOMUtils.jsm:311
(Diff revision 1)
>     */
>    defineLazyPreferenceGetter: function XPCU_defineLazyPreferenceGetter(
>                                     aObject, aName, aPreference,
>                                     aDefaultValue = null,
> -                                   aOnUpdate = null)
> +                                   aOnUpdate = null,
> +                                   aTransform = null)

Please just make this `aTransform = val => val` or something, and then just always call `aTransform` on the value.

::: js/xpconnect/tests/unit/test_xpcomutils.js:156
(Diff revision 1)
>  
>      equal(obj.pref, "defaultValue", "Should return default value after pref is reset");
>  
> +    obj = {};
> +    XPCOMUtils.defineLazyPreferenceGetter(obj, "pref", PREF, "defaultValue",
> +                                          null, value => `${value}-changed`);

Can you have this return an array, or something, instead just so we're sure there's no type coercion on the result?
Attachment #8862631 - Flags: review?(kmaglione+bmo) → review+
(Assignee)

Comment 37

4 months ago
mozreview-review-reply
Comment on attachment 8862632 [details]
Bug 1354682 Initial list of exceptions for the legacy addon warning

https://reviewboard.mozilla.org/r/134482/#review137778

Sorry, should have noted that the uuid you mentinoed is the default theme.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 41

4 months ago
Pushed by aswan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9374009a95e5
Add transform to XPCOMUtils.defineLazyPreferenceGetter r=kmag
https://hg.mozilla.org/integration/autoland/rev/56b8122e64a3
Add legacy badges in about:addons r=mossop
https://hg.mozilla.org/integration/autoland/rev/55405fd328f9
Initial list of exceptions for the legacy addon warning r=clouserw
Backed out in https://hg.mozilla.org/integration/autoland/rev/fe58bf6ff795 for, I kid you not, sessionstore bustage, https://treeherder.mozilla.org/logviewer.html#?job_id=95375697&repo=autoland
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 46

4 months ago
Pushed by aswan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/049c87b522f3
Add transform to XPCOMUtils.defineLazyPreferenceGetter r=kmag
https://hg.mozilla.org/integration/autoland/rev/1ddcc6f6bde2
Add legacy badges in about:addons r=mossop
https://hg.mozilla.org/integration/autoland/rev/53ceb9f0431c
Initial list of exceptions for the legacy addon warning r=clouserw
https://hg.mozilla.org/mozilla-central/rev/049c87b522f3
https://hg.mozilla.org/mozilla-central/rev/1ddcc6f6bde2
https://hg.mozilla.org/mozilla-central/rev/53ceb9f0431c
Status: NEW → RESOLVED
Last Resolved: 4 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55

Comment 48

4 months ago
I notice that Hybrid Webextension add-ons are flagged as Legacy.  Intentional or not?
(In reply to Gary [:streetwolf] from comment #48)
> I notice that Hybrid Webextension add-ons are flagged as Legacy. 
> Intentional or not?

Intentional.

Comment 50

4 months ago
So this means that all current complete themes will become obsolete?
Or is/will be there a way to convert them to the new system?
(Assignee)

Comment 51

4 months ago
(In reply to Alfred Kayser from comment #50)
> So this means that all current complete themes will become obsolete?

yes

> Or is/will be there a way to convert them to the new system?

Not everything has a direct analogue in the new system, see https://blog.mozilla.org/addons/2017/02/24/improving-themes-in-firefox/
This bug isn't a good place for this discussion, further questions should either be on the mailing list or in new bugs.
Tested this issue on Firefox 55.0a1 (2017-05-01) under Windows 10 64-bit, Mac OS X 10.12.1 and Ubuntu 16.04 32-bit and noticed the following potential issues:

 1.Yellow frame not displayed while using a complete theme: https://www.screencast.com/t/pn9T5Ja6c2 , https://www.screencast.com/t/p2uiAxESE0b0

 2.The badge is not aligned with the name in add-on details tab on Windows 10 64-bit: https://www.screencast.com/t/7vBxclqpJ

 3.The label is not displayed at all in Extensions tab and is zoomed in add-on details tab while using Classic Theme Restorer add-on (https://addons.mozilla.org/en-US/firefox/addon/classicthemerestorer/?src=cb-dl-users): https://www.screencast.com/t/xkDQlV9nqn and https://www.screencast.com/t/gcS8Qqln

 4.Will the same yellow-style be maintained also for a disabled add-on? https://www.screencast.com/t/6x6L0M01CZ0

 5.“Legacy” word is bolded on Ubuntu 16.04 32-bit on both Extensions and add-on detail tabs: https://www.screencast.com/t/AcWF1UXHVy

 6.The label seems a bit smaller in add-on details tab on Ubuntu 16.04 32-bit: https://www.screencast.com/t/bnxlXG19HfQ


Please let me know for which of the above mentioned issues should I file new bugs.
Flags: needinfo?(aswan)
(Assignee)

Comment 53

4 months ago
(In reply to Vasilica Mihasca, QA [:vasilica_mihasca] from comment #52)
> Tested this issue on Firefox 55.0a1 (2017-05-01) under Windows 10 64-bit,
> Mac OS X 10.12.1 and Ubuntu 16.04 32-bit and noticed the following potential
> issues:
> 
>  1.Yellow frame not displayed while using a complete theme:
> https://www.screencast.com/t/pn9T5Ja6c2 ,
> https://www.screencast.com/t/p2uiAxESE0b0

Correct, complete themes override all the browser styling.  Unless/until the complete them author updates the theme, this is expected.

>  2.The badge is not aligned with the name in add-on details tab on Windows
> 10 64-bit: https://www.screencast.com/t/7vBxclqpJ

File a followup for this I guess with a needinfo to emanuela, she specified all the margins etc.

>  3.The label is not displayed at all in Extensions tab and is zoomed in
> add-on details tab while using Classic Theme Restorer add-on
> (https://addons.mozilla.org/en-US/firefox/addon/classicthemerestorer/?src=cb-
> dl-users): https://www.screencast.com/t/xkDQlV9nqn and
> https://www.screencast.com/t/gcS8Qqln

I think this is basically the same issue as #1, CTR changes the styles on the page so when we change the contents of the page, it breaks.

>  4.Will the same yellow-style be maintained also for a disabled add-on?
> https://www.screencast.com/t/6x6L0M01CZ0
>
>  5.“Legacy” word is bolded on Ubuntu 16.04 32-bit on both Extensions and
> add-on detail tabs: https://www.screencast.com/t/AcWF1UXHVy
> 
>  6.The label seems a bit smaller in add-on details tab on Ubuntu 16.04
> 32-bit: https://www.screencast.com/t/bnxlXG19HfQ

These are all questions for emanuela, needinfo to her.
Flags: needinfo?(aswan) → needinfo?(emanuela)

Comment 54

4 months ago
> 2.The badge is not aligned with the name in add-on details tab on Windows
> 10 64-bit: https://www.screencast.com/t/7vBxclqpJ

I see. Unfortunately, I don't know how to solve the small dis-alignment. I think we need a front-end XUL dev who can make it better than my workaround. However, since the alignment is not completely off, I'm ok to ship it as it is right now and maybe fix it when we are adding the link.


> 4.Will the same yellow-style be maintained also for a disabled add-on?
> https://www.screencast.com/t/6x6L0M01CZ0

Yes. 

> 5.“Legacy” word is bolded on Ubuntu 16.04 32-bit on both Extensions and
> add-on detail tabs: https://www.screencast.com/t/AcWF1UXHVy

From your screenshot, I noticed how everything is bolder so, even if it looks different, it's too off in that context.
However, if we can ship different CSS according to the platform, I will suggest removing the {font-weight: 600;} declaration in the code for Ubuntu.
Flags: needinfo?(emanuela)

Comment 55

4 months ago
Created attachment 8864100 [details]
Legacy badge in linux, same size

>  6.The label seems a bit smaller in add-on details tab on Ubuntu 16.04
> 32-bit: https://www.screencast.com/t/bnxlXG19HfQ

It's not! Check the attachment ;)
Filed a new bug also for the Ubuntu issue in order to decide how achievable it is: Bug 1361971
Based on Comments 52, 53, 54, 55 and considering that the other issues are tracked separately I am marking this bug as verified on Firefox 55.0a1.
Status: RESOLVED → VERIFIED
status-firefox55: fixed → verified

Updated

3 months ago
Depends on: 1365079
You need to log in before you can comment on or make changes to this bug.