1172500 - Keyboard shortcut to toggle the bottom-docked host minimize mode

Status: RESOLVED FIXED

(DevTools :: Framework, defect)

Description

[Patrick Brosset <:pbro>]

Bug 867838 brings the ability to minimize the bottom-docked toolbox host.
We should add a keyboard shortcut for this to be more useful.

Comment 1

[Patrick Brosset <:pbro>]

Attachment: MozReview Request: Bug 1172500 - Adds a keyboard shortcut to minimize the toolbox; r=jryans

Bug 1172500 - Adds a keyboard shortcut to minimize the toolbox; r=jryans

Comment 2

[Patrick Brosset <:pbro>]

On top of the fix, the patch contains some minor eslint-related cleanups.

Comment 3

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8617296 [details]
MozReview Request: Bug 1172500 - Adds a keyboard shortcut to minimize the toolbox; r=jryans

https://reviewboard.mozilla.org/r/10607/#review9309

Ctrl-Shift-M is the shortcut for Responsive Design[1] on Windows / Linux.

I'm afraid you'll have to roll your shortcut dice again.

[1]: https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser-sets.inc#300-306

::: browser/devtools/framework/toolbox.js:35
(Diff revision 1)
> -loader.lazyGetter(this, "Hosts", () => require("devtools/framework/toolbox-hosts").Hosts);
> +loader.lazyGetter(this, "Hosts", () => {

Nit: You could use lazyRequireGetter:

loader.lazyRequireGetter(this, "Hosts", "devtools/framework/toolbox-hosts", true);

Same for more lines below.  I know you are just cleaning up for the linter so... up to you! :)

::: browser/devtools/framework/toolbox.js:758
(Diff revision 1)
> +           (osString == "Darwin" ? "Cmd+Shift+" : "Ctrl+Shift+") +

Is this really the only place we do this shortcut labelling?  Maybe it should be a shared utility function?

Comment 4

[Patrick Brosset <:pbro>]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #3)
> Comment on attachment 8617296 [details]
> MozReview Request: Bug 1172500 - Adds a keyboard shortcut to minimize the
> toolbox; r=jryans
> 
> https://reviewboard.mozilla.org/r/10607/#review9309
> 
> Ctrl-Shift-M is the shortcut for Responsive Design[1] on Windows / Linux.
> 
> I'm afraid you'll have to roll your shortcut dice again.
> 
> [1]:
> https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser-
> sets.inc#300-306
Damn ... What's the logic behind some of our shortcuts that use accel+shift on mac and accel+alt on PC? Why not always the same?
> ::: browser/devtools/framework/toolbox.js:35
> (Diff revision 1)
> > -loader.lazyGetter(this, "Hosts", () => require("devtools/framework/toolbox-hosts").Hosts);
> > +loader.lazyGetter(this, "Hosts", () => {
> 
> Nit: You could use lazyRequireGetter:
> 
> loader.lazyRequireGetter(this, "Hosts", "devtools/framework/toolbox-hosts",
> true);
> 
> Same for more lines below.  I know you are just cleaning up for the linter
> so... up to you! :)
Cleaned this up, thanks for the tip, I had never really looked into how Loader.jsm worked, that was a good opportunity for doing this.
> ::: browser/devtools/framework/toolbox.js:758
> (Diff revision 1)
> > +           (osString == "Darwin" ? "Cmd+Shift+" : "Ctrl+Shift+") +
> 
> Is this really the only place we do this shortcut labelling?  Maybe it
> should be a shared utility function?
When I wrote the patch, yeah it was (or close), now a change to definition.js has landed that introduced a whole bunch of new shortcut labelling, so we could do better. I'll file a bug for this.

Comment 5

[Patrick Brosset <:pbro>]

Comment on attachment 8617296 [details]
MozReview Request: Bug 1172500 - Adds a keyboard shortcut to minimize the toolbox; r=jryans

Bug 1172500 - Adds a keyboard shortcut to minimize the toolbox; r=jryans

Comment 6

[Patrick Brosset <:pbro>]

https://reviewboard.mozilla.org/r/10607/#review9371

> Is this really the only place we do this shortcut labelling?  Maybe it should be a shared utility function?

See bug 1173291

Comment 7

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Patrick Brosset [:pbrosset] [:patrick] [:pbro] from comment #4)
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #3)
> > Comment on attachment 8617296 [details]
> > MozReview Request: Bug 1172500 - Adds a keyboard shortcut to minimize the
> > toolbox; r=jryans
> > 
> > https://reviewboard.mozilla.org/r/10607/#review9309
> > 
> > Ctrl-Shift-M is the shortcut for Responsive Design[1] on Windows / Linux.
> > 
> > I'm afraid you'll have to roll your shortcut dice again.
> > 
> > [1]:
> > https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser-
> > sets.inc#300-306
> Damn ... What's the logic behind some of our shortcuts that use accel+shift
> on mac and accel+alt on PC? Why not always the same?

No idea why it's like that really... :/

Comment 8

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8617296 [details]
MozReview Request: Bug 1172500 - Adds a keyboard shortcut to minimize the toolbox; r=jryans

https://reviewboard.mozilla.org/r/10607/#review9405

This shortcut appears available on all OSes.  Can you post a try run so builds can be checked to be sure?

::: browser/devtools/framework/toolbox.js:37
(Diff revisions 1 - 2)
> +               "chrome://browser/locale/devtools/toolbox.properties");

Nit: This is kind of unusual wrapping...  Maybe put this path in a const var and use it here instead?

Comment 9

[Patrick Brosset <:pbro>]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #8)
> This shortcut appears available on all OSes.  Can you post a try run so
> builds can be checked to be sure?
Yes sure, I actually already ran 2 try builds, but didn't post them because some tests were failing on linux. I'm currently investigating and will post a fix and new try build very shortly.

Comment 10

[Patrick Brosset <:pbro>]

I went nuts this morning trying to figure out a shortcut to use.
Turns out accel+shift+- didn't work on linux. I tested a whole lot of them only to find that a lot of them just don't work even if they're not taken already.
I settled for accel+shift+U which works everywhere and isn't taken.
It seems to work nicely: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c8f4e965055

Comment 11

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Patrick Brosset [:pbrosset] [:patrick] [:pbro] from comment #10)
> I went nuts this morning trying to figure out a shortcut to use.
> Turns out accel+shift+- didn't work on linux. I tested a whole lot of them
> only to find that a lot of them just don't work even if they're not taken
> already.
> I settled for accel+shift+U which works everywhere and isn't taken.
> It seems to work nicely:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c8f4e965055

Okay, if you've tested all 3 OSes, then it sounds good to me!

Comment 12

[Patrick Brosset <:pbro>]

Attachment: bug1172500-minimize-shortcut-rebased.diff

Last changes as discussed earlier, and rebased on top of latest fx-team.
Try is green.
Ready to be checked-in.

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/15f05333624f

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/15f05333624f