1119133 - Keyboard shortcut to toggle devtools docking mode between last two positions

Status: VERIFIED FIXED

(DevTools :: General, defect, P2)

Description

[Patrick Brosset <:pbro>]

A keyboard shortcut to toggle the dock mode of the toolbox would be nice.

Ctrl/Cmd + Shift + D was just added to Chrome canary: https://twitter.com/addyosmani/status/552849728983924737

I suggest doing exactly the same.

Comment 1

[Patrick Brosset <:pbro>]

This bug could be a good candidate for a "good first bug". I'm not sure who could be a mentor for this one, but I'll give some high level guidelines here:

- First things first: dev environment and building: everything you need to know is here: https://wiki.mozilla.org/DevTools/Hacking.
- Getting in touch: the best way to get help is to join our IRC channel #devtools (other info here: https://wiki.mozilla.org/DevTools/GetInvolved).
- The code for this is inside /browser/devtools/, which means the dev flow will be like:
  - change something in this folder
  - build with './mach build browser/devtools'
  - run the browser './mach run'
- More specifically, the code that switches the dock mode is here: http://mxr.mozilla.org/mozilla-central/source/browser/devtools/framework/toolbox.js#1408
- We already have a set of key shortcuts defined in the toolbox XUL file: http://mxr.mozilla.org/mozilla-central/source/browser/devtools/framework/toolbox.xul#21 , I guess we should add one here.
- Then in |_addHostListeners| (which looks like a good candidate) here: http://mxr.mozilla.org/mozilla-central/source/browser/devtools/framework/toolbox.js#420 , we add a command event listener to this key and use it to call |switchHost|.

Or something along those lines.
NI? Brian as he probably is the one who knows most about this by now. Brian, you might be interested in mentoring this bug?

Comment 2

[Brian Grinstead [:bgrins]]

Also notice the behavior regarding an undocked toolbox (https://twitter.com/addyosmani/status/552859085734170624).  It seems that it will toggle between your last two docking modes, rather than just always switching between bottom and right.

So if I dock to right, then open in undocked, pressing ctrl + shift + d will toggle between right and undocked.

Because of this, we will also need a pref to keep track of the last docked mode and a more comprehensive test.  This pref can be set in the same general place as this._prefs.LAST_HOST: https://mxr.mozilla.org/mozilla-central/source/browser/devtools/framework/toolbox.js#1424

I would see this as a 'good next bug'.

Comment 3

[vikneshwar]

Attachment: toggle.patch

Hi Brian 

I tried out to make it work , but when i press Ctrl + Shift + D on my Ubuntu nothing happens . I think i am missing some logic in switchHost() . Would be great if you could point on what's going wrong  Thank you :)

Comment 4

[Brian Grinstead [:bgrins]]

Comment on attachment 8557661 [details] [diff] [review]
toggle.patch

Review of attachment 8557661 [details] [diff] [review]:
-----------------------------------------------------------------

I haven't applied the patch and debugged it, but I see something that looks suspicious (see below).  Can you confirm that the command event callback is firing when you do the keyboard shortcut by logging something out?

::: browser/devtools/framework/toolbox.js
@@ +426,5 @@
>  
> +    let toggleKey = this.doc.getElementById("toolbox-toggle-key");
> +    let hostType = Services.prefs.getCharPref(this._prefs.PREVIOUS_HOST);
> +    toggleKey.addEventListener("command",() => {
> +      this.switchHost(hostType);

I think you want to re-read hostType from the preference every time inside the 'command' event, since it could have changed since this function has run.

Comment 5

[Brian Grinstead [:bgrins]]

Please add a test for this functionality.  Here is a relevant test file that is doing similar things: https://dxr.mozilla.org/mozilla-central/source/browser/devtools/framework/test/browser_toolbox_hosts.js.  We should add a new test that is similar to this for this feature in the framework folder.

For another example test in the framework folder, here is a simple test using the newer testing style that can be a little easier to work with since it is using yielding instead of callbacks: https://dxr.mozilla.org/mozilla-central/source/browser/devtools/framework/test/browser_toolbox_getpanelwhenready.js

Comment 6

[vikneshwar]

Attachment: toggle-Host.patch

Hi Brian , i have problem in focusing when switching between window and bottom , bottom gets into focus but not window . And when switching between bottom and and right , i need to focus out by switching to different tab and focus again for the shortcut to work . 

I will upload the patch with the test , once i complete , Thank you :)

Comment 7

[Brian Grinstead [:bgrins]]

Seems like cmd+shift+D is already used for 'bookmark all tabs', so that is popping up after switching between bottom and right.  It's only showing up, though, because our command event isn't firing in this case until unfocusing/refocusing the browser window.  I've actually been able to trigger it when switching between side and undocked also, but you have to switch very quickly to make it happen.

I'm not immediately sure what is going on here.  It may have to do with the way the document is copied across iframes using iframe.swapFrameLoaders.

Please proceed with the test and we'll figure out what is going on with this before landing.

Comment 8

[Brian Grinstead [:bgrins]]

I've spent some time trying to track down the issue, and I believe it's the same problem as Bug 1022726.  Haven't been able to actually figure out what the solution is yet, though.

Comment 9

[Brian Grinstead [:bgrins]]

Comment on attachment 8558562 [details] [diff] [review]
toggle-Host.patch

Review of attachment 8558562 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the delay.  Getting this to work properly after switching hosts is blocked by Bug 1022726, but since this focus problem affects all keyboard shortcuts I think we can proceed with this anyway if we get a test in place.

::: browser/devtools/framework/toolbox.js
@@ +1432,2 @@
>        this._host = newHost;
> +      

Nit: trailing whitespace

Comment 10

[Brian Grinstead [:bgrins]]

Attachment: toggle-host.patch

Rebased on top of the changes in Bug 1022726, which unblocks this work.  vikneshwar, are you able to write a test for this to get it landed or should I take it over?

Comment 11

[vikneshwar]

Hi Brian , sorry for replying late , i hope you can take over the bug . Thank you :)

Comment 12

[Brian Grinstead [:bgrins]]

Attachment: toolbox-switch-hosts.patch

Actually got into a fair amount of test refactoring to get this how I wanted.  The test in browser_toolbox_hosts.js covers various corner cases of the new switchToPreviousHost function, while the new browser_keybindings_03.js covers some basic cases using the keyboard shortcuts.

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2846f0b22b47

Comment 13

[Patrick Brosset <:pbro>]

Comment on attachment 8616253 [details] [diff] [review]
toolbox-switch-hosts.patch

Review of attachment 8616253 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great to me!
And the toolbox still gains focus after a host switch, this means we can now switch the host and use the toolbox all from the keyboard, that's nice.
Just a few minor comments below.

::: browser/devtools/framework/test/browser_keybindings_03.js
@@ +6,5 @@
> +// Test that the toolbox 'switch to previous host' feature works.
> +// Pressing ctrl/cmd+shift+d should switch to the last used host.
> +
> +const URL = "data:text/html;charset=utf8,test page for toolbox switching";
> +let toolbox;

I suggest either nullifying toolbox at the end of the test, or not declaring it in the global scope at all and instead pass it as a parameter to checkHostType.

::: browser/devtools/framework/test/browser_toolbox_hosts.js
@@ +1,4 @@
>  /* vim: set ts=2 et sw=2 tw=80: */
>  /* Any copyright is dedicated to the Public Domain.
>     http://creativecommons.org/publicdomain/zero/1.0/ */
>  

While you're refactoring this test heavily, can you add "use strict"; at the top?

@@ +90,5 @@
> +function* testPreviousHost() {
> +  // last host was the window - make sure it's the same when re-opening
> +  is(toolbox.hostType, WINDOW, "host remembered");
> +
> +  info ("Switching to side");

nit: I think the prevailing formatting style for info/is/ok/... is not to have a space between the function name and the parens.

@@ +128,5 @@
> +  yield toolbox.switchToPreviousHost();
> +  checkHostType(BOTTOM, SIDE);
> +}
> +
> +function checkHostType(hostType, previousHostType) {

This is the same checkHostType function that's in the other test, right? It seems generic enough to be moved to head.js.

::: browser/devtools/framework/test/head.js
@@ +109,5 @@
>      return promise.resolve();
>    }
>  }
> +
> +

