Closed Bug 485381 Opened 12 years ago Closed 12 years ago

Add special widget for checkboxes

Categories

(Firefox for Android Graveyard :: General, defect)

x86
Windows XP
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mfinkle, Assigned: mfinkle)

Details

Attachments

(1 file, 1 obsolete file)

Attached patch patch (obsolete) — Splinter Review
Fennec uses a different visual presentation for checkbox controls. For an example see: http://people.mozilla.com/~mfinkle/checkbox-radio.png

Currently, the widget "hijacks" <checkbox> tags, and replaces the normal checkbox with a different XBL widget, which has the same API. The widget is built on <radiogroup>/<radio> elements.

We should consider whether replacing <checkbox> is the best way, or if we want to introduce a new tag - <toggle>, for example.

This patch also changes the "selected" color of re-themed <radio> elements. The background is now inherited and the selected item is gray. This matches Madhava's wireframes.
Attachment #369534 - Flags: review?(gavin.sharp)
Wouldn't it be simpler to extend the checkbox binding, and just override the content, setChecked, and the constructor?
Attached patch patch 2Splinter Review
This patch extends the checkbox binding so we get the default handlers. I also added a DTD for the checkbox strings.
Assignee: nobody → mark.finkle
Attachment #369534 - Attachment is obsolete: true
Attachment #369661 - Flags: review?(gavin.sharp)
Attachment #369534 - Flags: review?(gavin.sharp)
Comment on attachment 369661 [details] [diff] [review]
patch 2

>diff --git a/chrome/content/checkbox.xml b/chrome/content/checkbox.xml

>+<bindings

>+    xmlns:xbl="http://www.mozilla.org/xbl"

Doesn't appear to be used?

>+    <implementation>
>+      <constructor><![CDATA[
>+        let checked = this.getAttribute("checked");
>+        this.setChecked(checked == "true");

Use the checked getter?

>+      <method name="setChecked">
>+        <parameter name="aValue"/>
>+        <body>
>+        <![CDATA[
>+          var change = (aValue != (this.getAttribute("checked") == "true"));
>+          if (aValue) {
>+            this.setAttribute("checked", "true");
>+            this._group.selectedItem = this._on;
>+          }
>+          else {
>+            this.removeAttribute("checked");
>+            this._group.selectedItem = this._off;
>+          }

This is just copied from the original checkbox, but it could use the checked getter/setter as well.

>diff --git a/themes/hildon/platform.css b/themes/hildon/platform.css

>+checkbox {
>+  -moz-binding: url("chrome://browser/content/checkbox.xml#checkbox-radio");
>+}

I think this should be in /browser/base/content/browser.css .

Hate to keep harping about foreground/background colors, but you should probably also specify an explicit foreground color for :checked?

Also maybe get a followup for onlabel/offlabel? Not sure that on/off will work in all cases.
Attachment #369661 - Flags: review?(gavin.sharp) → review+
(In reply to comment #3)
> (From update of attachment 369661 [details] [diff] [review])
> >diff --git a/chrome/content/checkbox.xml b/chrome/content/checkbox.xml
> 
> >+<bindings
> 
> >+    xmlns:xbl="http://www.mozilla.org/xbl"
> 
> Doesn't appear to be used?

Gone

> 
> >+    <implementation>
> >+      <constructor><![CDATA[
> >+        let checked = this.getAttribute("checked");
> >+        this.setChecked(checked == "true");
> 
> Use the checked getter?

Done

> <snipped>
> This is just copied from the original checkbox, but it could use the checked
> getter/setter as well.


Used getter. Can't use setter.
 
> >diff --git a/themes/hildon/platform.css b/themes/hildon/platform.css
> 
> >+checkbox {
> >+  -moz-binding: url("chrome://browser/content/checkbox.xml#checkbox-radio");
> >+}
> 
> I think this should be in /browser/base/content/browser.css .

Done

> 
> Hate to keep harping about foreground/background colors, but you should
> probably also specify an explicit foreground color for :checked?

Done.

> 
> Also maybe get a followup for onlabel/offlabel? Not sure that on/off will work
> in all cases.

Bug 485607
http://hg.mozilla.org/mobile-browser/rev/1e0c59607a04
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Hi, just a few notes from a localizer point of view:
1) not all locales have short words. For example, in Lithuanian I now either have to use "Įjungta/Išjungta", or to abbreviate those words to "Įj./Išj.". That doesn't play nice either way.
2) from the image in Comment #0 it's hard to tell which checkboxes are on, and which are off. I fail to see how this is any better than just a classic checkbox (unless such widgets are used in the environment which Fennec targets).
You need to log in before you can comment on or make changes to this bug.