Closed Bug 1119133 Opened 5 years ago Closed 5 years ago

Keyboard shortcut to toggle devtools docking mode between last two positions

Categories

(DevTools :: General, defect, P2)

defect

Tracking

(firefox41 fixed)

VERIFIED FIXED
Firefox 41
Tracking Status
firefox41 --- fixed

People

(Reporter: pbro, Assigned: bgrins, Mentored)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [polish-backlog][difficulty=easy][good next bug])

Attachments

(3 files, 5 obsolete files)

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.
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?
Flags: needinfo?(bgrinstead)
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'.
Flags: needinfo?(bgrinstead)
Summary: Keyboard shortcut to dock devtools toolbox to the right and back → Keyboard shortcut to toggle devtools docking mode between last two positions
Mentor: bgrinstead
Whiteboard: [good next bug]
Attached patch toggle.patch (obsolete) — Splinter Review
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 :)
Assignee: nobody → lviknesh
Status: NEW → ASSIGNED
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.
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
Attached patch toggle-Host.patch (obsolete) — Splinter Review
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 :)
Attachment #8558562 - Flags: feedback?(bgrinstead)
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.
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.
Depends on: 1022726
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
Attachment #8558562 - Flags: feedback?(bgrinstead) → feedback+
Whiteboard: [good next bug] → [devedition-40][difficulty=easy][good next bug]
Priority: -- → P2
Attached patch toggle-host.patch (obsolete) — Splinter Review
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?
Attachment #8557661 - Attachment is obsolete: true
Attachment #8558562 - Attachment is obsolete: true
Flags: needinfo?(lviknesh)
Hi Brian , sorry for replying late , i hope you can take over the bug . Thank you :)
Flags: needinfo?(lviknesh)
Attached patch toolbox-switch-hosts.patch (obsolete) — Splinter Review
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
Assignee: lviknesh → bgrinstead
Attachment #8607746 - Attachment is obsolete: true
Attachment #8616253 - Flags: review?(pbrosset)
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.
Attachment #8616253 - Flags: review?(pbrosset) → review+
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&)]
Attached patch toolbox-switch-hosts.patch (obsolete) — Splinter Review
Addresses review comments.  Still has a crash on try for windows and osx debug builds, so not ready to land
Attachment #8616253 - Attachment is obsolete: true
Attached file 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
Flags: needinfo?(masayuki)
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
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.
Flags: needinfo?(masayuki)
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.
Attachment #8617853 - Flags: review?(bugs)
Attachment #8617853 - Flags: review?(bugs) → review+
(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.
Whiteboard: [devedition-40][difficulty=easy][good next bug] → [devedition-40][difficulty=easy][good next bug][leave open]
(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!
Whiteboard: [devedition-40][difficulty=easy][good next bug][leave open] → [devedition-40][difficulty=easy][good next bug]
remote:   https://hg.mozilla.org/integration/fx-team/rev/547e848ba27a
Whiteboard: [devedition-40][difficulty=easy][good next bug] → [devedition-40][difficulty=easy][good next bug][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/547e848ba27a
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [devedition-40][difficulty=easy][good next bug][fixed-in-fx-team] → [devedition-40][difficulty=easy][good next bug]
Target Milestone: --- → Firefox 41
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.
Flags: needinfo?(bgrinstead)
(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?
Flags: needinfo?(bgrinstead)
(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.
Whiteboard: [devedition-40][difficulty=easy][good next bug] → [polish-backlog][difficulty=easy][good next bug]
QA Whiteboard: [good first verify]
I just tested and it works on mine.
(In reply to jonathanshafer3 from comment #31)
> I just tested and it works on mine.

Alright, thanks!  Marking as verified
Status: RESOLVED → VERIFIED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.