Closed Bug 562849 Opened 13 years ago Closed 13 years ago

Reverse meanings of N900 volume buttons in portrait mode

Categories

(Firefox for Android Graveyard :: General, defect)

ARM
Maemo
defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: mbrubeck, Assigned: mbrubeck)

Details

Attachments

(1 file, 1 obsolete file)

Attached patch patch (obsolete) — Splinter Review
On the N900, the "volume down" button in landscape mode becomes the "volume up" button in portrait mode (since it is now on the top).

This patch switches the volume rocker zoom controls depending on the device orientation.
Comment on attachment 442590 [details] [diff] [review]
patch

>diff -r ce48d028c5ab chrome/content/browser-ui.js

>+  isPortrait: function isPortrait() {
>+    return (window.innerWidth < 500);
>+  },

We have an actual flag for this on the N900:

http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsNativeAppSupportUnix.cpp#257

You can query it from JS like this:
http://mxr.mozilla.org/mobile-browser/source/chrome/content/BrowserView.js#249

Just check for "screen-orientation", instead of device. I wonder if Android will support the system-info "screen-orientation" though.

>+      case "cmd_volumeLeft":
>+      case "cmd_volumeRight":

Why add cmd_volumeLeft and Right? I assume you want to keep cmd_zoomin & out fixed, regardless of orientation?


>diff -r ce48d028c5ab chrome/content/browser.xul

> #ifndef WINCE
>     <key id="key_quit" key="q" modifiers="accel" command="cmd_quit"/>
>     <key id="key_fullscreen" keycode="VK_F6" command="cmd_fullscreen"/>
>     <key id="key_zoomin2" keycode="VK_F7" command="cmd_zoomin"/>
>     <key id="key_zoomout2" keycode="VK_F8" command="cmd_zoomout"/>
> #else
>     <key id="key_quit" keycode="VK_F4" command="cmd_quit"/>
>-    <key id="key_zoomin2" keycode="VK_F1" command="cmd_zoomin"/>
>-    <key id="key_zoomout2" keycode="VK_F2" command="cmd_zoomout"/>
>+    <key id="key_zoomin2" keycode="VK_F1" command="cmd_volumeRight"/>
>+    <key id="key_zoomout2" keycode="VK_F2" command="cmd_volumeLeft"/>
> #endif

Your in the wrong side of the #ifndef. You want to change the top block (F7 and F8)
Attachment #442590 - Flags: review-
Until we know how Android reacts, including whether we want to use the volume keys for zooming on Android, I guess using the window.innerWidth for a "portrait" check is OK.
Attached patch patch v2Splinter Review
Updated patch, tested on N900 this time - there was a reason I hadn't asked for a review yet!  :)

This patch still uses window.innerWidth as a proxy for orientation.  If we make this Maemo-only then I will switch to the Maemo-specific call.

> Why add cmd_volumeLeft and Right? I assume you want
> to keep cmd_zoomin & out fixed, regardless of orientation?

Yeah, exactly.  I didn't like the idea of a "zoomin" command that sometimes zooms out - especially if we later add other ways to activate the command (like menu items or buttons).
Attachment #442590 - Attachment is obsolete: true
Attachment #442778 - Flags: review?(mark.finkle)
Comment on attachment 442778 [details] [diff] [review]
patch v2

nit: I think we should move "isPortrait" to Util.isPortrait

I can do that on checkin.
Attachment #442778 - Flags: review?(mark.finkle) → review+
Yeah - we should do this.

On Android, at least with multitouch support, we shouldn't do volume key
zooming.
pushed m-b:
http://hg.mozilla.org/mobile-browser/rev/50342678d8eb

pushed m-1.1:
http://hg.mozilla.org/releases/mobile-1.1/rev/bb635d81fdaf
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
verified FIXED on builds:

Mozilla/5.0 (X11; U; Linux armv7l; Nokia N900; en-US; rv:1.9.2.5pre) Gecko/20100503 Namoroka/3.6.5pre Fennec/1.1b2pre

and

Mozilla/5.0 (X11; U; Linux armv71; Nokia N900; en-US; rv:1.9.3a5pre) Gecko/20100503 Namoroka/3.7a5pre Fennec/1.1b2pre
Status: RESOLVED → VERIFIED
Flags: in-litmus?
litmus testcase https://litmus.mozilla.org/show_test.cgi?id=7522 updated
Flags: in-litmus? → in-litmus+
Component: Linux/Maemo → General
QA Contact: maemo-linux → general
tracking-fennec: ? → ---
You need to log in before you can comment on or make changes to this bug.