696533 - Add-ons preferences

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P2)

Description

[[:fabrice] Fabrice Desré]

Can we show them inline in the add-on manager?

Comment 1

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

Yes, that is my plan in bug 696532

Comment 2

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

If an add-on has options we need to be able to show/hide them by tapping a button. If an add-on is disabled, for whatever reason, the button should be disabled too.

Comment 4

[Madhava Enros [:madhava]]

mfinkle - Aren't we handling these in the secondary "add-ons details" screen for each add-on (vs. displayed after pressing a button)?

Comment 5

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

(In reply to Madhava Enros [:madhava] from comment #4)
> mfinkle - Aren't we handling these in the secondary "add-ons details" screen
> for each add-on (vs. displayed after pressing a button)?

Yes, that is the current design. Comment 2 is from Nov 21, 2011.

Comment 6

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

Attachment: WIP

Adds basic support for add-on options:
* Ports <setting> and friends to HTML
* Styles the <setting>s in aboutAddons.css

To do:
* Needs to get the checkboxes a bit bigger
* Small tweaks the layout CSS

Comment 7

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

Attachment: screenshot

Screenshot of Zippity add-on options

Comment 8

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

Attachment: WIP 2

Just wanted to update with the latest WIP before handing off to Matt. This patch does show the options for an add-on:
* The checkbox rows need full-toggle support. The bindings still use "TapSingle" :(
* The "menulist" and "control" (with embedded "menulist") rows are busted because this is a XUL page and <menulist> is not supported. I tried looking for ways to add support for a <setting> that held <option>s and then have the XBL put those children in a <select> but it didn't work.

As I said, passing to Matt. I don't have the time to really work on this.

Comment 9

[Matt Brubeck (:mbrubeck)]

Attachment: WIP

Just a snapshot, building on Mark's previous patches.  I still need to do most of the theming work, and do something about menulists.

Comment 10

[Patryk Adamczyk [:patryk] UX]

Attachment: Attached are some layout adjustments, I believe it is difficult to get the exact coordinates so please match the layout as close as possible. Basically try to make everything align to something.

Comment 11

[Patryk Adamczyk [:patryk] UX]

Attachment: H DPI Checkboxes

Comment 12

[Geoff Lankow (:darktrojan)]

Comment on attachment 600567 [details] [diff] [review]
WIP

Review of attachment 600567 [details] [diff] [review]:
-----------------------------------------------------------------

Drive-by comments. I hate that we're going to end up with three implementations of the same thing to support, but there isn't much else we can do.

::: mobile/android/chrome/content/bindings/settings.xml
@@ +18,5 @@
> +      <html:div class="setting-label">
> +        <html:div class="preferences-title" xbl:inherits="xbl:text=title"/>
> +        <html:div class="preferences-description" xbl:inherits="xbl:text=desc">
> +          <children/>
> +        </html:div>

This <children> element (and the others like it) won't get used. The binding won't apply if the <setting> has text nodes, so we remove them and put the text content in the desc attribute (this doesn't actually happen on mobile yet, but I'm fixing it).

@@ +157,5 @@
> +          <children/>
> +        </html:div>
> +      </html:div>
> +      <html:div anonid="input-container" class="setting-input">
> +        <children includes="button|menulist"/>

That's not going to work without menulists.

@@ +211,5 @@
> +      </html:div>
> +      <html:div anonid="input-container" class="setting-input">
> +        <html:select size="1">
> +          <children includes="option" />
> +        </html:select>

For this to be compatible with our other implementation (including XUL mobile), you'd have to pick out the <menuitem>s from options.xul and replace them with <options>.

::: mobile/android/themes/core/aboutAddons.css
@@ +247,5 @@
> +
> +setting[type="bool"] {
> +  -moz-binding: url("chrome://browser/content/bindings/settings.xml#setting-fulltoggle-bool");
> +}
> +

To hide setting types we don't support, set display:none by default, and display:block on each of the types we do support. See bug 665515 and https://bugzilla.mozilla.org/attachment.cgi?id=600219&action=diff

Comment 13

[Geoff Lankow (:darktrojan)]

Oh, I also meant to say that most of the JS implementation in the new bindings can be inherited, so you don't need a copy here. I didn't actually compare it with base bindings, but it looks mostly the same.

Comment 14

[JP Rosevear [:jpr]]

With the dependency fixed now and the UI mockups done, is there anything blocking this one from moving forward (I know Matt could have been doing a bunch of FF 11 stuff).

Comment 15

[Matt Brubeck (:mbrubeck)]

(In reply to JP Rosevear [:jpr] from comment #14)
> With the dependency fixed now and the UI mockups done, is there anything
> blocking this one from moving forward (I know Matt could have been doing a
> bunch of FF 11 stuff).

No, nothing is blocking this.

Comment 16

[Matt Brubeck (:mbrubeck)]

Attachment: 1. XBL bindings and basic styles

This includes fully functional bindings and addresses darktrojan's feedback.  It actually removes most of the code from the earlier patches.

This patch has only very basic styles for layout; additional theming will be done in the next patch.

Comment 17

[Patryk Adamczyk [:patryk] UX]

Attachment: H DPI Checkboxes

Comment 18

[Matt Brubeck (:mbrubeck)]

Attachment: 2. Style tweaks and checkbox images

This adjusts the layout based on Patryk's mockup, and incorporates the checkbox images.

Still to do:  Better theming for integer prefs and menulists.

Comment 19

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

Comment on attachment 605586 [details] [diff] [review]
2. Style tweaks and checkbox images

Since we are using the toolkit bindings for settings, we are using XUL widgets in the HTML, right? Those widget are all functional? The checkbox stuff in this patch makes it a better touch target. What of the other widgets?

Comment 20

[Matt Brubeck (:mbrubeck)]

(In reply to Mark Finkle (:mfinkle) from comment #19)
> Since we are using the toolkit bindings for settings, we are using XUL
> widgets in the HTML, right? Those widget are all functional? The checkbox
> stuff in this patch makes it a better touch target. What of the other
> widgets?

Yeah, the widgets all work; bug 732265 added SelectHelper UI for menulists.

The "spinner" widgets for integer prefs are not really usable for mobile, and menulists need some theme love.  I'm working on both of those next.

Comment 21

[Matt Brubeck (:mbrubeck)]

We might want to add full-row touch support to other setting types, as we already do for checkboxes...

Comment 22

[Matt Brubeck (:mbrubeck)]

Pushed these to inbound; I'll file follow-up bugs for the remaining issues and nominate them blocking as appropriate.

https://hg.mozilla.org/integration/mozilla-inbound/rev/8313b23498ad
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a85d32044db

Comment 23

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/8313b23498ad
https://hg.mozilla.org/mozilla-central/rev/7a85d32044db

Comment 24

[Matt Brubeck (:mbrubeck)]

Comment on attachment 605470 [details] [diff] [review]
1. XBL bindings and basic styles

[Approval Request Comment]
Mobile-only beta blocker.

String changes made by this patch: none

Comment 25

[Alex Keybl [:akeybl]]

Comment on attachment 605470 [details] [diff] [review]
1. XBL bindings and basic styles

[Triage Comment]
Mobile only - approved for Aurora 13.

Comment 26

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/20692069e82f
https://hg.mozilla.org/releases/mozilla-aurora/rev/3d1a7b3938d0

Comment 27

[Cristian Nicolae (:xti)]

Verified fixed on:

Firefox 15.0a1 (2012-04-30)
Firefox 14.0a2 (2012-04-30)

Device: Samsung Captivate
OS: Android 2.2