Closed Bug 815239 Opened 7 years ago Closed 7 years ago

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

Categories

(DevTools :: Inspector, defect, P1)

x86
macOS
defect

Tracking

(firefox20 verified)

RESOLVED FIXED
Firefox 21
Tracking Status
firefox20 --- verified

People

(Reporter: rcampbell, Assigned: paul)

References

Details

Attachments

(2 files, 2 obsolete files)

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.
Summary: Inspect Shortcut closes Devtools → [toolbox] Inspect Shortcut closes Devtools
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.
Blocks: 816946
No longer blocks: 788977
Summary: [toolbox] Inspect Shortcut closes Devtools → [toolbox] Using a tool keyboard shortcut twice closes the toolbox. It should select it instead.
Blocks: 818451
Depends on: 813031
Attached patch patch v1 (obsolete) — Splinter Review
Assignee: nobody → paul
Status: NEW → ASSIGNED
Attachment #690948 - Flags: review?(jwalker)
+   * 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.
Duplicate of this bug: 818451
Attached patch patch v2 (obsolete) — Splinter Review
Attachment #690948 - Attachment is obsolete: true
Attachment #690948 - Flags: review?(jwalker)
Attachment #691261 - Flags: review?(jwalker)
This version of the patch also fixes bug 818451. And we never close the toolbox if in a window.
Priority: -- → P2
Whiteboard: [has-patch]
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 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.
Priority: P2 → P1
Attached patch v3Splinter Review
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)
Whiteboard: [has-patch] → [needs-review]
review ping
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)
> @@ +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.
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.
(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.
Attached patch patch v3.1Splinter Review
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.
The command is called from the browser, so if the toolbox is undocked, it is unfocused. Always. So unfocused -> focused.
https://tbpl.mozilla.org/?tree=Try&rev=0546a03ce6e5
Whiteboard: [needs-review] → to land once try is green
Whiteboard: to land once try is green → [land-in-fx-team]
https://hg.mozilla.org/integration/fx-team/rev/90373967426c

This will need an uplift to Aurora.
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/90373967426c
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 21
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?
Attachment #698692 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Whiteboard: [land-in-aurora]
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.
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)
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.
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.