Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Add support for options_ui on Android

VERIFIED FIXED in Firefox 56

Status

()

Toolkit
WebExtensions: Android
P2
normal
VERIFIED FIXED
10 months ago
2 days ago

People

(Reporter: mattw, Assigned: mattw, NeedInfo)

Tracking

(Depends on: 3 bugs, Blocks: 2 bugs)

Trunk
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 affected, firefox56 verified)

Details

(Whiteboard: triaged)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(7 attachments)

Comment hidden (empty)

Updated

10 months ago
Blocks: 467520

Updated

10 months ago
Whiteboard: triaged

Updated

9 months ago
Blocks: 1226547
No longer blocks: 467520

Updated

9 months ago
Duplicate of this bug: 1292347

Updated

7 months ago
webextensions: --- → +

Updated

7 months ago
Assignee: nobody → mwein

Updated

5 months ago
Blocks: 1185785
Comment hidden (mozreview-request)
(Assignee)

Updated

4 months ago
Attachment #8855447 - Flags: feedback?(kmaglione+bmo)

Comment 3

4 months 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>.

Updated

4 months ago
Attachment #8855447 - Flags: feedback?(kmaglione+bmo) → feedback+
(Assignee)

Comment 4

3 months 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.
(Assignee)

Comment 5

3 months 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)
(Assignee)

Updated

3 months ago
Attachment #8855447 - Flags: review?(kmaglione+bmo) → review?(mixedpuppy)
(Assignee)

Comment 7

3 months ago
I will follow up with a test for this in a second commit.

Comment 8

3 months 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)
(Assignee)

Updated

2 months ago
Depends on: 1364945
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)
(Assignee)

Comment 10

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

Comment 11

15 days ago
Should there be some target for this bug? Currently it is not set.
(Assignee)

Comment 12

15 days 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

15 days 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+
(Assignee)

Updated

15 days ago
Keywords: qawanted

Comment 16

15 days 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
(Assignee)

Comment 17

15 days ago
Created attachment 8884482 [details]
test_addon
(Assignee)

Comment 18

15 days 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: 13 days ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56

Comment 20

11 days 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

11 days ago
Flags: needinfo?(amckay)

Comment 21

11 days ago
Created attachment 8885455 [details]
webex_prefs.png

Screenshot - see comment 20

Comment 22

10 days ago
Created attachment 8885752 [details]
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

9 days ago
Depends on: 1380575

Comment 23

9 days ago
Created attachment 8886125 [details]
options_uiAndroid.mp4

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

9 days ago
Status: RESOLVED → VERIFIED
status-firefox56: fixed → verified

Comment 24

9 days ago
Created attachment 8886155 [details]
Addon which demonstrates options display problem

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

9 days ago
Created attachment 8886219 [details]
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

9 days 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

6 days 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

5 days ago
Keywords: qawanted

Updated

2 days ago
Depends on: 1382572
You need to log in before you can comment on or make changes to this bug.