Last Comment Bug 710762 - Keyboard control for zoom in tilt (page inspector 3D)
: Keyboard control for zoom in tilt (page inspector 3D)
Status: VERIFIED FIXED
[tilt][fixed-in-fx-team][qa!]
: verified-beta
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: Firefox 12
Assigned To: Victor Porof [:vporof][:vp]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-14 09:43 PST by Kevin Dangoor
Modified: 2012-02-07 05:17 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
verified


Attachments
v1 (2.03 KB, patch)
2011-12-15 05:51 PST, Victor Porof [:vporof][:vp]
rcampbell: review-
Details | Diff | Splinter Review
v2 (10.64 KB, patch)
2011-12-17 01:52 PST, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review
v3 (12.85 KB, patch)
2011-12-17 04:03 PST, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review
v4 (12.84 KB, patch)
2011-12-19 06:42 PST, Victor Porof [:vporof][:vp]
rcampbell: review+
christian: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Kevin Dangoor 2011-12-14 09:43:07 PST
Victor tells me that there's no way right now to zoom using just the keyboard in 3D mode in the page inspector. It would be good to have one. The = and - keys seem like good options (for zoom in/out, respectively). Supporting shift+those keys would be good as well, so that the user could push + for zoom in.
Comment 1 Victor Porof [:vporof][:vp] 2011-12-15 05:51:23 PST
Created attachment 581938 [details] [diff] [review]
v1
Comment 2 Rob Campbell [:rc] (:robcee) 2011-12-16 07:27:10 PST
Comment on attachment 581938 [details] [diff] [review]
v1

+    if (str === "+" || str === "=") {
+      this.arcball.mouseScroll(-ARCBALL_TRANSLATION_STEP);
+    } else if (str === "-" || str === "_") {
+      this.arcball.mouseScroll(ARCBALL_TRANSLATION_STEP);
+    }

not sure all non-US keyboards have this arrangement.

see http://mxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/browser.dtd#471

Should probably mimic those keys.

Also, would be good to have a zoom reset key on Cmd/ctrl-0.

We should probably verify this with a test as well.
Comment 3 Victor Porof [:vporof][:vp] 2011-12-17 01:52:47 PST
Created attachment 582506 [details] [diff] [review]
v2

Don't ask me how I write these tests.
Comment 4 Victor Porof [:vporof][:vp] 2011-12-17 04:03:04 PST
Created attachment 582518 [details] [diff] [review]
v3

