Last Comment Bug 719039 - After the Highlighter was refactored, ESCAPE key closes Inspector when Tilt is open
: After the Highlighter was refactored, ESCAPE key closes Inspector when Tilt i...
Status: RESOLVED FIXED
[tilt]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Inspector (show other bugs)
: 12 Branch
: All All
-- normal (vote)
: Firefox 12
Assigned To: Victor Porof [:vporof][:vp]
:
: Gabriel Luong [:gl][1 biz day review guarantee] (ΦωΦ)
Mentors:
Depends on: 719732
Blocks: 719877
  Show dependency treegraph
 
Reported: 2012-01-18 07:41 PST by Victor Porof [:vporof][:vp]
Modified: 2012-01-31 01:24 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (2.73 KB, patch)
2012-01-20 06:28 PST, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review
v2 (4.12 KB, patch)
2012-01-20 06:43 PST, Victor Porof [:vporof][:vp]
paul: feedback+
Details | Diff | Splinter Review
v3 (4.67 KB, patch)
2012-01-20 07:11 PST, Victor Porof [:vporof][:vp]
no flags Details | Diff | Splinter Review
v4010 (4.77 KB, patch)
2012-01-27 14:28 PST, Victor Porof [:vporof][:vp]
rcampbell: review+
Details | Diff | Splinter Review

Description User image Victor Porof [:vporof][:vp] 2012-01-18 07:41:05 PST
Until now, the behavior was:

Inspector open             + ESCAPE pressed = Inspector closed
Inspector open & Tilt open + ESCAPE pressed = only Tilt closed

So currently, if Tilt is open and ESCAPE is pressed, we're not returning back to the Highlighter, but closing Inspector altogether. If this is the desired behavior, we can remove some redundant code from Tilt. However, I'm thinking it's not.
Comment 1 User image Paul Rouget [:paul] 2012-01-18 07:49:09 PST
You're right, this is a bug. We should go back to the initial behavior.
Comment 2 User image Victor Porof [:vporof][:vp] 2012-01-20 00:56:46 PST
I'll take a look at this.
Comment 3 User image Victor Porof [:vporof][:vp] 2012-01-20 06:28:22 PST
Created attachment 590184 [details] [diff] [review]
v1

The only change worth looking at is in browser/devtools/highlighter/inspector.jsm. I added a currentInstance getter in Tilt to get the visualizer for the current tab.
Comment 4 User image Victor Porof [:vporof][:vp] 2012-01-20 06:43:58 PST
Created attachment 590188 [details] [diff] [review]
v2

Using the bubbling trick. I left the currentInstance getter in there because it may be useful sometime!
Comment 5 User image Paul Rouget [:paul] 2012-01-20 07:07:35 PST
Comment on attachment 590188 [details] [diff] [review]
v2

This approach sounds good.

Not sure this is needed:
+      e.preventDefault();

Why don't you just use `code = e.keyCode` ?
Comment 6 User image Victor Porof [:vporof][:vp] 2012-01-20 07:11:34 PST
Created attachment 590193 [details] [diff] [review]
v3
Comment 7 User image Rob Campbell [:rc] (:robcee) 2012-01-27 11:56:14 PST
Comment on attachment 590193 [details] [diff] [review]
v3

+    canvas.addEventListener("keypress", this.onKeyPress, false);

should this be , true? Do we want capturing?

e.g., Inspector is now using:

     this.highlighter.highlighterContainer.addEventListener("keypress",
       this.onKeypress, true);

(line 367, InspectorUI).

and for completeness, you probably want to use preventDefault() as well as stopPropagation().
Comment 8 User image Rob Campbell [:rc] (:robcee) 2012-01-27 12:11:27 PST
ok, scratch that bit about needing preventDefault() it's redundant.
Comment 9 User image Victor Porof [:vporof][:vp] 2012-01-27 12:15:42 PST
(In reply to Rob Campbell [:rc] (robcee) from comment #8)
> ok, scratch that bit about needing preventDefault() it's redundant.

But I wanna!
Comment 10 User image Victor Porof [:vporof][:vp] 2012-01-27 14:28:26 PST
Created attachment 592273 [details] [diff] [review]
v4010
Comment 12 User image Tim Taubert [:ttaubert] 2012-01-31 01:24:12 PST
https://hg.mozilla.org/mozilla-central/rev/39afbd62c96e

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