Closed Bug 715518 Opened 12 years ago Closed 12 years ago

WASD controls don't work in 3D view if typeaheadfind is active

Categories

(DevTools :: General, defect)

x86_64
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 12

People

(Reporter: smaug, Assigned: vporof)

Details

(Whiteboard: [tilt][fixed-in-fx-team])

Attachments

(1 file, 3 obsolete files)

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.
Assignee: nobody → vporof
Whiteboard: tilt]
Whiteboard: tilt] → [tilt]
Status: NEW → ASSIGNED
Attached patch v1 (obsolete) — Splinter Review
Attachment #588512 - Flags: review?(rcampbell)
Attached patch v2 (obsolete) — Splinter Review
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.
Attachment #588512 - Attachment is obsolete: true
Attachment #588512 - Flags: review?(rcampbell)
Attachment #588548 - Flags: review?(rcampbell)
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.
Attached patch v3 (obsolete) — Splinter Review
Attachment #588548 - Attachment is obsolete: true
Attachment #588548 - Flags: review?(rcampbell)
Attachment #588726 - Flags: review?(rcampbell)
Attached patch v4Splinter Review
Cleaned up. https://tbpl.mozilla.org/?tree=Try&rev=99f836e9fef1
Attachment #588726 - Attachment is obsolete: true
Attachment #588726 - Flags: review?(rcampbell)
Attachment #588814 - Flags: review?(rcampbell)
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?
*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.
(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.
Attachment #588814 - Flags: review?(rcampbell) → review+
Whiteboard: [tilt] → [tilt][land-in-fx-team]
https://hg.mozilla.org/integration/fx-team/rev/0f664cd835ec
Whiteboard: [tilt][land-in-fx-team] → [tilt][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/0f664cd835ec
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 12
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: