Closed Bug 512882 Opened 10 years ago Closed 10 years ago

Create new option scheme for add-ons

Categories

(Firefox for Android Graveyard :: General, defect)

x86
Linux
defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: mfinkle, Assigned: vingtetun)

References

Details

Attachments

(2 files, 3 obsolete files)

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.
Attached patch Patch (obsolete) — Splinter Review
I've add a 'richtitle' binding to allow extensions's developers to separate their sections (as we do).
Attachment #397014 - Flags: review?(mark.finkle)
Attachment #397015 - Attachment is patch: false
Attachment #397015 - Attachment mime type: text/plain → image/png
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" ?
Attached patch Patch v0.2 (obsolete) — Splinter Review
* 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)
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-
Attached patch Patch v0.3Splinter Review
(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)
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
Closed: 10 years ago
Resolution: --- → FIXED
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.