Pressing Escape 'Esc' should close developer 'Inspect' mode.

RESOLVED FIXED in Firefox 10

Status

()

Firefox
Developer Tools: Inspector
P2
normal
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Johan C, Assigned: Johan C)

Tracking

({verified-beta})

Trunk
Firefox 11
verified-beta
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox10+ verified)

Details

(Whiteboard: [good first bug][mentor=paul][lang=js][fixed-in-fx-team][qa+][qa!:10])

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

6 years ago
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:8.0) Gecko/20100101 Firefox/8.0
Build ID: 20110928060149

Steps to reproduce:

1) In 'inspect' mode
2) Press 'Esc'


Actual results:

3) nothing


Expected results:

Inspect mode should close
(Assignee)

Updated

6 years ago
Component: General → Developer Tools
OS: Windows 7 → All
Hardware: x86_64 → All

Comment 1

6 years ago
The description for step 3 is not quite accurate. The Inspector(Highlighter) selects the current element rather than going away. The Escape key is used to escape (get away), not to select.

Same behavior (selecting the current element) in Firefox 10 Nightly Highlighter.
Blocks: 663830
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 2

6 years ago
Escape is used to lock and unlock a node.

Comment 3

6 years ago
(In reply to Paul Rouget [:paul] from comment #2)
> Escape is used to lock and unlock a node.

That use does not seem intuitive to me since it's a close opposite of many uses of the escape key, and it doesn't seem discoverable to me since many people won't try it (as would be expected to close or cancel something) and I can't find any help the Highlighter that explains the keys.

I tried F1, ?, h, and right clicking to find Highlighter help (all four of those on the main window, the Highlighter bar on the bottom of the window, and the Style Inspector window), and I also entered the Firefox help and searched for Highlighter (I didn't really expect that to work as it a feature for a future version).

Comment 4

6 years ago
Stephen, what do you think about this? Using escape to toggle the state of the inspection instead of closing the inspector?

Comment 5

6 years ago
We're doing developer tool prioritization, filter on 'brontozaur'
to ignore the spam.

P2 at least for a decision.
Priority: -- → P2

Comment 6

6 years ago
I agree here. Escape should "clear" the highlighter (close it). To lock a node, "click" is good. But we need a mean to "unlock a node" (going to the inspecting mode) from the keyboard.

"Return" does that, but I don't consider this key to be a good choice because it is on the right side of the keyboard, so you can't use it if you use your mouse with the right hand (like most of the people).

What about using the backquote key (`) ? On a qwerty layout, its position is perfect.

Return and Backquote to toggle the inspecting state.
We had some discussion about this in IRC today. As Paul says in Comment 6 and Johan and BJ in Comments 0 and 3, ESC should probably close the Highlighter. +1 to this.

I don't know about using ` for unlocking the node. Not sure that key is present or in the same location on all keyboards. As a somewhat useless datapoint, on my current keyboard, it's on the top right.
(In reply to Rob Campbell [:rc] (robcee) from comment #7)
> ESC should probably close the Highlighter. +1 to this.

Yeah ESC usually backs out or cancels modes so this makes sense.

> I don't know about using ` for unlocking the node. Not sure that key is
> present or in the same location on all keyboards. As a somewhat useless
> datapoint, on my current keyboard, it's on the top right.

I think it is supposed to be in the same spot on all QWERTY layouts. Which keyboard do you have?
(In reply to Stephen Horlander from comment #8)
> > I don't know about using ` for unlocking the node. Not sure that key is
> > present or in the same location on all keyboards. As a somewhat useless
> > datapoint, on my current keyboard, it's on the top right.
> 
> I think it is supposed to be in the same spot on all QWERTY layouts. Which
> keyboard do you have?

I have a Happy Hacker Keyboard Lite 2 (for Mac)! Somewhat non-standard, but I don't think the ` key is guaranteed to be top-left on all keyboards.
On my German keyboard, I need to press shift+´ and then space to get `. So that's probably not an option. Such a shortcut also seems undiscoverable, as you're not going to display it anywhere, are you?
(Assignee)

Comment 11

6 years ago
Can't ESC unlock nodes and close the highlighter?

E.g if a node is locked, pressing on ESC would unlock that node, and if there isn't a locked node the highlighter would close.

Other possible keys for locking/unlocking a node: space or enter.
Enter already locks and unlocks nodes. Space is an option, but that would override the default paging behavior which might be unpopular. Also considered TAB but because it's used for widget navigation, this might also be a problem.

maybe we should just stick with Enter to lock and unlock and ESC to close.

Updated

6 years ago
Whiteboard: [good first bug][mentor=paul]
(Assignee)

Comment 13

6 years ago
I'd like to give it a go.

Comment 14

6 years ago
(In reply to Johan from comment #13)
> I'd like to give it a go.

Sure thing! Ping me on IRC (paul on #devtools) if you don't know how to start.

Comment 15

6 years ago
You'll find the code here: http://mxr.mozilla.org/mozilla-central/source/browser/devtools/highlighter/inspector.jsm#1249

Updated

6 years ago
Whiteboard: [good first bug][mentor=paul] → [good first bug][mentor=paul][lang=js]

Updated

6 years ago
Assignee: nobody → johan.charlez
Status: NEW → ASSIGNED
(Assignee)

Comment 16

6 years ago
Created attachment 575455 [details] [diff] [review]
Initial patch
(Assignee)

Updated

6 years ago
Attachment #575455 - Flags: feedback?(paul)

Comment 17

6 years ago
Comment on attachment 575455 [details] [diff] [review]
Initial patch

So this looks good. I predict that this new behavior will break some tests. So you will have to update the tests too (located here: browser/devtools/highlighter/test )

To run the tests and see which one you're breaking, run:

TEST_PATH=browser/devtools/highlighter/test/ make -C ~/mozilla/obj mochitest-browser-chrome

Updated

6 years ago
Assignee: johan.charlez → nobody
Component: Developer Tools → Developer Tools: Inspector
QA Contact: general → developer.tools.inspector

Updated

6 years ago
Assignee: nobody → johan.charlez
(Assignee)

Comment 18

6 years ago
Created attachment 575674 [details] [diff] [review]
patch v0.2
Attachment #575674 - Flags: feedback?
(Assignee)

Updated

6 years ago
Attachment #575674 - Flags: feedback? → feedback?(paul)
(Assignee)

Updated

6 years ago
Attachment #575455 - Attachment is obsolete: true
Attachment #575455 - Flags: feedback?(paul)

Comment 19

6 years ago
Comment on attachment 575674 [details] [diff] [review]
patch v0.2

Review of attachment 575674 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. Thank you for that!

One more thing: in the test, we explicitly call InspectorUI.closeInspectoUI. Instead of that, you should simulate a ESC key event. Then you will test the "exit on ESC" feature as well. See what I mean?

Almost there :)
(Assignee)

Comment 20

6 years ago
Created attachment 575706 [details] [diff] [review]
patch v0.3
Attachment #575674 - Attachment is obsolete: true
Attachment #575674 - Flags: feedback?(paul)
Attachment #575706 - Flags: feedback?(paul)

Comment 21

6 years ago
Comment on attachment 575706 [details] [diff] [review]
patch v0.3

f+. Good work. Time for a formal review now.
Attachment #575706 - Flags: review?(rcampbell)
Attachment #575706 - Flags: feedback?(paul)
Attachment #575706 - Flags: feedback+

Updated

6 years ago
Version: 8 Branch → Trunk
(Assignee)

Comment 22

6 years ago
I'd like to fix one more thing first. The comment on line 1114 (http://mxr.mozilla.org/mozilla-central/source/browser/devtools/highlighter/inspector.jsm#1114) is no longer correct.

Updated

6 years ago
Attachment #575706 - Flags: review?(rcampbell)
(Assignee)

Comment 23

6 years ago
Created attachment 575743 [details] [diff] [review]
patch v0.3.1
Attachment #575706 - Attachment is obsolete: true
Attachment #575743 - Flags: review?(rcampbell)
(Assignee)

Comment 24

6 years ago
Comment on attachment 575743 [details] [diff] [review]
patch v0.3.1

Edited a comment to reflect the changes in this bug.
Comment on attachment 575743 [details] [diff] [review]
patch v0.3.1

Patch looks good. This should do the job.

some nits for you:

Watch out for extra spaces. Your test file has a bunch of lines of with single spaces in them. Blank lines should be just that.

+} 
\ No newline at end of file

Always have a newline or hg diff will complain.

(I'll make these corrections when checking it in)

Solid first patch!
Attachment #575743 - Flags: review?(rcampbell) → review+
Comment on attachment 575743 [details] [diff] [review]
patch v0.3.1

one other nit: feel free to add yourself to the Contributors section in the license block in inspector.jsm.
(Assignee)

Comment 27

6 years ago
Created attachment 575898 [details] [diff] [review]
addressed some minor issues
https://hg.mozilla.org/integration/fx-team/rev/450b1a81d2b0
Whiteboard: [good first bug][mentor=paul][lang=js] → [good first bug][mentor=paul][lang=js][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/450b1a81d2b0
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 11
Comment on attachment 575898 [details] [diff] [review]
addressed some minor issues

key binding fix, improves usability. Low-risk.
Attachment #575898 - Flags: approval-mozilla-aurora?
Comment on attachment 575898 [details] [diff] [review]
addressed some minor issues

This is approved for mozilla-aurora landing. Rob, can you escort this through the process ASAP? (Thanks, Johan!)
Attachment #575898 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
tracking-firefox10: --- → ?
https://hg.mozilla.org/releases/mozilla-aurora/rev/df597ccf379e

done!
status-firefox10: --- → fixed
Hi guys,
Couple of issues I found:

Build identifier: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:10.0) Gecko/20100101 Firefox/10.0

Steps to reproduce:
1. Open 2 tabs in Firefox
2. Go to www.google.com
3. Start the Inspect mode
4. Click on the google logo
5. Click on the other tab
6. Click back on google tab
7. Press ESC

Actual results:
Nothing happens

Expected results:
Inspect mode should exit

Notes:
After step 4, press on a button from the inspect bar on the bottom of the screen and then ESC. ESC doesn't work also.
(Assignee)

Comment 34

5 years ago
I can reproduce all of this on 10, 11 and 12, except for step 5 through 7 on central(12).
Looks to be focus related at first glance, but I shall look into it this further.
(Assignee)

Comment 35

5 years ago
I filed https://bugzilla.mozilla.org/show_bug.cgi?id=713676.

Updated

5 years ago
Depends on: 713676
Based on comment 34, this is verified fixed on Firefox 10 Beta1:
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:10.0) Gecko/20100101 Firefox/10.0
Mozilla/5.0 (X11; Linux x86_64; rv:10.0) Gecko/20100101 Firefox/10.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:10.0) Gecko/20100101 Firefox/10.0
Keywords: verified-beta
Whiteboard: [good first bug][mentor=paul][lang=js][fixed-in-fx-team] → [good first bug][mentor=paul][lang=js][fixed-in-fx-team][qa+][qa!:10]
status-firefox10: fixed → verified

Updated

5 years ago
tracking-firefox10: ? → +
You need to log in before you can comment on or make changes to this bug.