Closed
Bug 1302504
Opened 8 years ago
Closed 7 years ago
Add support for options_ui on Android
Categories
(WebExtensions :: Android, defect, P2)
WebExtensions
Android
Tracking
(firefox51 affected, firefox56 verified)
RESOLVED
FIXED
mozilla56
People
(Reporter: mattw, Assigned: rpl)
References
Details
(Keywords: meta, Whiteboard: triaged)
Attachments
(7 files)
No description provided.
Updated•8 years ago
|
Whiteboard: triaged
Updated•8 years ago
|
Updated•7 years ago
|
webextensions: --- → +
Updated•7 years ago
|
Assignee: nobody → mwein
Updated•7 years ago
|
Blocks: webext-android
Comment hidden (mozreview-request) |
Reporter | ||
Updated•7 years ago
|
Attachment #8855447 -
Flags: feedback?(kmaglione+bmo)
Comment 3•7 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>.
Updated•7 years ago
|
Attachment #8855447 -
Flags: feedback?(kmaglione+bmo) → feedback+
Reporter | ||
Comment 4•7 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•7 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•7 years ago
|
Attachment #8855447 -
Flags: review?(kmaglione+bmo) → review?(mixedpuppy)
Reporter | ||
Comment 7•7 years ago
|
||
I will follow up with a test for this in a second commit.
Comment 8•7 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)
Comment 9•7 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)
Comment 11•7 years ago
|
||
Should there be some target for this bug? Currently it is not set.
Reporter | ||
Comment 12•7 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•7 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+
Comment 16•7 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•7 years ago
|
||
Reporter | ||
Comment 18•7 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.
Comment 19•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/7331f829c36c
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Comment 20•7 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•7 years ago
|
Flags: needinfo?(amckay)
Comment 21•7 years ago
|
||
Screenshot - see comment 20
Comment 22•7 years ago
|
||
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.
Comment 23•7 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.
Status: RESOLVED → VERIFIED
Comment 24•7 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•7 years ago
|
||
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•7 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•7 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)
Reporter | ||
Comment 28•7 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•7 years ago
|
Assignee: matthewjwein → nobody
Comment 29•7 years ago
|
||
Reopening on behalf of mattw.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Updated•7 years ago
|
Updated•7 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 30•7 years ago
|
||
Looks like just bug 1388098 remaining on this tracker.
Assignee | ||
Comment 31•7 years ago
|
||
I'm closing this as fixed (all the new dependencies issues filed from its reopening are now fixed).
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
Resolution: --- → FIXED
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•