685334 - Menulist in Preferences needs Gingerbread styling

Status: RESOLVED WONTFIX

(Firefox for Android Graveyard :: General, defect)

Description

[Sriram Ramasubramanian [:sriram]]

The menulist in preferences uses Froyo theme. This needs to be updated for the Gingerbread theme.

Comment 1

[Sriram Ramasubramanian [:sriram]]

Attachment: WIP Patch

The patch removes buttons and fixes the menulist.

I would like to have comments on whether the CSS styling should be in platform.css or browser.css.

Are menulist being used anywhere else?

Comment 2

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

Comment on attachment 558980 [details] [diff] [review]
WIP Patch

* Keep the platform.css menulist styles. We use menulists in other places besides settings.
* Move the setting[type="menulist"] styles to theme/core/gingerbread/browser.css where we define styles for other setting[type=...]

Comment 3

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

Also, add a test for button and menulist toggle ?

Comment 4

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

The menulist styles under platform.css is preserved. The new styles have been moved to browser.css.

A patch for test file will be added separately.

Comment 5

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

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

>diff --git a/mobile/themes/core/gingerbread/browser.css b/mobile/themes/core/gingerbread/browser.css

>+setting[type="control"] > button {
>+  display: none;
>+}
>+setting[type="menulist"] {

Add a line break between rules

>+  -moz-box-align: stretch;
>+  -moz-binding: url("chrome://browser/content/bindings.xml#setting-fulltoggle-menulist") !important;
>+  position: relative;

Is the position:relative needed?

r+ but let's address the nits

Comment 6

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

The required changes have been made.

Comment 7

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

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

>diff --git a/mobile/chrome/content/bindings.xml b/mobile/chrome/content/bindings.xml

>+  <binding id="setting-fulltoggle-control" extends="chrome://mozapps/content/extensions/setting.xml#setting-control">

>+      <handler event="click" button="0" phase="capturing">

>+  <binding id="setting-fulltoggle-menulist" extends="chrome://mozapps/content/extensions/setting.xml#setting-multi">

>+      <handler event="click" button="0" phase="capturing">

Don't you need to switch these to use "TapSingle" like we did for the bool controls?

Also, we should move the <setting> bindings to a new chrome/content/bindings/setting.xml file (could be a followup bug)

>diff --git a/mobile/themes/core/gingerbread/images/dropmarker-hdpi.png b/mobile/themes/core/gingerbread/images/dropmarker-hdpi.png

This image seems big (filesize) for a drop marker. Can you use pngcrush to try to compact it a bit. Nothing to reduce the image quality, but sometimes pngcrush will remove left over comments and stuff in the image file.

Comment 8

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

The support for "TapSingle" has been added.
I am not sure if "oncommand" change that mbrubeck added for bool should go with this too.

I would be happy to move the settings to platform.css.

Comment 9

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

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

Looks OK to me. I would like to see chrome/content/bindings/setting.xml, and maybe a few others too, but that can be a followup bug.

Moving the setting CSS to platform.css can also be a follow up

Comment 10

[Matt Brubeck (:mbrubeck)]

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

>+  <binding id="setting-fulltoggle-control" extends="chrome://mozapps/content/extensions/setting.xml#setting-control">
>+    <handlers>
>+      <handler event="click" button="0" phase="capturing">
>+        <![CDATA[
>+        event.stopPropagation();
>+        ]]>
>+      </handler>
>+      <handler event="TapSingle" button="0" phase="capturing">
>+        <![CDATA[
>+        var button = this.childNodes[0];
>+        button.doCommand();
>+        event.stopPropagation();
>+        ]]>
>+      </handler>
>+    </handlers>
>+  </binding>

This breaks buttons like #prefs-uilanguage-button that use onclick instead of oncommand.  We could change that button as part of this patch, but add-on authors may also need to fix their add-ons.

>+setting[type="control"] > button {
>+  display: none;
>+}

I don't like this change because some settings (like Language) use the button text to show the current setting, and others (like the Sync "Connect/Disconnect" button) use it to show which action will happen on tap.

I'm not sure what the right solution is.  We could keep the buttons visible.  Or we could keep the text visible but style it differently so it doesn't look like a button.  Or we could remove the entire type="control" binding from this patch, and land only the type="menulist" changes.

Comment 11

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Attachment: build 20110926 screenshot on gingerbread

Menulist is partially whitewashed on gingerbread.  Adding screenshot to this bug.

Comment 12

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

Naoki - this is the wrong bug. This bug is about the preferences panel (preferences, addons downloads), not the action menu.

Comment 13

[Sriram Ramasubramanian [:sriram]]

Gone native!