512882 - Create new option scheme for add-ons

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Mark Finkle (:mfinkle) (use needinfo?)]

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.

Comment 1

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch

I've add a 'richtitle' binding to allow extensions's developers to separate their sections (as we do).

Comment 2

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: screenshot

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

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!

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

Oh, now I really want to change "richpref" -> "setting" :)

Comment 5

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(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" ?

Comment 6

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: 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...

Comment 7

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: screenshot

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

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.

Comment 9

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(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.

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

Oh, this should be blocking

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

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

Comment 12

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: 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 :( )

Comment 13

[Mark Finkle (:mfinkle) (use needinfo?)]

pushed with some tweaks after talking to madhava:
https://hg.mozilla.org/mobile-browser/rev/13167a569cae

Comment 14

[Fabrice Desré]

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 ?

Comment 15

[Mark Finkle (:mfinkle) (use needinfo?)]

(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.

Comment 16

[Joel Maher ( :jmaher ) (UTC -8)]

addons seem to be working solid.
verified on my n810 with 20090921 1.9.2 nightly build

Comment 17

[Brian King [:kinger]]

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?