Closed Bug 685334 Opened 11 years ago Closed 10 years ago

Menulist in Preferences needs Gingerbread styling

Categories

(Firefox for Android Graveyard :: General, defect)

ARM
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: sriram, Assigned: sriram)

References

Details

Attachments

(2 files, 3 obsolete files)

The menulist in preferences uses Froyo theme. This needs to be updated for the Gingerbread theme.
OS: Mac OS X → Android
Hardware: x86 → ARM
Attached patch WIP Patch (obsolete) — Splinter Review
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
Attached patch Patch (obsolete) — Splinter Review
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+
Attached patch Patch (obsolete) — Splinter Review
The required changes have been made.
Attachment #559324 - Attachment is obsolete: true
Attachment #562147 - Flags: review?(mark.finkle)
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-
Attached patch PatchSplinter Review
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-
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
Gone native!
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.