Add support for options_ui on Android

RESOLVED FIXED in Firefox 56

Status

P2
normal
RESOLVED FIXED
3 years ago
9 months ago

People

(Reporter: mattw, Assigned: rpl)

Tracking

(Blocks: 2 bugs, {meta})

Trunk
mozilla56
Dependency tree / graph

Firefox Tracking Flags

(firefox51 affected, firefox56 verified)

Details

(Whiteboard: triaged)

Attachments

(7 attachments)

Comment hidden (empty)

Updated

3 years ago
Blocks: 467520

Updated

3 years ago
Whiteboard: triaged

Updated

3 years ago
Blocks: 1226547
No longer blocks: 467520

Updated

3 years ago
Duplicate of this bug: 1292347

Updated

2 years ago
webextensions: --- → +

Updated

2 years ago
Assignee: nobody → mwein

Updated

2 years ago
Blocks: 1185785
(Reporter)

Updated

2 years ago
Attachment #8855447 - Flags: feedback?(kmaglione+bmo)

Comment 3

2 years ago
mozreview-review
Comment on attachment 8855447 [details]
Bug 1302504 - use mozbrowser to add support for options_ui on Android

https://reviewboard.mozilla.org/r/127308/#review130042

::: mobile/android/chrome/content/aboutAddons.js:28
(Diff revision 1)
> +XPCOMUtils.defineLazyModuleGetter(this, "AppConstants",
> +                                  "resource://gre/modules/AppConstants.jsm");

This looks unused?

::: mobile/android/chrome/content/aboutAddons.js:356
(Diff revision 1)
> -    // Retrieve the extensions preferences
> -    try {
> -      let optionsURL = aListItem.getAttribute("optionsURL");
> +    let optionsURL = aListItem.getAttribute("optionsURL");
> +    switch (parseInt(addon.optionsType)) {
> +      case AddonManager.OPTIONS_TYPE_INLINE_BROWSER:
> +        optionsHeader.classList.add("hidden");

I'm not sure we really want to hide this. But it we do, please remove "hidden" before the switch-case, and re-add it when it's necessary.

::: mobile/android/chrome/content/aboutAddons.js:365
(Diff revision 1)
> +    let list = document.querySelector("#addons-list");
> +    list.style.display = "none";
> +    let details = document.querySelector("#addons-details");
> +    details.style.display = "block";
> +    document.documentElement.setAttribute("details", "true");

Hm. This is kind of gross, but I guess it's not your fault.

::: mobile/android/chrome/content/aboutAddons.js:373
(Diff revision 1)
> +    details.style.display = "block";
> +    document.documentElement.setAttribute("details", "true");
> +  },
> +
> +  createWebExtensionOptions: async function(destination, optionsURL, browserStyle) {
> +    let frame = document.createElementNS("http://www.w3.org/1999/xhtml", "html:iframe");

No need for the "html:" prefix. Or the namespace. This is already an XHTML document with a default namespace, so just `document.createElement("iframe")`

::: mobile/android/chrome/content/aboutAddons.js:374
(Diff revision 1)
> +    document.documentElement.setAttribute("details", "true");
> +  },
> +
> +  createWebExtensionOptions: async function(destination, optionsURL, browserStyle) {
> +    let frame = document.createElementNS("http://www.w3.org/1999/xhtml", "html:iframe");
> +    frame.setAttribute("id", "options");

Please use the same ID that we use for the desktop <browser>.
Attachment #8855447 - Flags: feedback?(kmaglione+bmo) → feedback+
(Reporter)

Comment 4

2 years ago
mozreview-review-reply
Comment on attachment 8855447 [details]
Bug 1302504 - use mozbrowser to add support for options_ui on Android

https://reviewboard.mozilla.org/r/127308/#review130042

> This looks unused?

Removed.

> I'm not sure we really want to hide this. But it we do, please remove "hidden" before the switch-case, and re-add it when it's necessary.

On desktop we don't show the options header for webextions options, so I think we should do the same here.

> Hm. This is kind of gross, but I guess it's not your fault.

Yeah, it's not ideal. I'll see if I can make this cleaner.

> No need for the "html:" prefix. Or the namespace. This is already an XHTML document with a default namespace, so just `document.createElement("iframe")`

Nice, will update.

> Please use the same ID that we use for the desktop <browser>.

I didn't see one for desktop so I added an ID there. Let me know if I should do something different.
(Reporter)

Comment 5

2 years ago
mozreview-review-reply
Comment on attachment 8855447 [details]
Bug 1302504 - use mozbrowser to add support for options_ui on Android

https://reviewboard.mozilla.org/r/127308/#review130042

> On desktop we don't show the options header for webextions options, so I think we should do the same here.

However, on desktop we also don't show an options header for non-webextensions..
Comment hidden (mozreview-request)
(Reporter)

Updated

2 years ago
Attachment #8855447 - Flags: review?(kmaglione+bmo) → review?(mixedpuppy)
(Reporter)

Comment 7

2 years ago
I will follow up with a test for this in a second commit.

Comment 8

2 years ago
mozreview-review
Comment on attachment 8855447 [details]
Bug 1302504 - use mozbrowser to add support for options_ui on Android

https://reviewboard.mozilla.org/r/127308/#review137004

::: mobile/android/chrome/content/aboutAddons.js:373
(Diff revision 2)
> +
> +  createWebExtensionOptions: async function(destination, optionsURL, browserStyle) {
> +    destination.innerHTML = "";
> +
> +    let frame = document.createElement("iframe");
> +    frame.setAttribute("id", "addon-options");

If its possible for more than one to be in dom, use a unique id.  not clear where the id is being used either.

::: mobile/android/chrome/content/aboutAddons.js:434
(Diff revision 2)
>            let id = aListItem.getAttribute("addonID");
>            Services.obs.notifyObservers(document, AddonManager.OPTIONS_NOTIFICATION_DISPLAYED, id);
>          }
>        }
>        xhr.send(null);
>      } catch (e) { }

