Closed Bug 691884 Opened 13 years ago Closed 13 years ago

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

Categories

(DevTools :: Inspector, defect, P2)

defect

Tracking

(firefox10+ verified)

RESOLVED FIXED
Firefox 11
Tracking Status
firefox10 + verified

People

(Reporter: johan.charlez, Assigned: johan.charlez)

References

Details

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

Attachments

(2 files, 3 obsolete files)

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
Component: General → Developer Tools
OS: Windows 7 → All
Hardware: x86_64 → All
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
Escape is used to lock and unlock a node.
(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).
Stephen, what do you think about this? Using escape to toggle the state of the inspection instead of closing the inspector?
We're doing developer tool prioritization, filter on 'brontozaur'
to ignore the spam.

P2 at least for a decision.
Priority: -- → P2
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?
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.
Whiteboard: [good first bug][mentor=paul]
I'd like to give it a go.
(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.
Whiteboard: [good first bug][mentor=paul] → [good first bug][mentor=paul][lang=js]
Assignee: nobody → johan.charlez
Status: NEW → ASSIGNED
Attached patch Initial patch (obsolete) — Splinter Review
Attachment #575455 - Flags: feedback?(paul)
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
Assignee: johan.charlez → nobody
Component: Developer Tools → Developer Tools: Inspector
QA Contact: general → developer.tools.inspector
Assignee: nobody → johan.charlez
Attached patch patch v0.2 (obsolete) — Splinter Review
Attachment #575674 - Flags: feedback?
Attachment #575674 - Flags: feedback? → feedback?(paul)
Attachment #575455 - Attachment is obsolete: true
Attachment #575455 - Flags: feedback?(paul)
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 :)
Attached patch patch v0.3 (obsolete) — Splinter Review
Attachment #575674 - Attachment is obsolete: true
Attachment #575674 - Flags: feedback?(paul)
Attachment #575706 - Flags: feedback?(paul)
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+
Version: 8 Branch → Trunk
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.
Attachment #575706 - Flags: review?(rcampbell)
Attached patch patch v0.3.1Splinter Review
Attachment #575706 - Attachment is obsolete: true
Attachment #575743 - Flags: review?(rcampbell)
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.
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
Closed: 13 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+
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.
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.
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]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.