Closed
Bug 691884
Opened 13 years ago
Closed 13 years ago
Pressing Escape 'Esc' should close developer 'Inspect' mode.
Categories
(DevTools :: Inspector, defect, P2)
DevTools
Inspector
Tracking
(firefox10+ verified)
RESOLVED
FIXED
Firefox 11
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)
4.56 KB,
patch
|
rcampbell
:
review+
|
Details | Diff | Splinter Review |
4.32 KB,
patch
|
johnath
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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
Updated•13 years ago
|
OS: Windows 7 → All
Hardware: x86_64 → All
Comment 1•13 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.
Comment 2•13 years ago
|
||
Escape is used to lock and unlock a node.
Comment 3•13 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•13 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•13 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•13 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.
Comment 7•13 years ago
|
||
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•13 years ago
|
||
(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•13 years ago
|
||
(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•13 years ago
|
||
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•13 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.
Comment 12•13 years ago
|
||
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•13 years ago
|
Whiteboard: [good first bug][mentor=paul]
Assignee | ||
Comment 13•13 years ago
|
||
I'd like to give it a go.
Comment 14•13 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•13 years ago
|
||
You'll find the code here: http://mxr.mozilla.org/mozilla-central/source/browser/devtools/highlighter/inspector.jsm#1249
Updated•13 years ago
|
Whiteboard: [good first bug][mentor=paul] → [good first bug][mentor=paul][lang=js]
Updated•13 years ago
|
Assignee: nobody → johan.charlez
Status: NEW → ASSIGNED
Assignee | ||
Comment 16•13 years ago
|
||
Attachment #575455 -
Flags: feedback?(paul)
Comment 17•13 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•13 years ago
|
Assignee: johan.charlez → nobody
Component: Developer Tools → Developer Tools: Inspector
QA Contact: general → developer.tools.inspector
Updated•13 years ago
|
Assignee: nobody → johan.charlez
Assignee | ||
Comment 18•13 years ago
|
||
Attachment #575674 -
Flags: feedback?
Attachment #575674 -
Flags: feedback? → feedback?(paul)
Attachment #575455 -
Attachment is obsolete: true
Attachment #575455 -
Flags: feedback?(paul)
Comment 19•13 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•13 years ago
|
||
Attachment #575674 -
Attachment is obsolete: true
Attachment #575674 -
Flags: feedback?(paul)
Attachment #575706 -
Flags: feedback?(paul)
Comment 21•13 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•13 years ago
|
Version: 8 Branch → Trunk
Assignee | ||
Comment 22•13 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•13 years ago
|
Attachment #575706 -
Flags: review?(rcampbell)
Assignee | ||
Comment 23•13 years ago
|
||
Attachment #575706 -
Attachment is obsolete: true
Attachment #575743 -
Flags: review?(rcampbell)
Assignee | ||
Comment 24•13 years ago
|
||
Comment on attachment 575743 [details] [diff] [review] patch v0.3.1 Edited a comment to reflect the changes in this bug.
Comment 25•13 years ago
|
||
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 26•13 years ago
|
||
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•13 years ago
|
||
Comment 28•13 years ago
|
||
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]
Comment 29•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/450b1a81d2b0
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 11
Comment 30•13 years ago
|
||
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 31•13 years ago
|
||
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+
Updated•13 years ago
|
tracking-firefox10:
--- → ?
Comment 32•13 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/df597ccf379e done!
status-firefox10:
--- → fixed
Comment 33•12 years ago
|
||
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•12 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•12 years ago
|
||
I filed https://bugzilla.mozilla.org/show_bug.cgi?id=713676.
Comment 36•12 years ago
|
||
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]
Updated•12 years ago
|
Updated•12 years ago
|
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•