pretty big catch here, is it necessary to wrap all this?  can we at least log the error?

::: toolkit/mozapps/extensions/content/extensions.js:3627
(Diff revision 2)
>  
>    createOptionsBrowser: Task.async(function*(parentNode) {
>      let browser = document.createElement("browser");
>      browser.setAttribute("type", "content");
>      browser.setAttribute("disableglobalhistory", "true");
> +    browser.setAttribute("id", "addon-options");

dito on the previous comment about the id for the options browser.
Attachment #8855447 - Flags: review?(mixedpuppy)
(Reporter)

Updated

2 years ago
Depends on: 1364945

Comment 9

2 years ago
Hi Andy, Matthew, 

I learned from Trello that this is the only remaining item for Web extension MVP in Firefox 56. Should I assume Matthew will be back on this and complete it soon so we can probably catch up the pre-beta test? (seems we'll miss the on-going mid-nightly test)

Thanks.
Flags: needinfo?(mwein)
Flags: needinfo?(amckay)
(Reporter)

Comment 10

2 years ago
Yes, this is next in my queue.
Flags: needinfo?(mwein)

Comment 11

2 years ago
Should there be some target for this bug? Currently it is not set.
(Reporter)

Comment 12

2 years ago
mozreview-review-reply
Comment on attachment 8855447 [details]
Bug 1302504 - use mozbrowser to add support for options_ui on Android

https://reviewboard.mozilla.org/r/127308/#review137004

> If its possible for more than one to be in dom, use a unique id.  not clear where the id is being used either.

There's only one ever in the DOM at a time.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 15

2 years ago
mozreview-review
Comment on attachment 8855447 [details]
Bug 1302504 - use mozbrowser to add support for options_ui on Android

https://reviewboard.mozilla.org/r/127308/#review160398
Attachment #8855447 - Flags: review?(mixedpuppy) → review+
(Reporter)

Updated

2 years ago
Keywords: qawanted

Comment 16

2 years ago
Pushed by mwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7331f829c36c
use mozbrowser to add support for options_ui on Android r=mixedpuppy
(Reporter)

Comment 17

2 years ago
Posted file test_addon
(Reporter)

Comment 18

2 years ago
Steps for testing:
1. Download the attached test_addon on android.
2. Visit about:addons and view the preferences for the addon.
3. Confirm that the extension preferences appear below the default preferences.
https://hg.mozilla.org/mozilla-central/rev/7331f829c36c
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56

Comment 20

2 years ago
In Android nightly, testing an unsigned addon, the message
'This addon could not be verified...'
obscures the prefs.
It should be above, or beneath, or something.
I'll post a screenshot when I can.

Updated

2 years ago
Flags: needinfo?(amckay)

Comment 21

2 years ago
Posted image webex_prefs.png
Screenshot - see comment 20

Comment 22

2 years ago
Posted image webex prefs iframe
The problem I described in comment 20 appears not to be caused by the 'unsigned' warning. In the debugger I can see that the options are truncated because they're in a small iframe. See screenshot.

The iframe is scrollable - so usable (just).

In the same addon on desktop the options occupy the full screen width.

Updated

2 years ago
Depends on: 1380575

Comment 23

2 years ago
This issue is verified as fixed on Firefox 56.0a1 (2017-07-13) under Android 6.0.1 and Android 7.1.2.

The extension preferences are displayed below the default preferences, please have a look at the attached file.

Updated

2 years ago
Status: RESOLVED → VERIFIED
status-firefox56: fixed → verified

Comment 24

2 years ago
This (unsigned) addon is the one giving the problems I described in comment 20, and the subsequent screenshots. 

The test addon from comment 17 works fine - but has too few options.

Comment 25

2 years ago
Posted image Joola.gif
Hello Dave Royal,

Here is a video with your WebExtension that has multiple options.

After install, the options are not displayed, only after you refresh the about:addons web page.

This is the issue you are talking about?
Flags: needinfo?(dave)

Comment 26

2 years ago
The issue is that the options are displayed in a tiny area of the screen on my Nexus 9 tablet - see my screenshots in comment 21 and comment 22. They should, IMO, occupy the entire width of the viewport as they do on desktop.

It's not even obvious the options are is scrollable - I discovered that by accident when using the debugger. 

The pre-webex version of the addon (it's in AMO) displays the options across the full width and they go off the bottom of the page. Why would the webex version be different?
Flags: needinfo?(dave)

Comment 27

2 years ago
Can this be reopened? See my screenshot in comment 21 and subsequent comments.
I don't think the implementation is usable on a tablet like my Nexus 9.
Flags: needinfo?(mwein)

Updated

2 years ago
Keywords: qawanted

Updated

2 years ago
Depends on: 1382572
(Reporter)

Comment 28

2 years ago
Hi Dave - yes, we definitely should reopen this to fix the UI for the devices it isn't working on. However, someone else will need to work on this.
Flags: needinfo?(matthewjwein)
(Reporter)

Updated

2 years ago
Assignee: matthewjwein → nobody
Reopening on behalf of mattw.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Blocks: 1386316
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Updated

2 years ago
Depends on: 1388098

Updated

2 years ago
Depends on: 1391002

Comment 30

2 years ago
Looks like just bug 1388098 remaining on this tracker.
Assignee: nobody → lgreco
webextensions: + → ---
Keywords: meta
(Assignee)

Updated

2 years ago
Depends on: 1395911
(Assignee)

Comment 31

2 years ago
I'm closing this as fixed (all the new dependencies issues filed from its reopening are now fixed).
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
Resolution: --- → FIXED

Updated

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