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]
:
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 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 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 Victor Porof [:vporof][:vp] 2012-01-20 00:56:46 PST
I'll take a look at this.
Comment 3 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 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 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 Victor Porof [:vporof][:vp] 2012-01-20 07:11:34 PST
Created attachment 590193 [details] [diff] [review]
v3
Comment 7 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 Rob Campbell [:rc] (:robcee) 2012-01-27 12:11:27 PST
ok, scratch that bit about needing preventDefault() it's redundant.
Comment 9 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 Victor Porof [:vporof][:vp] 2012-01-27 14:28:26 PST
Created attachment 592273 [details] [diff] [review]
v4010
Comment 12 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.