Closed Bug 1302504 Opened 8 years ago Closed 7 years ago

Add support for options_ui on Android

Categories

(WebExtensions :: Android, defect, P2)

defect

Tracking

(firefox51 affected, firefox56 verified)

RESOLVED FIXED
mozilla56
Tracking Status
firefox51 --- affected
firefox56 --- verified

People

(Reporter: mattw, Assigned: rpl)

References

Details

(Keywords: meta, Whiteboard: triaged)

Attachments

(7 files)

      No description provided.
Blocks: abp
Whiteboard: triaged
Blocks: webext-port-abp
No longer blocks: abp
webextensions: --- → +
Assignee: nobody → mwein
Attachment #8855447 - Flags: feedback?(kmaglione+bmo)
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+
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.
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..
Attachment #8855447 - Flags: review?(kmaglione+bmo) → review?(mixedpuppy)
I will follow up with a test for this in a second commit.
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)
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)
Yes, this is next in my queue.
Flags: needinfo?(mwein)
Should there be some target for this bug? Currently it is not set.
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 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+
Keywords: qawanted
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
Attached file test_addon
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
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
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.
Flags: needinfo?(amckay)
Attached image webex_prefs.png
Screenshot - see comment 20
Attached 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.
Depends on: 1380575
Attached video 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.
Status: RESOLVED → VERIFIED
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.
Attached 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)
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)
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)
Keywords: qawanted
Depends on: 1382572
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)
Assignee: matthewjwein → nobody
Reopening on behalf of mattw.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Blocks: 1386316
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Depends on: 1388098
Depends on: 1391002
Looks like just bug 1388098 remaining on this tracker.
Assignee: nobody → lgreco
webextensions: + → ---
Keywords: meta
Depends on: 1395911
I'm closing this as fixed (all the new dependencies issues filed from its reopening are now fixed).
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: