Closed
Bug 1329027
Opened 8 years ago
Closed 8 years ago
addon options menus don't appear
Categories
(Firefox for Android Graveyard :: Add-on Manager, defect, P1)
Tracking
(fennec+, firefox51- wontfix, firefox52+ wontfix, firefox-esr52 unaffected, firefox53+ verified, firefox54+ fixed, firefox55 verified)
VERIFIED
FIXED
Firefox 55
People
(Reporter: knipesteven, Assigned: mattw)
References
Details
(Keywords: regression)
Attachments
(2 files)
7.24 KB,
application/x-xpinstall
|
Details | |
59 bytes,
text/x-review-board-request
|
kmag
:
review+
gchang
:
approval-mozilla-aurora+
gchang
:
approval-mozilla-beta+
|
Details |
User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:50.0) Gecko/20100101 Firefox/50.0 Build ID: 20161208153507 Steps to reproduce: Install any addon with an options menu to firefox beta (51.0b9) or nightly (53.0a1) and go Tools -> Add-ons -> Select the addon Examples include uBlock Origin, Save Link Menus, and my own addon (attached). Tested on two devices, Android 6.0.1 and 5.1 Actual results: None of the options are visible. Disabling then reenabling the addon usually makes the options menu reappear. I've attached an addon I made where disabling then reenabling causes the 'Options' text to briefly appear, then disappears. This may be a problem with my addon but the primary bug applies to all addons with options. Expected results: Visible options menu. Works as expected on Firefox (50.0.2)
Comment 1•8 years ago
|
||
Hello, I didn't manage to reproduce what you described on all branches with Android 6.0.1 and 5.1 (LG G4, HTC Desire 820) and addons: uBlock Origin, Save Link Menus. If i enable and disable, the options menu is displayed. And, your addon I couldn't install on Nightly, only on Beta, Aurora. On what device did you see the issue?
Flags: needinfo?(ssk97)
Reporter | ||
Comment 2•8 years ago
|
||
The devices I used were a Moto G (1st gen) and an Nvidia SHIELD tablet K1. I may have miscommunicated, after the enable+disable the options menu is displayed, it's before that that there's a problem. Here's some pictures to try to better explain the problem. http://imgur.com/a/OBuTq Also, confirming that the latest Nightly doesn't install my addon. Not sure what the problem is there, it installed fine when I reported the bug. I'll try to look into it later.
Flags: needinfo?(ssk97)
Comment 3•8 years ago
|
||
Same as bug 1334695? When I raised that I thought it was only SDK simple prefs but I now see it affects all my addons. 3 devices - Android 4.4 and 5.1 Disabling+enabling makes the word 'OPTIONS' reappear very briefly, but the options themselves do not appear.
Many users have reported such issue with uBlock Origin: https://github.com/gorhill/uBlock/issues/1379 Unfortunately, I am not in a position to try to reproduce for Firefox Android.
Comment 5•8 years ago
|
||
[Tracking Requested - why for this release]: Add-on options may fail to display in the add-on manager. On my phone, I can reproduce this with Logview and uBlock, but not TapTranslate. For the former two, the options only appear after dis- and re-enabling the add-on, but not after a normal startup. I'm seeing this on Release as well as on Nightly - for the latter both with my regular and a fresh profile. Unless someone beats me to it, I'll try and see if mozregression yields anything useful.
Status: UNCONFIRMED → NEW
tracking-fennec: --- → ?
Has Regression Range: --- → no
Has STR: --- → yes
status-firefox51:
--- → affected
status-firefox52:
--- → affected
status-firefox53:
--- → affected
status-firefox54:
--- → affected
tracking-firefox51:
--- → ?
tracking-firefox52:
--- → ?
tracking-firefox53:
--- → ?
tracking-firefox54:
--- → ?
Ever confirmed: true
Keywords: regression,
regressionwindow-wanted
OS: Unspecified → Android
Hardware: Unspecified → All
Comment 7•8 years ago
|
||
Regressing push is https://hg.mozilla.org/integration/fx-team/pushloghtml?changeset=5b40e209b9a7, so most probably bug 1300808.
Updated•8 years ago
|
Has Regression Range: no → yes
Comment 8•8 years ago
|
||
The corresponding Adblock Plus issue is https://issues.adblockplus.org/ticket/4867.
Updated•8 years ago
|
Blocks: webext-android
Updated•8 years ago
|
tracking-fennec: ? → +
Priority: -- → P2
Comment 10•8 years ago
|
||
This seems like a severe regression. Tracking for 52 and up, though there's little time left if we want to fix this in 52.
Assignee | ||
Updated•8 years ago
|
Component: Add-on Manager → WebExtensions: Untriaged
Flags: needinfo?(mwein)
Product: Firefox for Android → Toolkit
Updated•8 years ago
|
tracking-fennec: + → ---
Component: WebExtensions: Untriaged → General
Product: Toolkit → Add-on SDK
Version: 51 Branch → unspecified
Comment 11•8 years ago
|
||
Could someone please make this high priority? Two 'stable' versions already with this. Not everyone will figure out by themselves that it is necessary to disable and re-enable affected addons in order to restore their Options. Without being able to access addons options the impression is that a core feature is missing...
Comment 12•8 years ago
|
||
As I already mentioned in comment 3, disabling and re-enabling does not restore the options of my SDK addons - originally bug 1334695. Nor does it work for others, e.g. Stylish 2.0.7 (And webextensions options_ui is not implemented yet.)
Updated•8 years ago
|
Component: General → Add-ons Manager
Product: Add-on SDK → Toolkit
Updated•8 years ago
|
Component: Add-ons Manager → Add-on Manager
Product: Toolkit → Firefox for Android
Comment 13•8 years ago
|
||
Flagging again, since this was reset during this game of hot potato...
tracking-fennec: --- → ?
status-firefox55:
--- → affected
Comment 14•8 years ago
|
||
The corresponding uBlock Origin issue is https://github.com/gorhill/uBlock/issues/1379.
Comment 15•8 years ago
|
||
Agree to P1 this regression bug. I'll get the team to look at this in the next sprint planning (mar20), but Fennec core browser team currently is quite occupied. NI to Matthew that per comment 7 there could be something relevant to bug 1300808 or Bug 1302504
tracking-fennec: ? → +
Flags: needinfo?(mwein)
Priority: P2 → P1
Assignee | ||
Comment 16•8 years ago
|
||
I confirmed that bug 1300808 is the root cause of this regression. I will follow up in a few minutes with a patch which should fix this issue.
Flags: needinfo?(mwein)
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → mwein
Comment 18•8 years ago
|
||
mozreview-review |
Comment on attachment 8847753 [details] Bug 1329027 - Fix regression caused by bug 1300808 https://reviewboard.mozilla.org/r/120672/#review123688 ::: mobile/android/chrome/content/aboutAddons.js:230 (Diff revision 1) > + if (aAddon.isWebExtension) { > + // TODO(matt): Support OPTIONS_TYPE_INLINE_BROWSER after bug 1302504 lands. > - switch (aAddon.optionsType) { > + switch (aAddon.optionsType) { > - case AddonManager.OPTIONS_TYPE_INLINE: > + case AddonManager.OPTIONS_TYPE_INLINE: > - optionsURL = aAddon.optionsURL || ""; > + optionsURL = aAddon.optionsURL || ""; > - break; > + break; > - default: > + default: > - optionsURL = ""; > + optionsURL = ""; > - } > + } > + } This doesn't make sense. A WebExtension should never have OPTIONS_TYPE_INLINE, and any other extension with inline options should have OPTIONS_TYPE_INLINE. What options type are we trying to support here?
Attachment #8847753 -
Flags: review?(kmaglione+bmo) → review-
Assignee | ||
Comment 19•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8847753 [details] Bug 1329027 - Fix regression caused by bug 1300808 https://reviewboard.mozilla.org/r/120672/#review123688 > This doesn't make sense. A WebExtension should never have OPTIONS_TYPE_INLINE, and any other extension with inline options should have OPTIONS_TYPE_INLINE. What options type are we trying to support here? I see, then I think this code should just be simplified to: ```javascript if (aAddon.isWebExtension) { optionsURL = ""; } ``` We are trying to add support for options on Android, which is tracked by bug 1302504. Android expects options to be XUl-based, so when it tries to load a URL for WebExtension options it throws an XML parsing error - this is what bug 1300808 was meant to fix, but that bug caused this regression. Do you agree or have a different suggestion?
Assignee | ||
Comment 20•8 years ago
|
||
mozreview-review-reply |
Comment on attachment 8847753 [details] Bug 1329027 - Fix regression caused by bug 1300808 https://reviewboard.mozilla.org/r/120672/#review123688 > I see, then I think this code should just be simplified to: > > ```javascript > if (aAddon.isWebExtension) { > optionsURL = ""; > } > ``` > > We are trying to add support for options on Android, which is tracked by bug 1302504. Android expects options to be XUl-based, so when it tries to load a URL for WebExtension options it throws an XML parsing error - this is what bug 1300808 was meant to fix, but that bug caused this regression. > > Do you agree or have a different suggestion? s/Android/Fennec/g
Comment 21•8 years ago
|
||
No, we should be checking the optionsType exclusively. What is the optionsType in the case where this is failing?
Assignee | ||
Comment 22•8 years ago
|
||
Okay, I see what you are saying now. It's failing when the optionsType is OPTIONS_TYPE_INLINE_BROWSER. I'll upload a new patch.
Comment hidden (mozreview-request) |
Comment 24•8 years ago
|
||
mozreview-review |
Comment on attachment 8847753 [details] Bug 1329027 - Fix regression caused by bug 1300808 https://reviewboard.mozilla.org/r/120672/#review125074 ::: mobile/android/chrome/content/aboutAddons.js:229 (Diff revision 2) > let updateable = (aAddon.permissions & AddonManager.PERM_CAN_UPGRADE) > 0; > let uninstallable = (aAddon.permissions & AddonManager.PERM_CAN_UNINSTALL) > 0; > > - // TODO(matt): Add support for OPTIONS_TYPE_INLINE_BROWSER once bug 1302504 lands. > let optionsURL; > - switch (aAddon.optionsType) { > + let optionsType = parseInt(aAddon.optionsType); Why the parseInt? In any case, no need for a separate variable here. ::: mobile/android/chrome/content/aboutAddons.js:232 (Diff revision 2) > - // TODO(matt): Add support for OPTIONS_TYPE_INLINE_BROWSER once bug 1302504 lands. > let optionsURL; > - switch (aAddon.optionsType) { > + let optionsType = parseInt(aAddon.optionsType); > + switch (optionsType) { > case AddonManager.OPTIONS_TYPE_INLINE: > - optionsURL = aAddon.optionsURL || ""; > + optionsURL = aAddon.optionsURL; Still need the `|| ""` here. ::: mobile/android/chrome/content/aboutAddons.js:234 (Diff revision 2) > + case AddonManager.OPTIONS_TYPE_INLINE_BROWSER: > + // Ignore WebExtension options until options_ui support is added - Bug 1302504. > + optionsURL = ""; > break; There's no need for a separate case for this. It's already handled by the default.
Attachment #8847753 -
Flags: review?(kmaglione+bmo)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 27•8 years ago
|
||
mozreview-review |
Comment on attachment 8847753 [details] Bug 1329027 - Fix regression caused by bug 1300808 https://reviewboard.mozilla.org/r/120672/#review125620
Attachment #8847753 -
Flags: review?(kmaglione+bmo) → review+
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 29•8 years ago
|
||
Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/59f60e7f92ca Fix regression caused by bug 1300808 r=kmag
Keywords: checkin-needed
Comment 30•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/59f60e7f92ca
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Comment 32•8 years ago
|
||
Matt, can you request uplift? This could still make the beta 7 build next Monday. I'd like to see us verify the fix in beta, too.
Flags: needinfo?(mwein)
Flags: needinfo?(ioana.bugs)
Assignee | ||
Comment 34•8 years ago
|
||
Comment on attachment 8847753 [details] Bug 1329027 - Fix regression caused by bug 1300808 Approval Request Comment [Feature/Bug causing the regression]: Bug 1300808 [User impact if declined]: Users won't see add-on options for legacy add-ons [Is this code covered by automated tests?]: No, but confirmed with manual tests [Has the fix been verified in Nightly?]: Yes [Needs manual test from QE? If yes, steps to reproduce]: Yes 1. Download a legacy add-on and a WebExtension which have Preferences and confirm they show up in about:addons. [List of other uplifts needed for the feature/fix]: N/A [Is the change risky?]: No [Why is the change risky/not risky?]: It is a simple one-line change that only affects add-on preferences. [String changes made/needed]: N/A
Flags: needinfo?(mwein)
Attachment #8847753 -
Flags: approval-mozilla-beta?
Attachment #8847753 -
Flags: approval-mozilla-aurora?
Comment 35•8 years ago
|
||
Comment on attachment 8847753 [details] Bug 1329027 - Fix regression caused by bug 1300808 Fix an addon regression that menu is missing. Aurora54+ & Beta53+.
Attachment #8847753 -
Flags: approval-mozilla-beta?
Attachment #8847753 -
Flags: approval-mozilla-beta+
Attachment #8847753 -
Flags: approval-mozilla-aurora?
Attachment #8847753 -
Flags: approval-mozilla-aurora+
Comment 36•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/3394e3fa0f1e
Comment 37•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/1b0b9e1915d1
Updated•8 years ago
|
Comment 38•8 years ago
|
||
Verified as fixed on Beta build 53.0b7. Devices: -Asus ZenPad 8.0 Z380KL (Android 6.0.1) -Huawei Honor 5X (Android 5.1.1)
Status: RESOLVED → VERIFIED
Comment 40•8 years ago
|
||
Hello, The settings appear for uBlock Origin for example but do not appear for Save Link Menus only after disabling and re-enabling the addon as mentioned in Comment 0. The issue is the same for Beta (54.0b2) and Nightly (55.0a1). Could this still be Firefox related or is the issue related to the addon?
Flags: needinfo?(jh+bugzilla)
Comment 41•8 years ago
|
||
"Save Link Menus" uses OPTIONS_TYPE_DIALOG. I don't know whether that is technically correct, although in practice it did work before bug 1300808. Maybe we should just specifically block OPTIONS_TYPE_INLINE_BROWSER if I'm understanding the motivation beyond bug 1300808 right and leave the optionsURL alone for everything else just as before?
Flags: needinfo?(jh+bugzilla) → needinfo?(mwein)
Assignee | ||
Comment 42•8 years ago
|
||
(In reply to Jan Henning [:JanH] from comment #41) > "Save Link Menus" uses OPTIONS_TYPE_DIALOG. I don't know whether that is > technically correct, although in practice it did work before bug 1300808. > > Maybe we should just specifically block OPTIONS_TYPE_INLINE_BROWSER if I'm > understanding the motivation beyond bug 1300808 right and leave the > optionsURL alone for everything else just as before? I think that sounds good. From my initial understanding I thought all XPCOM- and XUL-based add-ons used OPTIONS_TYPE_INLINE, but apparently that's not the case. Although, unless "Save Link Menus" is showing its options in a dialog box, I think it should be using OPTIONS_TYPE_INLINE. I'm working on adding support for OPTIONS_TYPE_INLINE_BROWSER in bug 1302504, so I could either fix this in that bug or upload a fix for this here depending on its urgency.
Flags: needinfo?(mwein)
Comment 43•8 years ago
|
||
It would be nice to uplift that to at least 54, so probably better here or maybe a clone of this bug?
Comment 44•8 years ago
|
||
Hi :mattw, Do you consider to uplift this to Beta54?
Flags: needinfo?(mwein)
Updated•6 years ago
|
Flags: needinfo?(ioana.bugs)
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•