[BrowserAction] properties supported on desktop but not android should warn rather than prevent installation

VERIFIED FIXED in Firefox 55

Status

defect
P2
normal
VERIFIED FIXED
2 years ago
11 months ago

People

(Reporter: mattw, Assigned: mattw)

Tracking

(Blocks 1 bug)

unspecified
mozilla56
Unspecified
Android
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed, firefox56 verified)

Details

(Whiteboard: [browserAction]triaged)

Attachments

(3 attachments)

Assignee

Description

2 years ago
The first version of the browser action API on android relies on the title of the browser action, instead of the icon, for displaying it in the browser's main menu - meaning that the extension should still function without default_icon.

Currently, extensions using default_icon on desktop will fail to install on android because this property is marked as unsupported. Instead, we should provide a warning when default_icon is used to allow these extensions to install successfully.

Comment 1

2 years ago
Would it be possible to fix this quickly and uplift to 55? Having a scenario where the manifest.json has to change dramatically between Desktop and Android is something that we really don't have a good story for the developers or on AMO yet.
Flags: needinfo?(mwein)
Assignee

Comment 2

2 years ago
Yeah, it should just be a matter or adding "deprecated" with a warning message to the schema for default_icon - I'll upload a patch today.
Flags: needinfo?(mwein)
Assignee

Updated

2 years ago
Summary: [BrowserAction] default_icon on android should warn rather than prevent installation → [BrowserAction] properties supported on desktop but not android should warn rather than prevent installation
Assignee

Updated

2 years ago
Assignee: nobody → mwein
Comment hidden (mozreview-request)

Comment 5

2 years ago
Shouldn't properties supported on desktop, but not on Android, rather be just ignored. Otherwise you would encourage people to provide different builds for desktop and Android, which seems to be unnecessary.

Comment 6

2 years ago
mozreview-review
Comment on attachment 8877695 [details]
Bug 1367494 - Warn on properties supported on desktop but not android instead of preventing installation

https://reviewboard.mozilla.org/r/149126/#review153568

I would remove "at this time" from the deprecation message but up to you...
Attachment #8877695 - Flags: review?(aswan) → review+

Updated

2 years ago
Duplicate of this bug: 1371887
Assignee

Comment 8