nit: just 1 empty line to separate functions is enough.

@@ +119,5 @@
> +function synthesizeKeyElement(el) {
> +  let key = el.getAttribute("key") || el.getAttribute("keycode");
> +  let mod = {};
> +  el.getAttribute("modifiers").split(" ").forEach((m) => mod[m+"Key"] = true);
> +  info("Synthesizing: key="+key+", mod="+JSON.stringify(mod));

nit: please space around + string concatenations.

::: browser/devtools/framework/toolbox.js
@@ +1483,5 @@
>      return newHost;
>    },
>  
>    /**
> +   * Loads the tool just left to the currently selected tool.

Looks like a comment copied from another function.

::: browser/devtools/framework/toolbox.xul
@@ +69,5 @@
>      <key id="toolbox-force-reload-key2"
>           keycode="VK_F5"
>           oncommand="void(0);"
>           modifiers="accel"/>
> +    <key id="toolbox-toggle-key"

Shouldn't this be named toolbox-toggle-host-key instead? toolbox-toggle makes me think this is to show/hide the toolbox.

Comment 14

[Brian Grinstead [:bgrins]]

Looks like this is somehow causing a crash: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6b330dd3622a.

TEST-UNEXPECTED-FAIL | browser/devtools/framework/test/browser_keybindings_03.js | application terminated with exit code 1
PROCESS-CRASH | browser/devtools/framework/test/browser_keybindings_03.js | application crashed [@ mozilla::TextInputProcessor::NotifyIME(mozilla::widget::TextEventDispatcher*, mozilla::widget::IMENotification const&)]

Comment 15

[Brian Grinstead [:bgrins]]

Attachment: toolbox-switch-hosts.patch

Addresses review comments.  Still has a crash on try for windows and osx debug builds, so not ready to land

Comment 16

[Brian Grinstead [:bgrins]]

Attachment: crash.txt

Hi Masayuki, looking at the history of TextInputProcessor.cpp I think you might be the right person to help figure out what's causing a crash triggered by a work in progress patch for devtools.

I have a try push [0] that is crashing on Windows and OSX debug builds with the message 'application crashed [@mozilla::TextInputProcessor::NotifyIME(mozilla::widget::TextEventDispatcher*, mozilla::widget::IMENotification const&)]'.

I've been able to reproduce this crash in the test in a debug build, but am not sure what's causing this.  Do you have any insights that could help me figure out what's going on?

For background, what this patch is doing is adding a keyboard shortcut to switch the devtools window between two different docking positions via toolbox.switchHost [1].  One quirk with this is that we are calling iframe.swapFrameLoaders to keep the document state when it switches between frames.

[0]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6b330dd3622a
[1]: https://dxr.mozilla.org/mozilla-central/source/browser/devtools/framework/toolbox.js#1555

Comment 17

[Brian Grinstead [:bgrins]]

Also, not sure if this is related but I should also mention that in Bug 1022726 we landed a fix which was kind of strange.  We call this.frame.contentWindow.focus() twice, once after the call to swapFrameLoaders and once after the keys have been added to the window.  Otherwise you would have to refocus the window manually for key events to work.  Maybe this is something that could be fixed at the platform level instead.

https://dxr.mozilla.org/mozilla-central/source/browser/devtools/framework/toolbox.js#1571
https://dxr.mozilla.org/mozilla-central/source/browser/devtools/framework/toolbox.js#1587

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

It seems that a TextInputProcessor instance is reused with different TextEventDispatcher. I'm not sure why this occurs with EventUtils.js. However, looks like this is a bug of them. I'll post a patch to this bug.

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Implement TextEventDispatcher::EndInputTransaction() for ensuring TextEventDispatcher forgets the link with TextInputProcessor

Still testing...
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f241024cbafa

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8617853 [details] [diff] [review]
Implement TextEventDispatcher::EndInputTransaction() for ensuring TextEventDispatcher forgets the link with TextInputProcessor

TextEventDispatcher holds TextInputProcessor as a weak reference. If TIP is destroyed, its reference becomes null. However, if it's not gone by the TextEventDispatcher receives a notification to IME, TextEventDispatcher may notify the unlinked TextInputProcessor of the received notification.

So, if TIP begins new input transaction with different TextEventDispatcher, it should notify the linking TextEventDispatcher of end input transaction explicitly.

Comment 21

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/76a74dc6ccc9

Comment 22

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Pulsebot from comment #21)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/76a74dc6ccc9

I pushed the fix of the cause of the oranges.

Comment 23

[Brian Grinstead [:bgrins]]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #22)
> (In reply to Pulsebot from comment #21)
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/76a74dc6ccc9
> 
> I pushed the fix of the cause of the oranges.

Thanks for fixing this!

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/76a74dc6ccc9

Comment 25

[Brian Grinstead [:bgrins]]

Attachment: toolbox-switch-hosts.patch

Rebased.  New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9d0bded80f99

Comment 26

[Brian Grinstead [:bgrins]]

remote:   https://hg.mozilla.org/integration/fx-team/rev/547e848ba27a

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/547e848ba27a

Comment 28

[Will Bamberg [:wbamberg]]

I've added this here: https://developer.mozilla.org/en-US/docs/Tools/Keyboard_shortcuts#Toolbox
...and made it so keyboard shortcuts show up in the Toolbox page: https://developer.mozilla.org/en-US/docs/Tools/Tools_Toolbox#Keyboard_shortcuts.

Comment 29

[Brian Grinstead [:bgrins]]

(In reply to Will Bamberg [:wbamberg] from comment #28)
> I've added this here:
> https://developer.mozilla.org/en-US/docs/Tools/Keyboard_shortcuts#Toolbox

Nice, thanks!

> ...and made it so keyboard shortcuts show up in the Toolbox page:
> https://developer.mozilla.org/en-US/docs/Tools/
> Tools_Toolbox#Keyboard_shortcuts.

Is that dynamically loading the same table or do we need to keep them in sync manually?

Comment 30

[Will Bamberg [:wbamberg]]

(In reply to Brian Grinstead [:bgrins] from comment #29)
> (In reply to Will Bamberg [:wbamberg] from comment #28)
> > I've added this here:
> > https://developer.mozilla.org/en-US/docs/Tools/Keyboard_shortcuts#Toolbox
> 
> Nice, thanks!
> 
> > ...and made it so keyboard shortcuts show up in the Toolbox page:
> > https://developer.mozilla.org/en-US/docs/Tools/
> > Tools_Toolbox#Keyboard_shortcuts.
> 
> Is that dynamically loading the same table or do we need to keep them in
> sync manually?

All the "keyboard shortcut" sections load their content from the main document at https://developer.mozilla.org/en-US/docs/Tools/Keyboard_shortcuts.

Comment 31

[jonathanshafer3]

I just tested and it works on mine.

Comment 32

[Brian Grinstead [:bgrins]]

(In reply to jonathanshafer3 from comment #31)
> I just tested and it works on mine.

Alright, thanks!  Marking as verified