Last Comment Bug 696533 - Add-ons preferences
: Add-ons preferences
Status: VERIFIED FIXED
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: ARM Android
: P2 normal (vote)
: Firefox 14
Assigned To: Matt Brubeck (:mbrubeck)
:
:
Mentors:
: 717065 (view as bug list)
Depends on: 762015 732063 732265 735874
Blocks: abp 696518 aom 737692
  Show dependency treegraph
 
Reported: 2011-10-21 17:21 PDT by [:fabrice] Fabrice Desré
Modified: 2016-07-29 14:20 PDT (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
verified
verified
beta+
11+


Attachments
WIP (13.75 KB, patch)
2012-01-17 23:23 PST, Mark Finkle (:mfinkle) (use needinfo?)
no flags Details | Diff | Splinter Review
screenshot (86.11 KB, image/png)
2012-01-17 23:23 PST, Mark Finkle (:mfinkle) (use needinfo?)
no flags Details
WIP 2 (18.22 KB, patch)
2012-02-02 19:25 PST, Mark Finkle (:mfinkle) (use needinfo?)
no flags Details | Diff | Splinter Review
WIP (20.73 KB, patch)
2012-02-24 16:48 PST, Matt Brubeck (:mbrubeck)
no flags Details | Diff | Splinter Review
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. (756.42 KB, image/png)
2012-03-01 08:40 PST, Patryk Adamczyk [:patryk] UX
no flags Details
H DPI Checkboxes (67.48 KB, application/zip)
2012-03-01 10:30 PST, Patryk Adamczyk [:patryk] UX
no flags Details
1. XBL bindings and basic styles (12.46 KB, patch)
2012-03-13 11:56 PDT, Matt Brubeck (:mbrubeck)
mark.finkle: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
H DPI Checkboxes (19.93 KB, application/zip)
2012-03-13 15:35 PDT, Patryk Adamczyk [:patryk] UX
no flags Details
2. Style tweaks and checkbox images (33.17 KB, patch)
2012-03-13 16:17 PDT, Matt Brubeck (:mbrubeck)
mark.finkle: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description [:fabrice] Fabrice Desré 2011-10-21 17:21:38 PDT
Can we show them inline in the add-on manager?
Comment 1 Mark Finkle (:mfinkle) (use needinfo?) 2011-10-22 13:18:10 PDT
Yes, that is my plan in bug 696532
Comment 2 Mark Finkle (:mfinkle) (use needinfo?) 2011-11-21 21:43:57 PST
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 3 Mark Finkle (:mfinkle) (use needinfo?) 2012-01-12 07:31:29 PST
*** Bug 717065 has been marked as a duplicate of this bug. ***
Comment 4 Madhava Enros [:madhava] 2012-01-13 10:33:57 PST
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?) 2012-01-13 10:35:52 PST
(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?) 2012-01-17 23:23:03 PST
Created attachment 589413 [details] [diff] [review]
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?) 2012-01-17 23:23:46 PST
Created attachment 589414 [details]
screenshot

Screenshot of Zippity add-on options
Comment 8 Mark Finkle (:mfinkle) (use needinfo?) 2012-02-02 19:25:28 PST
Created attachment 594056 [details] [diff] [review]
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) 2012-02-24 16:48:41 PST
Created attachment 600567 [details] [diff] [review]
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 2012-03-01 08:40:57 PST
Created attachment 601991 [details]
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 2012-03-01 10:30:14 PST
Created attachment 602014 [details]
H DPI Checkboxes
Comment 12 Geoff Lankow (:darktrojan) 2012-03-01 14:13:31 PST
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) 2012-03-01 14:21:16 PST
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] 2012-03-10 12:56:29 PST
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) 2012-03-13 10:16:31 PDT
(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) 2012-03-13 11:56:32 PDT
Created attachment 605470 [details] [diff] [review]
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 2012-03-13 15:35:12 PDT
Created attachment 605568 [details]
H DPI Checkboxes
Comment 18 Matt Brubeck (:mbrubeck) 2012-03-13 16:17:31 PDT
Created attachment 605586 [details] [diff] [review]
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?) 2012-03-13 16:47:48 PDT
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) 2012-03-13 16:56:21 PDT
(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) 2012-03-13 17:02:24 PDT
We might want to add full-row touch support to other setting types, as we already do for checkboxes...
Comment 22 Matt Brubeck (:mbrubeck) 2012-03-13 22:20:27 PDT
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 24 Matt Brubeck (:mbrubeck) 2012-03-19 12:07:18 PDT
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] 2012-03-20 13:03:50 PDT
Comment on attachment 605470 [details] [diff] [review]
1. XBL bindings and basic styles

[Triage Comment]
Mobile only - approved for Aurora 13.
Comment 27 Cristian Nicolae (:xti) 2012-04-30 09:27:07 PDT
Verified fixed on:

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

Device: Samsung Captivate
OS: Android 2.2

Note You need to log in before you can comment on or make changes to this bug.