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

RESOLVED FIXED in Firefox 12

Status

()

Firefox
Developer Tools
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: smaug, Assigned: vporof)

Tracking

unspecified
Firefox 12
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

5 years ago
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)

Updated

5 years ago
Assignee: nobody → vporof
Whiteboard: tilt]
(Assignee)

Updated

5 years ago
Whiteboard: tilt] → [tilt]
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 1

5 years ago
Created attachment 588512 [details] [diff] [review]
v1
Attachment #588512 - Flags: review?(rcampbell)
(Assignee)

Comment 2

5 years ago
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.
Attachment #588512 - Attachment is obsolete: true
Attachment #588512 - Flags: review?(rcampbell)
Attachment #588548 - Flags: review?(rcampbell)
(Assignee)

Comment 3

5 years ago
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.
(Assignee)

Comment 4

5 years ago
Created attachment 588726 [details] [diff] [review]
v3
Attachment #588548 - Attachment is obsolete: true
Attachment #588548 - Flags: review?(rcampbell)
Attachment #588726 - Flags: review?(rcampbell)
(Assignee)

Comment 5

5 years ago
Created attachment 588814 [details] [diff] [review]
v4

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?
(Assignee)

Comment 7

5 years ago
*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
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 12
You need to log in before you can comment on or make changes to this bug.