815239 - [toolbox] Using a tool keyboard shortcut twice closes the toolbox. It should select it instead.

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P1)

Description

[Rob Campbell [:rc] (:robcee)]

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.

Comment 1

[Victor Porof [:vporof][:vp]]

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.

Comment 2

[Paul Rouget [:paul]]

Attachment: patch v1

Comment 3

[Paul Rouget [:paul]]

+   * 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.

Comment 5

[Paul Rouget [:paul]]

Attachment: patch v2

Comment 6

[Paul Rouget [:paul]]

This version of the patch also fixes bug 818451. And we never close the toolbox if in a window.

Comment 7

[Joe Walker [:jwalker]]

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+

Comment 8

[Joe Walker [:jwalker]]

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.

Comment 9

[Paul Rouget [:paul]]

Attachment: v3

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.

Comment 10

[Paul Rouget [:paul]]

review ping

Comment 11

[Joe Walker [:jwalker]]

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 ?

Comment 12

[Paul Rouget [:paul]]

> @@ +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

[Joe Walker [:jwalker]]

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.

Comment 14

[Paul Rouget [:paul]]

(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.

Comment 15

[Paul Rouget [:paul]]

Attachment: patch v3.1

Comment 16

[Joe Walker [:jwalker]]

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.

Comment 17

[Paul Rouget [:paul]]

The command is called from the browser, so if the toolbox is undocked, it is unfocused. Always. So unfocused -> focused.

Comment 18

[Paul Rouget [:paul]]

https://tbpl.mozilla.org/?tree=Try&rev=0546a03ce6e5

Comment 19

[Paul Rouget [:paul]]

https://hg.mozilla.org/integration/fx-team/rev/90373967426c

This will need an uplift to Aurora.

Comment 20

[Panos Astithas (he/him) [:past] (please ni?)]

https://hg.mozilla.org/mozilla-central/rev/90373967426c

Comment 21

[Paul Rouget [:paul]]

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

Comment 22

[Paul Rouget [:paul]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1059001a53f5

Comment 23

[Tiziana Sellitto [:tiziana]]

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

[Mihai Morar, (:MihaiMorar)]

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

[Ioana (away)]

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.