It would also probably best if zooming via keyboard and mouse would be unified in the arcball implementation. Added fix in here because a new bug would seem too much for this.
Comment 5 Rob Campbell [:rc] (:robcee) 2011-12-19 05:07:52 PST
Comment on attachment 582518 [details] [diff] [review]
v3

   /**
    * Object retaining the current pressed key codes.
@@ -1114,6 +1109,7 @@
   this._lastTrans = vec3.create();
   this._deltaTrans = vec3.create();
   this._currentTrans = vec3.create(aInitialTrans);
+  this._zoomAmmount = 0;
 
zoomAmount (typo!)

 
-    let scrollValue = this._scrollValue;
+    let zoomAmmount = this._zoomAmmount;
     let keyCode = this._keyCode;

+    if (keyCode[Ci.nsIDOMKeyEvent.DOM_VK_I] ||
+        keyCode[Ci.nsIDOMKeyEvent.DOM_VK_ADD] ||
+        keyCode[Ci.nsIDOMKeyEvent.DOM_VK_EQUALS]) {

these are going to be impossible to localize. Kind of the reason I wanted to sniff the keycodes off of the document's page zoom keys. Either that or put them in a properties file.

Certainly for I, O and R those keys may not make sense in other locales. +/- and = may be ok, but I've never really looked to see how they get localized elsewhere.

It also looks like we could replace all the previous if(key) checks with a switch statement.

+   * Function handling the arcball zoom ammount.

pesky typo again.

So, typos fixed, not fully sure what to do about the l10n issue. Might be ok for a follow-up.

Axel, do you have any recommendations for zoom key controls? Is sniffing the DOM for the current zoom keys an acceptable solution or should we bundle these into a properties file? Thanks.
Comment 6 Victor Porof [:vporof][:vp] 2011-12-19 06:14:14 PST
(In reply to Rob Campbell [:rc] (robcee) from comment #5)
> 
> these are going to be impossible to localize. Kind of the reason I wanted to
> sniff the keycodes off of the document's page zoom keys. Either that or put
> them in a properties file.
> 
> Certainly for I, O and R those keys may not make sense in other locales. +/-
> and = may be ok, but I've never really looked to see how they get localized
> elsewhere.
> 

Then maybe we should localize ALL the keys? (even for up/down/left/right and rotations..)

> 
> So, typos fixed, not fully sure what to do about the l10n issue. Might be ok
> for a follow-up.
> 
> Axel, do you have any recommendations for zoom key controls? Is sniffing the
> DOM for the current zoom keys an acceptable solution or should we bundle
> these into a properties file? Thanks.

Afaik, sniffing would be bad because it returns a string ("+" for zoom, etc.). It's not safe to get the correct keycode from that. (For example, on my keyboard "-" is interpreted as the same keycode for DOM_VK_INSERT, and doing fromCharCode returns "M" for the keyCode I get when I press "-").
Comment 7 Rob Campbell [:rc] (:robcee) 2011-12-19 06:32:51 PST
hm, yeah. OK.

Maybe we can file a bug to make the other keys localizeable. Want to file a bug for that?

In the meantime we can get this in for this release (with the typo fixed).
Comment 8 Victor Porof [:vporof][:vp] 2011-12-19 06:42:34 PST
Created attachment 582816 [details] [diff] [review]
v4

Fixed typo. Followup bug for localization would be perfect.
Comment 9 Rob Campbell [:rc] (:robcee) 2011-12-19 08:39:05 PST
Comment on attachment 582816 [details] [diff] [review]
v4

ok!
Comment 10 Rob Campbell [:rc] (:robcee) 2011-12-20 09:03:55 PST
https://hg.mozilla.org/integration/fx-team/rev/de1cf77a8d6b
Comment 11 Rob Campbell [:rc] (:robcee) 2011-12-20 09:04:52 PST
Comment on attachment 582816 [details] [diff] [review]
v4

This would be nice to get into aurora. It adds page zoom controls on +, -, I, O keys. It even has a test!
Comment 12 Victor Porof [:vporof][:vp] 2011-12-21 03:14:32 PST
For the record, there's the followup bug 712026 for localization.
Comment 13 Ed Morley [:emorley] 2011-12-21 11:58:09 PST
https://hg.mozilla.org/mozilla-central/rev/de1cf77a8d6b
Comment 14 christian 2011-12-21 15:56:36 PST
Comment on attachment 582816 [details] [diff] [review]
v4

[triage comment]
Approved for aurora. While this probably wouldn't meet our criteria at the end of a cycle ("nice to have", feature works fine without, etc). We are early in the cycle and the risk looks low to take this for what will likely be a banner feature.
Comment 15 Rob Campbell [:rc] (:robcee) 2012-01-04 06:32:42 PST
https://hg.mozilla.org/releases/mozilla-aurora/rev/37d9a6bb1156
Comment 16 Rob Campbell [:rc] (:robcee) 2012-01-23 10:02:47 PST
Comment on attachment 582518 [details] [diff] [review]
v3

clearing old requests.
Comment 17 Mihaela Velimiroviciu (:mihaelav) 2012-02-07 05:17:03 PST
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:11.0) Gecko/20100101 Firefox/11.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:12.0a2) Gecko/20120206 Firefox/12.0a2

Verified the fix on the latest beta build (11.0 beta 1). "-"/"=", "I"/"O", "-"/"+" keys support zooming in and out in 3D mode in the page inspector.

Note You need to log in before you can comment on or make changes to this bug.