710762 - Keyboard control for zoom in tilt (page inspector 3D)

Status: VERIFIED FIXED

(DevTools :: General, defect)

Description

[Kevin Dangoor]

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]]

Attachment: v1

Comment 2

[Rob Campbell [:rc] (:robcee)]

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]]

Attachment: v2

Don't ask me how I write these tests.

Comment 4

[Victor Porof [:vporof][:vp]]

Attachment: 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)]

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]]

(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)]

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]]

Attachment: v4

Fixed typo. Followup bug for localization would be perfect.

Comment 9

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 582816 [details] [diff] [review]
v4

ok!

Comment 10

[Rob Campbell [:rc] (:robcee)]

https://hg.mozilla.org/integration/fx-team/rev/de1cf77a8d6b

Comment 11

[Rob Campbell [:rc] (:robcee)]

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]]

For the record, there's the followup bug 712026 for localization.

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/de1cf77a8d6b

Comment 14

[LegNeato]

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)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/37d9a6bb1156

Comment 16

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 582518 [details] [diff] [review]
v3

clearing old requests.

Comment 17

[Mihaela Velimiroviciu (:mihaelav)]

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.