Last Comment Bug 715518 - WASD controls don't work in 3D view if typeaheadfind is active
: WASD controls don't work in 3D view if typeaheadfind is active
Status: RESOLVED FIXED
[tilt][fixed-in-fx-team]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: Firefox 12
Assigned To: Victor Porof [:vporof][:vp]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-05 08:18 PST by Olli Pettay [:smaug]
Modified: 2012-01-21 07:37 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (1.91 KB, patch)
2012-01-13 13:35 PST, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review
v2 (7.19 KB, patch)
2012-01-13 15:37 PST, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review
v3 (14.44 KB, patch)
2012-01-15 04:11 PST, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review
v4 (7.99 KB, patch)
2012-01-15 23:10 PST, Victor Porof [:vporof][:vp]
rcampbell: review+
Details | Diff | Splinter Review

Description Olli Pettay [:smaug] 2012-01-05 08:18:04 PST
If the "search for text when I start typing" is active, using
W-A-S-D tries to just find some text in the page, and controlling
the 3D view doesn't work.
Comment 1 Victor Porof [:vporof][:vp] 2012-01-13 13:35:56 PST
Created attachment 588512 [details] [diff] [review]
v1
Comment 2 Victor Porof [:vporof][:vp] 2012-01-13 15:37:31 PST
Created attachment 588548 [details] [diff] [review]
v2

On a closer look, it seems that this bug isn't limited only on WASD keys. For example, the functionality introduced by bug 712029 relies on the R key. Or, for zooming, we'd have to use + - or I O keys, and that would also trigger the Quick Find search box when typeahead is enabled.

Added a patch fixing every possible use case, and avoiding stuff like this to happen in the future.
Comment 3 Victor Porof [:vporof][:vp] 2012-01-14 01:08:21 PST
https://tbpl.mozilla.org/php/getParsedLog.php?id=8540877&tree=Try

TEST-INFO | chrome://mochitests/content/browser/browser/devtools/tilt/test/browser_tilt_arcball.js | Console message: [JavaScript Warning: "WebGL: Can't get a usable WebGL context" {file: "resource:///modules/devtools/TiltGL.jsm" line: 1606}]
TEST-INFO | chrome://mochitests/content/browser/browser/devtools/tilt/test/browser_tilt_arcball.js | Console message: [JavaScript Warning: "WebGL: Can't get a usable WebGL context" {file: "resource:///modules/devtools/TiltGL.jsm" line: 1606}]
TEST-INFO | chrome://mochitests/content/browser/browser/devtools/tilt/test/browser_tilt_arcball.js | Console message: [JavaScript Error: "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIDOMHTMLCanvasElement.getContext]"]
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/tilt/test/browser_tilt_arcball.js | application timed out after 330 seconds with no output

I know the problem: I'll need to test for WebGL first (like in all other tests). Only a small part of the test will require this.
Comment 4 Victor Porof [:vporof][:vp] 2012-01-15 04:11:43 PST
Created attachment 588726 [details] [diff] [review]
v3
Comment 5 Victor Porof [:vporof][:vp] 2012-01-15 23:10:59 PST
Created attachment 588814 [details] [diff] [review]
v4

Cleaned up. https://tbpl.mozilla.org/?tree=Try&rev=99f836e9fef1
Comment 6 Rob Campbell [:rc] (:robcee) 2012-01-19 12:53:58 PST
Comment on attachment 588814 [details] [diff] [review]
v4

+++ b/browser/devtools/tilt/TiltVisualizer.jsm
@@ -994,14 +994,12 @@
   {
     let code = e.keyCode || e.which;
 
-    if (code >= e.DOM_VK_LEFT && code <= e.DOM_VK_DOWN) {
+    if (!e.altKey && !e.ctrlKey && !e.metaKey && !e.shiftKey) {
       e.preventDefault();
       e.stopPropagation();
-    }
-    if (e.altKey || e.ctrlKey || e.metaKey || e.shiftKey) {
+      this.arcball.keyDown(code);
+    } else {
       this.arcball.cancelKeyEvents();
-    } else {
-      this.arcball.keyDown(code);
     }
   },
 

alright, lemme see if I've got this: You're checking that the event doesn't include a modifier key, and if so, you preventDefault, stopPropagation and then send the key code the arcball controller. And then cancel them?

This only happens when the tilt context has focus or does this capture all key events? Like on the breadcrumbs and HTML panel too?
Comment 7 Victor Porof [:vporof][:vp] 2012-01-19 13:55:43 PST
*head spinning*

I think you got this wrong. The code would be 

if (!e.altKey && !e.ctrlKey && !e.metaKey && !e.shiftKey) {
  e.preventDefault();
  e.stopPropagation();
  this.arcball.keyDown(code);
} else {
  this.arcball.cancelKeyEvents();
}

First of all, this line a bit above would help: 
canvas.addEventListener("keydown", this.onKeyDown, false);

Same goes for all other events. So, obviously, I'm catching events only on the canvas overlay when it has focus.

Second of all:
* check if a modifier key is pressed; if not, tilt is king and proceed with what the arcball needs to do. (the modifier key scenario avoids tons of unfiled bugs which we can talk about on irc).
* if a modifier key *is* pressed, cancel all key actions happening in the arcball

This isn't a thing I fixed here, it was an older fix. I'm rewording 
"if (e.altKey || e.ctrlKey || e.metaKey || e.shiftKey)" to
"if (!e.altKey && !e.ctrlKey && !e.metaKey && !e.shiftKey)"
because i cleaned up some code in those key event handlers in this particular bug (no more "if (code >= e.DOM_VK_LEFT && code <= e.DOM_VK_DOWN)").

Talk to me on IRC.
Comment 8 Rob Campbell [:rc] (:robcee) 2012-01-20 09:23:04 PST
(In reply to Victor Porof from comment #7)
> *head spinning*
> 
> I think you got this wrong. The code would be 

quite prossibly!

> if (!e.altKey && !e.ctrlKey && !e.metaKey && !e.shiftKey) {
>   e.preventDefault();
>   e.stopPropagation();
>   this.arcball.keyDown(code);
> } else {
>   this.arcball.cancelKeyEvents();
> }
> 
> First of all, this line a bit above would help: 
> canvas.addEventListener("keydown", this.onKeyDown, false);
> 
> Same goes for all other events. So, obviously, I'm catching events only on
> the canvas overlay when it has focus.

Ah, ok, that's helpful. A day of code reviews makes rob a something-something. Should've looked up the originating method.

> Second of all:
> * check if a modifier key is pressed; if not, tilt is king and proceed with
> what the arcball needs to do. (the modifier key scenario avoids tons of
> unfiled bugs which we can talk about on irc).

clown'll eat me.

> * if a modifier key *is* pressed, cancel all key actions happening in the
> arcball
> 
> This isn't a thing I fixed here, it was an older fix. I'm rewording 
> "if (e.altKey || e.ctrlKey || e.metaKey || e.shiftKey)" to
> "if (!e.altKey && !e.ctrlKey && !e.metaKey && !e.shiftKey)"
> because i cleaned up some code in those key event handlers in this
> particular bug (no more "if (code >= e.DOM_VK_LEFT && code <=
> e.DOM_VK_DOWN)").
> 
> Talk to me on IRC.

OK.
Comment 9 Rob Campbell [:rc] (:robcee) 2012-01-20 10:46:53 PST
https://hg.mozilla.org/integration/fx-team/rev/0f664cd835ec
Comment 10 Rob Campbell [:rc] (:robcee) 2012-01-21 07:37:48 PST
https://hg.mozilla.org/mozilla-central/rev/0f664cd835ec

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