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

RESOLVED FIXED in Firefox 12

Status

()

Firefox
Developer Tools: Inspector
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: vporof, Assigned: vporof)

Tracking

12 Branch
Firefox 12
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [tilt])

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

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

Updated

6 years ago
Whiteboard: [tilt]

Comment 1

6 years ago
You're right, this is a bug. We should go back to the initial behavior.
(Assignee)

Comment 2

6 years ago
I'll take a look at this.
Assignee: nobody → vporof
Status: NEW → ASSIGNED
(Assignee)

Comment 3

6 years ago
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.
Attachment #590184 - Flags: feedback?(paul)
(Assignee)

Comment 4

6 years ago
Created attachment 590188 [details] [diff] [review]
v2

Using the bubbling trick. I left the currentInstance getter in there because it may be useful sometime!
Attachment #590188 - Flags: feedback?(paul)

Updated

6 years ago
Blocks: 719783

Comment 5

6 years ago
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` ?
Attachment #590188 - Flags: feedback?(paul) → feedback+
(Assignee)

Comment 6

6 years ago
Created attachment 590193 [details] [diff] [review]
v3
Attachment #590184 - Attachment is obsolete: true
Attachment #590184 - Flags: feedback?(paul)
Attachment #590193 - Flags: review?(rcampbell)
Attachment #590193 - Flags: feedback?(paul)

Updated

6 years ago
No longer blocks: 719783
(Assignee)

Updated

6 years ago
Depends on: 719732
(Assignee)

Updated

6 years ago
Blocks: 719877
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().
Attachment #590193 - Flags: review?(rcampbell)
ok, scratch that bit about needing preventDefault() it's redundant.
(Assignee)

Comment 9

6 years ago
(In reply to Rob Campbell [:rc] (robcee) from comment #8)
> ok, scratch that bit about needing preventDefault() it's redundant.

But I wanna!
(Assignee)

Comment 10

6 years ago
Created attachment 592273 [details] [diff] [review]
v4010
Attachment #590188 - Attachment is obsolete: true
Attachment #590193 - Attachment is obsolete: true
Attachment #590193 - Flags: feedback?(paul)
Attachment #592273 - Flags: review?(rcampbell)
Attachment #592273 - Flags: review?(rcampbell) → review+
Whiteboard: [tilt] → [tilt][land-in-fx-team]

Comment 11

6 years ago
https://hg.mozilla.org/integration/fx-team/rev/39afbd62c96e
Whiteboard: [tilt][land-in-fx-team] → [tilt][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/39afbd62c96e
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [tilt][fixed-in-fx-team] → [tilt]
Target Milestone: --- → Firefox 12
You need to log in before you can comment on or make changes to this bug.