2 years ago
(In reply to sebastian.noack from comment #5)
> Shouldn't properties supported on desktop, but not on Android, rather be
> just ignored. Otherwise you would encourage people to provide different
> builds for desktop and Android, which seems to be unnecessary.

If we disabled default_icon on desktop, we would break many existing desktop addons which use that property. On the other hand, the first version of the browser action API on android doesn't use default_icon, but instead uses default_title to add menu items for each browser action. An addon that is designed to work on both desktop and android will fail to install on android if it uses default_icon or any other browser specific property. By marking these properties as deprecated instead of unsupported, we will instead log warnings to the console, which will allow addons designed to work both on desktop and android to install successfully on android.
Comment hidden (mozreview-request)

Comment 10

2 years ago
Pushed by mwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ed85f2ec9f2a
Warn on properties supported on desktop but not android instead of preventing installation r=aswan
https://hg.mozilla.org/mozilla-central/rev/ed85f2ec9f2a
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56

Comment 12

2 years ago
I wasn't suggesting to unsupport these properties on desktop, but to not issue a warning on Android and instead rather silently ignore them there.

Comment 13

2 years ago
Posted file Video+Logs.zip
The installation of the next WebExtension https://addons-dev.allizom.org/en-US/android/addon/open-my-page/ that uses default_icon in the browser_action works, but I don’t see any warnings displayed in the remote browser console for the unsupported properties.
 
Verified on Fennec 56.0a1 (2017-06-20) under Android 7.1.2
 
Any thoughts about this Matthew ?
Flags: needinfo?(mwein)
Hi! I have just added a bunch of workarounds for my extension to get it to load on Firefox for Android 55 Beta.
This includes things like adding:
<link rel="stylesheet" type="text/css" href="chrome://browser/content/extension.css" />
<link rel="stylesheet" type="text/css" href="chrome://browser/content/extension-mac.css" />
to the browser_action/default_popup file or declaring the browser action icon in code instead of the manifest (which means that, on desktop Firefox, the default "puzzle piece" icon for be visible on startup in the toolbar).

Since these changes have not been uplifted to Firefox for Android 55, I take it that I'll have to maintain these workarounds for a couple of releases? Is there any chance if this still becoming part of the 55 Beta version?
I'll take that as a no, then. Thx, bbl :-(
Hi Cosmin,
can you check also the messages logged to "adb logcat" while the extension is installing?

these "deprecated" warnings are not visible to the user, but they should have been logged in the "adb logcat" logs and in the browser console (the "Main Process" when connecting from WebIDE to the Firefox for Android instance).
Flags: needinfo?(matthewjwein) → needinfo?(cosmin.badescu)
Comment on attachment 8877695 [details]
Bug 1367494 - Warn on properties supported on desktop but not android instead of preventing installation

Approval Request Comment
[Feature/Bug causing the regression]:
- Bug 1331742 - Add support for browserAction.onClicked
  When the first pieces of the browserAction API have been implemented for Android (during the Firefox 55 cycle), the browserAction API schema has been added to Firefox for Android and, by marking the browser_action manifest properties as unsupported, we have started to prevent the installation of addons with these properties in the manifest as a unintended side-effect.

[User impact if declined]:
Firefox for Android 55 will block the installation of any extension which has these properties in the manifest, which would force the addon developer to create to different add-ons with different manifest.json files to be able to support both Firefox Desktop and Firefox for Android.

[Is this code covered by automated tests?]:
This patch doesn't contain new automated tests.

(but the related "deprecated" and "unsupported" feature of the API schemas have their own tests: http://searchfox.org/mozilla-central/rev/ad093e98f42338effe2e2513e26c3a311dd96422/toolkit/components/extensions/test/xpcshell/test_ext_schemas.js)

[Has the fix been verified in Nightly?]:
It has been manually verified that the installation is not prevented anymore
(testing the logged message is still ongoing) 

[Needs manual test from QE? If yes, steps to reproduce]:
Yes, we should use the same STR that we are already using to manually verify the behavior on this fix on nightly.
 
[List of other uplifts needed for the feature/fix]:
None

[Is the change risky?]:
Risks should be pretty low.

[Why is the change risky/not risky?]:
This change is restricted to the JSON of a single API schema file, 
and it is restricted to the "WebExtension" add-ons. 

[String changes made/needed]:
None
Attachment #8877695 - Flags: approval-mozilla-beta?
Comment on attachment 8877695 [details]
Bug 1367494 - Warn on properties supported on desktop but not android instead of preventing installation

I don't think this meets the bar for uplift this late in the cycle (we only have a single beta build left)
Attachment #8877695 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
Just wanted to mention that I currently receive an error popup about some "failed add-on installation" due to the add-on being "broken" every time I start Firefox Beta for Android that is very likely related to this backwards-incompatible change (even if it's "just" one version, it's still breaking stuff nonetheless).

ofc, Luca already gave a much more in-depth description of this, so I guess I'll be ignored like last time.

Just wanted to mention that this is indeed an issue that users (aside from me) will actually notice…

Comment 20

2 years ago
Posted file Warnings+Logs.zip
Thanks Luca!

The “deprecated” warnings are displayed in the WebIDE “Main Process”.
Please see the attached files.

This issue is verified as fixed on Fennec 56.0a1 (2017-07-27) under Android 7.1.2.
Flags: needinfo?(cosmin.badescu)

Updated

2 years ago
Status: RESOLVED → VERIFIED

Updated

2 years ago
Duplicate of this bug: 1385455

Comment 22

2 years ago
For what it's worth, here is my feedback re. this issue.

I was really hoping this would be fixed for Firefox for Android 55. If the fix is only available for Firefox for Android 56, this is seriously delaying the publishing the webext version of my extension which I was planning to 1st release on August 22nd on stable AMO channel[1]. My extension is one of those cases requiring an intermediate embedded-webext version in order to seamlessly migrate user data from legacy to webext storage.

[1] https://github.com/gorhill/uBlock/wiki/Firefox-WebExtensions

Comment 23

2 years ago
Sorry, wish we'd known earlier. But its too late to uplift to 55 which is being merged to release today(ish).

Comment 24

2 years ago
If there's a point release for 55, I'd like to nominate this patch. It's very low risk and causes lots of add-ons to have problems.
Flags: needinfo?(jcristau)
Comment on attachment 8877695 [details]
Bug 1367494 - Warn on properties supported on desktop but not android instead of preventing installation

comments 17 and 24
Flags: needinfo?(jcristau)
Attachment #8877695 - Flags: approval-mozilla-release?
Comment on attachment 8877695 [details]
Bug 1367494 - Warn on properties supported on desktop but not android instead of preventing installation

we're going to build a rc2, so based on the impact on lots of add-ons, let's take this one on 55
Attachment #8877695 - Flags: approval-mozilla-release? → approval-mozilla-release+

Updated

11 months ago
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.