Closed Bug 1329027 Opened 3 years ago Closed 2 years ago

addon options menus don't appear

Categories

(Firefox for Android :: Add-on Manager, defect, P1)

All
Android
defect

Tracking

()

VERIFIED FIXED
Firefox 55
Tracking Status
fennec + ---
firefox51 - wontfix
firefox52 + wontfix
firefox-esr52 --- unaffected
firefox53 + verified
firefox54 + fixed
firefox55 --- verified

People

(Reporter: ssk97, Assigned: mattw)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(2 files)

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)
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)
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)
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.
[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
Ever confirmed: true
OS: Unspecified → Android
Hardware: Unspecified → All
Duplicate of this bug: 1334695
Regressing push is https://hg.mozilla.org/integration/fx-team/pushloghtml?changeset=5b40e209b9a7, so most probably bug 1300808.
Blocks: 1300808
Flags: needinfo?(mwein)
Has Regression Range: no → yes
The corresponding Adblock Plus issue is https://issues.adblockplus.org/ticket/4867.
Blocks: abp
Too late for 51. Mark 51 won't fix.
tracking-fennec: ? → +
Priority: -- → P2
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.
Component: Add-on Manager → WebExtensions: Untriaged
Flags: needinfo?(mwein)
Product: Firefox for Android → Toolkit
tracking-fennec: + → ---
Component: WebExtensions: Untriaged → General
Product: Toolkit → Add-on SDK
Version: 51 Branch → unspecified
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...
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.)
Component: General → Add-ons Manager
Product: Add-on SDK → Toolkit
Component: Add-ons Manager → Add-on Manager
Product: Toolkit → Firefox for Android
Flagging again, since this was reset during this game of hot potato...
tracking-fennec: --- → ?
The corresponding uBlock Origin issue is https://github.com/gorhill/uBlock/issues/1379.
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
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)
Assignee: nobody → mwein
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-
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?
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
No, we should be checking the optionsType exclusively. What is the optionsType in the case where this is failing?
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 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 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+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/59f60e7f92ca
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Duplicate of this bug: 1350327
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)
Now working again with Nightly, thanks.
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 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+
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
Duplicate of this bug: 1356877
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)
"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)
(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)
It would be nice to uplift that to at least 54, so probably better here or maybe a clone of this bug?
Hi :mattw,
Do you consider to uplift this to Beta54?
Flags: needinfo?(mwein)
There's nothing new to uplift here.
Blocks: 1360448
Will fix in bug 1360448.
Flags: needinfo?(mwein)
Flags: needinfo?(ioana.bugs)
You need to log in before you can comment on or make changes to this bug.