784395 - Make "Request Desktop Site" checkbox clearer

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Theme and Visual Design, defect)

Description

[Ian Barlow (:ibarlow)]

Attachment: mockup

Our "Request Desktop Site" can be toggled on and off, but we don't make that clear enough to our users -- for one thing, we should be indicating that this is a toggle before users even touch it. We don't show an empty checkbox next to the menu item right now, so it isn't really clear that it works like an on/off switch. 

Attached is a design that adds an "off" state to the checkbox. 

--

Note: This should also affect the "Bookmark" menu item.

Comment 1

[Sriram Ramasubramanian [:sriram]]

Should this be added for "Bookmark" too?

Comment 2

[Ian Barlow (:ibarlow)]

Yessir.

Comment 3

[Ian Barlow (:ibarlow)]

Attachment: graphic assets for checkboxes - mdpi/hdpi/xhdpi

Comment 4

[Sriram Ramasubramanian [:sriram]]

Attachment: WIP

The images are bigger : 24x24. We've been using something like 20x18 so far. It's better to use a smaller one (or increase the width of the menu).

Also, this uses ImageView instead of changing to CheckBox, as the CheckBox tends to receive focus -- which means the check changes and entire row doesnt get touch event and things get worse.

A final patch will be ready with smaller images.

Comment 5

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

This patch changes the checkbox's size, adds an unchecked state, and fits it smaller for our custom menu.
Additionally there was a bug with checkable/checked state code, which is fixed now.

Comment 6

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

Comment on attachment 662703 [details] [diff] [review]
Patch

>diff --git a/mobile/android/base/GeckoMenuInflater.java b/mobile/android/base/GeckoMenuInflater.java

>     public void setValues(ParsedItem item, MenuItem menuItem) {
>         menuItem.setChecked(item.checked)
>                 .setVisible(item.visible)
>                 .setEnabled(item.enabled)
>                 .setCheckable(item.checkable)
>-                .setCheckable(item.checked)
>+                .setChecked(item.checked)
>                 .setIcon(item.iconRes)
>                 .setShowAsAction(item.showAsAction);
>     }

setChecked is called twice in this code. I assume that setCheckable must be called first. If so, remove the first call to setChecked

r+, but fix this

Comment 7

[Sriram Ramasubramanian [:sriram]]

The extra setCheckable() has been removed.
https://hg.mozilla.org/integration/mozilla-inbound/rev/4023d860c363

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4023d860c363

Comment 9

[Andreea Pod]

Verified fixed:

Build: Firefox 18.0a1 (2012-09-20), Firefox 17.0a2 (2012-09-20)
Device: Samsung Galaxy Nexus
OS: Android 4.1.1

Comment 10

[Andreea Pod]

Correction:

Build: Firefox 18.0a1 (2012-09-21)