Last Comment Bug 691884 - Pressing Escape 'Esc' should close developer 'Inspect' mode.
: Pressing Escape 'Esc' should close developer 'Inspect' mode.
Status: RESOLVED FIXED
[good first bug][mentor=paul][lang=js...
: verified-beta
Product: Firefox
Classification: Client Software
Component: Developer Tools: Inspector (show other bugs)
: Trunk
: All All
: P2 normal (vote)
: Firefox 11
Assigned To: Johan C
:
: Patrick Brosset <:pbro>
Mentors:
Depends on: 713676
Blocks: 663830
  Show dependency treegraph
 
Reported: 2011-10-04 12:52 PDT by Johan C
Modified: 2012-01-03 13:14 PST (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
verified


Attachments
Initial patch (1.09 KB, patch)
2011-11-18 07:50 PST, Johan C
no flags Details | Diff | Splinter Review
patch v0.2 (2.13 KB, patch)
2011-11-19 07:54 PST, Johan C
no flags Details | Diff | Splinter Review
patch v0.3 (3.74 KB, patch)
2011-11-19 15:48 PST, Johan C
paul: feedback+
Details | Diff | Splinter Review
patch v0.3.1 (4.56 KB, patch)
2011-11-20 06:43 PST, Johan C
rcampbell: review+
Details | Diff | Splinter Review
addressed some minor issues (4.32 KB, patch)
2011-11-21 10:03 PST, Johan C
bugzilla: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Johan C 2011-10-04 12:52:30 PDT
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
Comment 1 B.J. Herbison 2011-10-24 07:46:27 PDT
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.
Comment 2 Paul Rouget [:paul] 2011-10-24 08:02:54 PDT
Escape is used to lock and unlock a node.
Comment 3 B.J. Herbison 2011-10-24 16:21:04 PDT
(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 Paul Rouget [:paul] 2011-10-25 07:54:36 PDT
Stephen, what do you think about this? Using escape to toggle the state of the inspection instead of closing the inspector?
Comment 5 Dave Camp (:dcamp) 2011-10-27 08:43:02 PDT
We're doing developer tool prioritization, filter on 'brontozaur'
to ignore the spam.

P2 at least for a decision.
Comment 6 Paul Rouget [:paul] 2011-11-01 08:56:14 PDT
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.
Comment 7 Rob Campbell [:rc] (:robcee) 2011-11-01 19:58:17 PDT
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.
Comment 8 Stephen Horlander [:shorlander] 2011-11-02 07:37:42 PDT
(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?
Comment 9 Rob Campbell [:rc] (:robcee) 2011-11-02 08:53:31 PDT
(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.
Comment 10 Dão Gottwald [:dao] 2011-11-02 08:58:08 PDT
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?
Comment 11 Johan C 2011-11-02 09:17:30 PDT
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.
Comment 12 Rob Campbell [:rc] (:robcee) 2011-11-02 14:36:04 PDT
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.
Comment 13 Johan C 2011-11-16 16:22:17 PST
I'd like to give it a go.
Comment 14 Paul Rouget [:paul] 2011-11-17 01:35:21 PST
(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 Paul Rouget [:paul] 2011-11-17 01:59:12 PST
You'll find the code here: http://mxr.mozilla.org/mozilla-central/source/browser/devtools/highlighter/inspector.jsm#1249
Comment 16 Johan C 2011-11-18 07:50:28 PST
Created attachment 575455 [details] [diff] [review]
Initial patch
Comment 17 Paul Rouget [:paul] 2011-11-18 08:18:02 PST
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
Comment 18 Johan C 2011-11-19 07:54:49 PST
Created attachment 575674 [details] [diff] [review]
patch v0.2
Comment 19 Paul Rouget [:paul] 2011-11-19 08:24:00 PST
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 :)
Comment 20 Johan C 2011-11-19 15:48:32 PST
Created attachment 575706 [details] [diff] [review]
patch v0.3
Comment 21 Paul Rouget [:paul] 2011-11-19 16:13:58 PST
Comment on attachment 575706 [details] [diff] [review]
patch v0.3

f+. Good work. Time for a formal review now.
Comment 22 Johan C 2011-11-19 16:54:28 PST
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.
Comment 23 Johan C 2011-11-20 06:43:43 PST
Created attachment 575743 [details] [diff] [review]
patch v0.3.1
Comment 24 Johan C 2011-11-20 06:49:47 PST
Comment on attachment 575743 [details] [diff] [review]
patch v0.3.1

Edited a comment to reflect the changes in this bug.
Comment 25 Rob Campbell [:rc] (:robcee) 2011-11-21 09:03:34 PST
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!
Comment 26 Rob Campbell [:rc] (:robcee) 2011-11-21 09:12:38 PST
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.
Comment 27 Johan C 2011-11-21 10:03:10 PST
Created attachment 575898 [details] [diff] [review]
addressed some minor issues
Comment 28 Rob Campbell [:rc] (:robcee) 2011-11-21 12:54:23 PST
https://hg.mozilla.org/integration/fx-team/rev/450b1a81d2b0
Comment 29 Rob Campbell [:rc] (:robcee) 2011-11-22 06:32:07 PST
https://hg.mozilla.org/mozilla-central/rev/450b1a81d2b0
Comment 30 Rob Campbell [:rc] (:robcee) 2011-11-22 06:33:51 PST
Comment on attachment 575898 [details] [diff] [review]
addressed some minor issues

key binding fix, improves usability. Low-risk.
Comment 31 Johnathan Nightingale [:johnath] 2011-11-22 14:48:15 PST
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!)
Comment 32 Rob Campbell [:rc] (:robcee) 2011-11-23 02:38:18 PST
https://hg.mozilla.org/releases/mozilla-aurora/rev/df597ccf379e

done!
Comment 33 Paul Silaghi, QA [:pauly] 2011-12-27 06:02:54 PST
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.
Comment 34 Johan C 2011-12-27 09:23:41 PST
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.
Comment 35 Johan C 2011-12-27 10:17:59 PST
I filed https://bugzilla.mozilla.org/show_bug.cgi?id=713676.
Comment 36 Paul Silaghi, QA [:pauly] 2011-12-28 00:29:06 PST
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

Note You need to log in before you can comment on or make changes to this bug.