Menulist in Preferences needs Gingerbread styling

RESOLVED WONTFIX

Status

Firefox for Android Graveyard
General
RESOLVED WONTFIX
7 years ago
6 years ago

People

(Reporter: sriram, Assigned: sriram)

Tracking

Trunk
ARM
Android
Dependency tree / graph

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

7 years ago
The menulist in preferences uses Froyo theme. This needs to be updated for the Gingerbread theme.
(Assignee)

Updated

7 years ago
OS: Mac OS X → Android
Hardware: x86 → ARM
(Assignee)

Comment 1

7 years ago
Created attachment 558980 [details] [diff] [review]
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?
Attachment #558980 - Flags: review?(mark.finkle)
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=...]
Attachment #558980 - Flags: review?(mark.finkle) → review-
Also, add a test for button and menulist toggle ?
Assignee: nobody → sriram
Blocks: 653134
Status: NEW → ASSIGNED
(Assignee)

Comment 4

7 years ago
Created attachment 559324 [details] [diff] [review]
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.
Attachment #558980 - Attachment is obsolete: true
Attachment #559324 - Flags: review?(mark.finkle)
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
Attachment #559324 - Flags: review?(mark.finkle) → review+
(Assignee)

Comment 6

7 years ago
Created attachment 562147 [details] [diff] [review]
Patch

The required changes have been made.
Attachment #559324 - Attachment is obsolete: true
Attachment #562147 - Flags: review?(mark.finkle)
(Assignee)

Updated

7 years ago
Blocks: 682412
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.
Attachment #562147 - Flags: review?(mark.finkle) → review-
(Assignee)

Comment 8

7 years ago
Created attachment 562195 [details] [diff] [review]
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.
Attachment #562147 - Attachment is obsolete: true
Attachment #562195 - Flags: review?(mbrubeck)
Attachment #562195 - Flags: review?(mark.finkle)
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
Attachment #562195 - Flags: review?(mark.finkle) → review+
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.
Attachment #562195 - Flags: review?(mbrubeck) → review-
Created attachment 562616 [details]
build 20110926 screenshot on gingerbread

Menulist is partially whitewashed on gingerbread.  Adding screenshot to this bug.
Naoki - this is the wrong bug. This bug is about the preferences panel (preferences, addons downloads), not the action menu.
Blocks: 693049
(Assignee)

Comment 13

6 years ago
Gone native!
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.