Closed
Bug 1354682
Opened 7 years ago
Closed 7 years ago
Flag a legacy add-on with "legacy" in Add-ons Manager
Categories
(Toolkit :: Add-ons Manager, enhancement, P2)
Toolkit
Add-ons Manager
Tracking
()
VERIFIED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | verified |
People
(Reporter: andy+bugzilla, Assigned: aswan)
References
Details
(Whiteboard: triaged)
Attachments
(8 files)
42.45 KB,
image/jpeg
|
Details | |
29.41 KB,
image/png
|
Details | |
72.83 KB,
image/png
|
Details | |
176.96 KB,
image/jpeg
|
Details | |
59 bytes,
text/x-review-board-request
|
mossop
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
kmag
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
clouserw
:
review+
|
Details |
142.52 KB,
image/jpeg
|
Details |
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•7 years ago
|
Assignee: nobody → aswan
Priority: -- → P2
Whiteboard: triaged
Assignee | ||
Comment 1•7 years 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)
Assignee | ||
Comment 3•7 years 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)
Comment 4•7 years ago
|
||
forwarding to Emanuela as she created those mocks...
Flags: needinfo?(mjaritz) → needinfo?(emanuela)
Comment 5•7 years ago
|
||
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•7 years 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•7 years 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•7 years 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•7 years ago
|
||
Assignee | ||
Comment 10•7 years ago
|
||
This doesn't look great to me, do you want to change the styling in details Emanuela?
Flags: needinfo?(emanuela)
Comment 11•7 years 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•7 years ago
|
||
I created this jsbin, I hope it may help :) https://jsbin.com/reyuyozagi/edit?html,css,output
Assignee | ||
Comment 13•7 years 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•7 years 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•7 years ago
|
||
Here you go: https://queue.taskcluster.net/v1/task/RDjPH0iYRdiIO0OwmdvrJw/runs/0/artifacts/public/build/target.dmg
Comment 16•7 years ago
|
||
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•7 years ago
|
Attachment #8862189 -
Flags: review?(dtownsend)
Comment 18•7 years 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•7 years 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)
Comment 20•7 years ago
|
||
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•7 years 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•7 years ago
|
Attachment #8862632 -
Flags: review?(wclouser)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 29•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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•7 years 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
Comment 42•7 years ago
|
||
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•7 years 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
Comment 47•7 years ago
|
||
bugherder |
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
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Comment 48•7 years ago
|
||
I notice that Hybrid Webextension add-ons are flagged as Legacy. Intentional or not?
Comment 49•7 years ago
|
||
(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•7 years 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•7 years 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.
Comment 52•7 years ago
|
||
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•7 years 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•7 years 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•7 years ago
|
||
> 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 ;)
Comment 56•7 years ago
|
||
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
You need to log in
before you can comment on or make changes to this bug.
Description
•