Create new option scheme for add-ons

VERIFIED FIXED

Status

Fennec Graveyard
General
VERIFIED FIXED
8 years ago
4 years ago

People

(Reporter: mfinkle, Assigned: vingtetun)

Tracking

Details

Attachments

(2 attachments, 3 obsolete attachments)

Fennec currently has an <iframe> that is used to display the <optionsURL> of an add-on. This fails for so many reasons:
* Sizing failure1: the option XUL is very like not to fit inside the iframe
* Sizing failure2: the iframe has a hard time resiing itself to fit the content, even if it is small enough to see on the screen.
* The UI look and feel for options in Fennec is broken by using random XUL

Therefore we need a new way to display options for an addon:
* No iframe. We will add the contents directly to the add-ons manager.
* <richpref> widgets only. We want the add-on options to look like other Fennec options.
* Less is more. We want to encourage add-on developers to re-think the options exposed by their add-ons when developing for Fennec.

The details:
* Add-ons will override, via chrome manifest, the optionsURL they use for Firefox when installing into Fennec
* Fennec will expect a valid snippet of XUL, built using <richpref>, in the optionsURL file
* Fennec will use XHR to load the XUL and append it to the add-on manager row item, inside an <xul:vbox>.

The result should be a list of options that look like Fennec options and are still pannable in the add-ons list. The nsPromptService patch uses XHR to load "dialogs" the same way I am suggesting we load the options list.
Assignee: nobody → 21
Created attachment 397014 [details] [diff] [review]
Patch

I've add a 'richtitle' binding to allow extensions's developers to separate their sections (as we do).
Attachment #397014 - Flags: review?(mark.finkle)
Created attachment 397015 [details]
screenshot
Attachment #397015 - Attachment is patch: false
Attachment #397015 - Attachment mime type: text/plain → image/png
Blocks: 491601
Comment on attachment 397014 [details] [diff] [review]
Patch

Looks like you can remove _optionsHandler altogether. The only part of it you still use is .box and you can just make that a field on the binding itself.

Also, let's get Madhava to see if he wants to somehow visually show that the listed options are "part" of the add-on. Right now you really can't tell that the options are associated with the add-on.

Great job!
Attachment #397014 - Flags: ui-review?(madhava)
Attachment #397014 - Flags: review?(mark.finkle)
Attachment #397014 - Flags: review-
Oh, now I really want to change "richpref" -> "setting" :)
(In reply to comment #3)
> Also, let's get Madhava to see if he wants to somehow visually show that the
> listed options are "part" of the add-on. Right now you really can't tell that
> the options are associated with the add-on.

yes, we have discussed about that with Madhava yesterday on IRC (not sure that only read Madhava's thoughts and say yes to all his suggestions is really a discussion :))

(In reply to comment #4)
> Oh, now I really want to change "richpref" -> "setting" :)

Any idea for the richtitle name? maybe "settingstitle" ?
Created attachment 397261 [details] [diff] [review]
Patch v0.2

* rename richpref to setting
* rename richtitle to settingstitle
* Add a margin before the "options" button and the prefs

hmm, did 'hg rename' keep the history? it seems strange in the patch...
Attachment #397014 - Attachment is obsolete: true
Attachment #397261 - Flags: ui-review?
Attachment #397261 - Flags: review?(mark.finkle)
Attachment #397014 - Flags: ui-review?(madhava)
Attachment #397261 - Flags: ui-review? → ui-review?(madhava)
Created attachment 397262 [details]
screenshot
Attachment #397015 - Attachment is obsolete: true
Comment on attachment 397261 [details] [diff] [review]
Patch v0.2

Overall, this looks good.I want to test it a bit more before final review.

Also, I like the padding, except for the "Options" button itself. Can we keep it without padding, so we maximize space for buttons in portrait and we are consistent with the buttons in the "Get Add-ons" sections.

Lastly, we need to inform existing add-on developers, weave for example, that we are renaming the richpref bindings.

Oh, really last thing, "settingstitle": I'm wondering if that should be "settings" and act as a container for "setting" elements. More on that after I think about it more.
(In reply to comment #8)
> (From update of attachment 397261 [details] [diff] [review])
> Also, I like the padding, except for the "Options" button itself. Can we keep
> it without padding, so we maximize space for buttons in portrait and we are
> consistent with the buttons in the "Get Add-ons" sections.

Madhava?

> 
> Lastly, we need to inform existing add-on developers, weave for example, that
> we are renaming the richpref bindings.

What's the usual method for that? Should i add a needdocumention flag to the bug?

> 
> Oh, really last thing, "settingstitle": I'm wondering if that should be
> "settings" and act as a container for "setting" elements. More on that after I
> think about it more.

I find <settings><children/></settings> a good idea.
Oh, this should be blocking
tracking-fennec: --- → ?
Comment on attachment 397261 [details] [diff] [review]
Patch v0.2

* Let's do the settingstitle -> settings change and make it a container for setting elements
* Do not pad the "Options" button
* Use millimeters for the left padding, not "32px" (2.2mm on hildon and 1.1mm on wince)
* Make the "Options" button stay pressed while the options list is visible
Attachment #397261 - Flags: ui-review?(madhava)
Attachment #397261 - Flags: review?(mark.finkle)
Attachment #397261 - Flags: review-
Created attachment 398397 [details] [diff] [review]
Patch v0.3

(In reply to comment #11)
> (From update of attachment 397261 [details] [diff] [review])
> * Let's do the settingstitle -> settings change and make it a container for
> setting elements
> * Do not pad the "Options" button
> * Use millimeters for the left padding, not "32px" (2.2mm on hildon and 1.1mm
> on wince)
> * Make the "Options" button stay pressed while the options list is visible

Adress comments except the pressed state of the "Options button" which is more a css related problem. (there is no class for that :( )
Attachment #397261 - Attachment is obsolete: true
Attachment #398397 - Flags: review?(mark.finkle)
(Reporter)

Updated

8 years ago
Attachment #398397 - Flags: review?(mark.finkle) → review+
pushed with some tweaks after talking to madhava:
https://hg.mozilla.org/mobile-browser/rev/13167a569cae
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED

Comment 14

8 years ago
The code at https://hg.mozilla.org/mobile-browser/rev/13167a569cae#l1.90 will fail if the xhr.responseDocument is null. Should we fill a new bug on this ?
(In reply to comment #14)
> The code at https://hg.mozilla.org/mobile-browser/rev/13167a569cae#l1.90 will
> fail if the xhr.responseDocument is null. Should we fill a new bug on this ?

Yes, please. Good catch.
addons seem to be working solid.
verified on my n810 with 20090921 1.9.2 nightly build
Status: RESOLVED → VERIFIED
The override option described at:

http://starkravingfinkle.org/blog/2009/09/fennec-handling-add-on-options/

... is quite straightforward. 

However, what if there is no options url to override? So, the mobile add-on is sharing the same code-base as the desktop add-on, and former has options but the latter does not, i.e. no em:optionsUrl.

Know any good hacks to keep the button disabled on desktop while enabling it on mobile?
tracking-fennec: ? → ---
You need to log in before you can comment on or make changes to this bug.