Closed
Bug 815239
Opened 12 years ago
Closed 11 years ago
[toolbox] Using a tool keyboard shortcut twice closes the toolbox. It should select it instead.
Categories
(DevTools :: Inspector, defect, P1)
Tracking
(firefox20 verified)
RESOLVED
FIXED
Firefox 21
Tracking | Status | |
---|---|---|
firefox20 | --- | verified |
People
(Reporter: rcampbell, Assigned: paul)
References
Details
Attachments
(2 files, 2 obsolete files)
6.96 KB,
patch
|
jwalker
:
review+
|
Details | Diff | Splinter Review |
6.92 KB,
patch
|
lsblakk
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
With the Devtools Toolbox patches landed, it's no longer possible to re-enable "Inspect mode" with the Cmd-alt-I keyboard shortcut. 1. Open Inspector (cmd-alt-I) 2. Click something to lock the Inspector. 3. Hit Cmd-alt-I again. Expect: Inspect mode to be re-enabled. Actual: Devtools panel closes.
Updated•12 years ago
|
Summary: Inspect Shortcut closes Devtools → [toolbox] Inspect Shortcut closes Devtools
Comment 1•12 years ago
|
||
Also: 1. Open Debugger (cmd+alt+S) 2. Switch to Inspector 3. Hit Cmd+alt+S again Toolbox closes. I'd expect to be switched to the debugger.
Assignee | ||
Updated•11 years ago
|
Summary: [toolbox] Inspect Shortcut closes Devtools → [toolbox] Using a tool keyboard shortcut twice closes the toolbox. It should select it instead.
Assignee | ||
Comment 2•11 years ago
|
||
Assignee | ||
Comment 3•11 years ago
|
||
+ * selectToolCommand's behavior:
+ * - if the toolbox is closed,
+ * we open the toolbox and select the tool
+ * - if the toolbox is open, and the targetted tool is selected,
+ * we close the toolbox
+ * - if the toolbox is open, and the targetted tool is not selected,
+ * we select it
Maybe we want to add an extra rule:
> if the toolbox is open, and the targetted tool is selected, AND the host is a window,
> we don't close, but focus the window.
Assignee | ||
Comment 5•11 years ago
|
||
Attachment #690948 -
Attachment is obsolete: true
Attachment #690948 -
Flags: review?(jwalker)
Attachment #691261 -
Flags: review?(jwalker)
Assignee | ||
Comment 6•11 years ago
|
||
This version of the patch also fixes bug 818451. And we never close the toolbox if in a window.
Assignee | ||
Updated•11 years ago
|
Priority: -- → P2
Whiteboard: [has-patch]
Comment 7•11 years ago
|
||
Comment on attachment 691261 [details] [diff] [review] patch v2 Review of attachment 691261 [details] [diff] [review]: ----------------------------------------------------------------- We're going to need to rebase this now the api review has landed, and I think selectToolCommand belongs in gDevToolsBrowser now. Otherwise r+
Attachment #691261 -
Flags: review?(jwalker) → review+
Comment 8•11 years ago
|
||
Comment on attachment 691261 [details] [diff] [review] patch v2 Review of attachment 691261 [details] [diff] [review]: ----------------------------------------------------------------- Actually thinking some more - why not make showToolbox() call toolbox.raise() ? That seems simpler, and I can't think of a time when you wouldn't want showToolbox to raise the toolbox.
Assignee | ||
Updated•11 years ago
|
Priority: P2 → P1
Assignee | ||
Comment 9•11 years ago
|
||
The refactoring actually fixed this bug. But this patch still includes some improvements: - raise the toolbox - don't close if host is window I also addressed Joe's comment.
Attachment #691261 -
Attachment is obsolete: true
Attachment #697886 -
Flags: review?(jwalker)
Assignee | ||
Updated•11 years ago
|
Whiteboard: [has-patch] → [needs-review]
Assignee | ||
Comment 10•11 years ago
|
||
review ping
Comment 11•11 years ago
|
||
Comment on attachment 697886 [details] [diff] [review] v3 Review of attachment 697886 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/devtools/framework/gDevTools.jsm @@ +145,5 @@ > let promise = (hostType != null && toolbox.hostType != hostType) ? > toolbox.switchHost(hostType) : > Promise.resolve(null); > > + toolbox.raise(); Perhaps we should do this lower down: return promise.then(function() { toolbox.raise(); return toolbox; }); Which would mean that we would only draw attention to the toolbox when everything was selected properly. @@ +269,5 @@ > + * and the host is NOT a window, we close the toolbox > + * - if the toolbox is open, and the targetted tool is selected, > + * and the host is a window, we raise the toolbox window > + */ > + selectToolCommand: function(gBrowser, toolId) { How is this required over the old toggleToolboxCommand ?
Attachment #697886 -
Flags: review?(jwalker)
Assignee | ||
Comment 12•11 years ago
|
||
> @@ +269,5 @@ > > + * and the host is NOT a window, we close the toolbox > > + * - if the toolbox is open, and the targetted tool is selected, > > + * and the host is a window, we raise the toolbox window > > + */ > > + selectToolCommand: function(gBrowser, toolId) { > > How is this required over the old toggleToolboxCommand ? So I'm not against merging the 2 commands, but they have 2 different goals and behavior: > toggleToolboxCommand Toggle the toolbox. Used in the toolbar and the menu. If the toolbox is open, we close it. If the toolbox is closed, we open it. > selectToolCommand Open or select a tool if no open yet. Close toolbox otherwise if not a window. I could merge these 2 functions into one and assume that if toolId is null, we're assuming that we want the behavior of toggleToolboxCommand. But that sounds a little too "magic" too me.
Comment 13•11 years ago
|
||
So Victor's scenario in comment 1 - If we're undocked, wouldn't we want the same behaviour as for the docked case? I'm not sure why we need the docked case to be special. If I'm missing something obvious, then r+ to Paul's patch with only the toolbox.raise(); move. If we would want the same behaviour in both cases, then r+ anyway because this is still an improvement, but I think we should file a follow-up.
Updated•11 years ago
|
Attachment #697886 -
Flags: review+
Assignee | ||
Comment 14•11 years ago
|
||
(In reply to Joe Walker [:joe_walker] [:jwalker] from comment #13) > So Victor's scenario in comment 1 - If we're undocked, wouldn't we want the > same behaviour as for the docked case? > > I'm not sure why we need the docked case to be special. If I'm missing > something obvious, then r+ to Paul's patch with only the toolbox.raise(); > move. > > If we would want the same behaviour in both cases, then r+ anyway because > this is still an improvement, but I think we should file a follow-up. The behavior is: - if docked - if tool is selected, we close the toolbox - if tool is not selected, we select the tool That lets us quickly bring the console, type something, press the same keybinding, and close it. For example: ctrl-shift-k, look at the logs/errors, ctrl-shift-k, close the console. - if undocked, we just raise the toolbox, we never close it. It would be weird to close the window in this case as the toolbox is not focused.
Assignee | ||
Comment 15•11 years ago
|
||
Comment 16•11 years ago
|
||
I'm still not clear what is the essential difference between docked and undocked windows, that means we need different behaviour on key presses. Don't many of our tools have a 3 state thing just like the developer command line: closed v ^ open/unfocused -> open/focused If you do close a window by mistake, then it's easy to get it back again - your fingers are perfectly placed.
Assignee | ||
Comment 17•11 years ago
|
||
The command is called from the browser, so if the toolbox is undocked, it is unfocused. Always. So unfocused -> focused.
Assignee | ||
Comment 18•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=0546a03ce6e5
Whiteboard: [needs-review] → to land once try is green
Assignee | ||
Updated•11 years ago
|
Whiteboard: to land once try is green → [land-in-fx-team]
Assignee | ||
Comment 19•11 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/90373967426c This will need an uplift to Aurora.
Assignee | ||
Updated•11 years ago
|
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
Comment 20•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/90373967426c
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 21
Assignee | ||
Comment 21•11 years ago
|
||
Comment on attachment 698692 [details] [diff] [review] patch v3.1 [Approval Request Comment] Bug caused by (feature/regressing bug #): new feature User impact if declined: wrong keybinding behavior. The toolbox is a new feature, we want to get the keybinding right when it's released. Testing completed (on m-c, etc.): m-c Risk to taking this patch (and alternatives if risky): low String or UUID changes made by this patch: no
Attachment #698692 -
Flags: approval-mozilla-aurora?
Updated•11 years ago
|
Attachment #698692 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Assignee | ||
Updated•11 years ago
|
Whiteboard: [land-in-aurora]
Assignee | ||
Comment 22•11 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/1059001a53f5
status-firefox20:
--- → fixed
Whiteboard: [land-in-aurora]
Comment 23•11 years ago
|
||
Based on https://bugzilla.mozilla.org/show_bug.cgi?id=815239#c1 . Verified fixed on Firefox 20 Beta 6 – 20130320062118: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:20.0) Gecko/20100101 Firefox/20.0 Not sure about verifying the description 1. Opened the Inspector (cmd-alt-I) 2. Selected an element in the inspector (the inspector is locked) 3. Hit cmd-alt-I again (nothing happens). 4. Click in the page 5. Hit cmd-alt-I the toolbox closes / Hit cmd-alt-s the toolbox switch to debugger Summary: The inspect mode is not "re-enabled" if locked, but the toolbox closes if, at least, I click in the page. If the inspect mode is locked and I click in the page the result is a switch to debugger and the toolbox doesn't close.
Comment 24•11 years ago
|
||
Following STR from Comment 0, on FF 20b6 on Mac OS 10.8, at step 3 Devtools panel is still closing. If you're looking to fix the issue as it's described on Comment 0, this is not fixed yet. In addition STR from Comment 1 work as expected, if you open Debugger (cmd + alt + s) switch on Inspector and press again (cmd + alt + S) it switches back to Debugger. Same for Inspector (cmd + alt + I) switch on Debugger and press again (cmd + shift I)
Comment 25•11 years ago
|
||
Verified as fixed on: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:20.0) Gecko/20100101 Firefox/20.0 Comment 1 was fixed - you can switch between all the tools in the toolbox by using keyword shortcuts. Considering comment 3, comment 0 was by design, so it won't be fixed in this bug.
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•