719039 - After the Highlighter was refactored, ESCAPE key closes Inspector when Tilt is open

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Victor Porof [:vporof][:vp]]

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]]

You're right, this is a bug. We should go back to the initial behavior.

Comment 2

[Victor Porof [:vporof][:vp]]

I'll take a look at this.

Comment 3

[Victor Porof [:vporof][:vp]]

Attachment: 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]]

Attachment: v2

Using the bubbling trick. I left the currentInstance getter in there because it may be useful sometime!

Comment 5

[Paul Rouget [:paul]]

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]]

Attachment: v3

Comment 7

[Rob Campbell [:rc] (:robcee)]

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)]

ok, scratch that bit about needing preventDefault() it's redundant.

Comment 9

[Victor Porof [:vporof][:vp]]

(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]]

Attachment: v4010

Comment 11

[Paul Rouget [:paul]]

https://hg.mozilla.org/integration/fx-team/rev/39afbd62c96e

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/39afbd62c96e