Closed Bug 999568 Opened 10 years ago Closed 9 years ago

Detached devtools should be brought to the foreground if F12 is pressed within the browser

Categories

(DevTools :: Framework, defect)

29 Branch
defect
Not set
normal

Tracking

(firefox40 fixed)

RESOLVED FIXED
Firefox 40
Tracking Status
firefox40 --- fixed

People

(Reporter: Samuel.Hodge, Assigned: julian.descottes, Mentored)

References

Details

(Keywords: polish)

Attachments

(1 file, 2 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0 (Beta/Release)
Build ID: 20140417185217

Steps to reproduce:

1. Open the devtools and detach them from the browser window
2. While focused on the browser window, press F12


Actual results:

The devtools are closed


Expected results:

The devtools should be brought to the foreground.

Perhaps closing the devtools makes sense if it is the currently active window, but if the browser is the active window, it's convenient to bring the devtools to the front and focus on them.

Google Chrome has the expected behavior.
Component: Untriaged → Developer Tools
+1 for this!
Status: UNCONFIRMED → NEW
Component: Developer Tools → Developer Tools: Framework
Ever confirmed: true
Keywords: polish
OS: Windows 8.1 → All
Hardware: x86_64 → All
I came here to file a bug report on this, so instead I would like to express my support for this ticket.
It looks like the toggleToolboxCommand function here [1] should first be checking the current ToolboxHost type [2] and if a WindowHost, should check if it's in the background or not.
I'm not sure yet how to check that a window is in the background, but that shouldn't be too hard. There might even be doc about it on MDN.

[1] http://mxr.mozilla.org/mozilla-central/source/browser/devtools/framework/gDevTools.jsm#562
[2] http://mxr.mozilla.org/mozilla-central/source/browser/devtools/framework/toolbox-hosts.js#28
Mentor: pbrosset
Here's another related problem: F12 with the detached toolbox window in the foreground doesn't do anything, it should close the toolbox.
I would like to start working on this issue. 

@Patrick : Can you assign it to me ?
Thanks! Assigned now.
If you haven't done so yet, make sure to go over our getting started guide: https://wiki.mozilla.org/DevTools/GetInvolved#Hacking (in particular make sure to clone fx-team as explained in the doc).
Once done, I believe the info in comment 3 should help you get started.
(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #4)
> Here's another related problem: F12 with the detached toolbox window in the
> foreground doesn't do anything, it should close the toolbox.
This seems to be tracked in bug 907961.
Assignee: nobody → julian.descottes
Status: NEW → ASSIGNED
Attached patch bug999568.patch (obsolete) — Splinter Review
Here is a first patch proposal for this bug.

@Patrick : Can you review and let me know your comments ?

Regarding the patch itself : should we apply the same logic to the devtools' own devtools (toggled with toggleBrowserToolboxCommand) ?

Regarding the tests : Only added a mochitest. Since they take quite some time to run, I am not sure how thorough I should be. For now the test only covers the behaviour when using the devtools in a separate window.

Thanks
Attachment #8597721 - Flags: review?(pbrosset)
As discussed in bug 907961 comment 4, we decided to solve both bugs here.
So, this means we also need to solve this problem here:

> F12 in detached devtools window currently does nothing.
> It should follow the behavior of the docked devtools and close itself.
@Patrick : In reply to your message in bug 907961 :

> The only thing is that there's a second way to toggle the devtools:
> alt-cmd-I (on Mac, and whatever is equivalent on Windows).
> See: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-sets.inc#311

This key is bound to the command "Tools:DevToolbox", same command is used for F12.

See 
- http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-sets.inc#29
- http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-sets.inc#98

With this patch, when used in a browser window, CTRL+SHIFT+I/CMD+ALT+I should be equivalent to F12 (disclaimer : I didn't test this yet).
When used in the devtools, it will not do anything (ie. same as current behavior).

You think it should close the devtools window in this case as well ? 
Will be the same as F12 in all situations then ?
Flags: needinfo?(pbrosset)
Comment on attachment 8597721 [details] [diff] [review]
bug999568.patch

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

Looking great.
Only F+ for now, since you said you wanted to merge this with bug 907961.
Up to you if you want to keep separated patches (one patch for this fix, one other for the fix to bug 907961, and one for all tests), or just one patch for all, your choice.

::: browser/devtools/framework/gDevTools.jsm
@@ +571,5 @@
>    toggleToolboxCommand: function(gBrowser) {
>      let target = devtools.TargetFactory.forTab(gBrowser.selectedTab);
>      let toolbox = gDevTools.getToolbox(target);
>  
> +    let isDocked = toolbox && toolbox.hostType != devtools.Toolbox.HostType.WINDOW;

Could you add a very short comment here (or maybe in the function jsdoc block above) that explains that we don't always toggle per say (we don't just cycle between show and hide) in the case of a window host, and we instead bring to front.

::: browser/devtools/framework/test/browser_toolbox_toggle.js
@@ +8,5 @@
> +let DevTools = temp.DevTools;
> +let gDevToolsBrowser = temp.gDevToolsBrowser;
> +
> +Cu.import("resource://gre/modules/devtools/Loader.jsm", temp);
> +let devtools = temp.devtools;

I think the last 6 lines can be removed without any consequences on the test.
The devtools/framework/test folder contains tests that haven't been yet cleaned up like other test folders have, and so there's a lot of inconsistent stuff around, and duplicated imports (for instance, if you look in head.js, in the same directory, you'll see that some things are already imported).
So you can get rid of everything from `let temp = {};` to `let devtools = temp.devtools`

@@ +16,5 @@
> +
> +function test() {
> +  gBrowser.selectedTab = gBrowser.addTab();
> +  let target = TargetFactory.forTab(gBrowser.selectedTab);
> +

You can also massively simplify this function by using the common helper function `addTab` from head.js (head.js is loaded first, before your test starts):

function test() {
  addTab("data:text/html;charset=utf-8,test for toggle toolbox in different hosts").then(tab => {
    gDevTools.showToolbox(TargetFactory.forTab(tab)).then...
  });
}

@@ +24,5 @@
> +             .then(testSwitchToWindowHost, console.error)
> +             .then(null, console.error);
> +  }, true);
> +
> +  content.location = "data:text/html,test for toggle toolbox in different hosts";

Please add the charset:
data:text/html;charset=utf-8,test for ...

@@ +29,5 @@
> +}
> +
> +function testSwitchToWindowHost(aToolbox) {
> +  toolbox = aToolbox;
> +  toolbox.switchHost(Toolbox.HostType.WINDOW).then(testCheckWindowHost);

I'm not going to ask you to rewrite this test using the new task-based style we've started to use elsewhere in the devtools, because none of the tests in this directory use it yet, but just for info, instead of chaining callbacks and promises handlers like this, we could rewrite the test like:

add_task(function*() {
  let tab = yield addTab("data:text....");
  let toolbox = yield gDevTools.showToolbox(TargetFactory.forTab(tab));

  yield toolbox.switchHost(Toolbox.HostType.WINDOW);
  
  let onMainWindowFocus = once(window, "focus", true); // Note that head.js contains a handy `once` helper.
  window.focus();
  yield onMainWindowFocus;

  let onToolboxFocus = once(toolbox._host._window, "focus", true);
  gDevToolsBrowser.toggleToolboxCommand(gBrowser);
  yield onToolboxFocus;
  
  ok(true ,...);

  ... etc ...
});

@@ +62,5 @@
> +function cleanup() {
> +  Services.prefs.setCharPref("devtools.toolbox.host", Toolbox.HostType.BOTTOM);
> +
> +  toolbox.destroy().then(function() {
> +    DevTools = Toolbox = toolbox = null;

You can remove `DevTools` in this nullifying statement.
Attachment #8597721 - Flags: review?(pbrosset) → feedback+
(In reply to julian.descottes from comment #11)
> You think it should close the devtools window in this case as well ? 
> Will be the same as F12 in all situations then ?
Yes I think it makes sense that these 2 different shortcuts have the exact same behavior.
Flags: needinfo?(pbrosset)
Attached patch Bug999568-2.patch (obsolete) — Splinter Review
@Patrick : Thanks a lot for the feedback !

I updated my patch based on your comments and integrated the fix for Bug 907961.

- most changes are in the test ; now relying on addTask + synthesizeEvent. It's more complete and feels more readable, let me know what you think
- devtools window can also be closed via CTRL+SHIFT+I, as discussed
Attachment #8597721 - Attachment is obsolete: true
Attachment #8599005 - Flags: feedback?(pbrosset)
Comment on attachment 8599005 [details] [diff] [review]
Bug999568-2.patch

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

Thanks Julian for the patch. I've tested it locally, it works perfectly! The keybindings behavior are much better now.
The code changes look great too, really nice work. No need for F? here, I can proceed straight to review.
And I only have one minor comment about the test.
So R+ with that fixed (which means you don't need to ask for review again next time you upload an update to the patch, just set R+ yourself).
I've pushed this patch to TRY: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ac40d5027838
Let's keep an eye on this, if everything turns green, fine, if not, we'll have to see what tests this breaks and why.

::: browser/devtools/framework/test/browser_toolbox_toggle.js
@@ +1,4 @@
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +const URL = "data:text/html;charset=utf-8,Test toggling devtools using keyboard shortcuts";
> +

This test looks great, thanks for making use of add_task!
One minor comment: when tests start to fail on remote servers (because of course they tend to fail on test servers, but not locally, to make things fun), we're only left with logs, and it helps a great bunch if the logs actually contain info about what the test is doing. One nice way to do this is by adding 'info("...")' statements throughout the test.
So, everywhere you would add a comment like:
// Now testing this ...
replace it with:
info("Now testing this...");

Basically try and instrument the main steps of the test with info statements so that it's easy to follow the test steps by reading a log file.

Otherwise perfect, and it passes fine for me locally on my mac.
Attachment #8599005 - Flags: feedback?(pbrosset) → review+
(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #15)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=ac40d5027838
So far, your patch doesn't seem to make any of the existing tests fail, good.
However, the new test seems to consistently fail on Linux (so far, it looks like it passes ok on other platforms).
The logs point to a timeout when waiting for the toolbox window host to focus.
(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #16)
> The logs point to a timeout when waiting for the toolbox window host to
> focus.

Looking at other tests, I could try with SimpleTest.waitOnFocus (used in test browser_chatwindow.js) instead of listening to the "focus" event on the window object. 

I saw some tests using timeouts (executeSoon) or polling to wait for the window focus, but this seems more a hack than anything else.

(will need to setup a Linux environment to test this, so might take a bit of time)
After asking on #developers, @markh advised to use waitForFocus in order to fix the focus issue. Hopefully will submit a new patch after that.
Now using waitForFocus on the toolbox window before trying to focus the main window.

The test now passes on Linux on my machine.

@Patrick : Could you push this on try ?

Other changes in this patch :
- moved comments to info in the test
- removed "#filter substitution" in toolbox-window.xul
Attachment #8599005 - Attachment is obsolete: true
Attachment #8600084 - Flags: review+
Thanks Julian for taking care of this!
Here's a new try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dff04fadfaa9
Thanks Patrick.

Got two orange lights on Linux & Linux Debug : Bug 1144323 & Bug 1158027.
Got one red on Linux Debug : Bug 1137757.

Does this mean that those failures were expected ? Should we proceed ?
Flags: needinfo?(pbrosset)
(In reply to julian.descottes from comment #21)
> Thanks Patrick.
> 
> Got two orange lights on Linux & Linux Debug : Bug 1144323 & Bug 1158027.
> Got one red on Linux Debug : Bug 1137757.
> 
> Does this mean that those failures were expected ? Should we proceed ?
Yeah, all intermittent failures unrelated to your patch. So let's go and land this.
Flags: needinfo?(pbrosset)
Keywords: checkin-needed
(What the checkin-needed keyword means is that the bug will now show up on one of our sheriffs' radar who will then push the changeset to fx-team. Once on this inbound branch, and if it sticks, it will then get merged into mozilla-central very shortly after. This, in turn, means that it will be built with the next Nightly. Once in Nightly, it rides the trains to release. The next merge to (once called aurora) dev-edition with FF40 is 2015-05-11).

Thanks a lot Julian for your help!

If you feel like giving a shot at fixing other devtools bugs, here's the list of all current mentored devtools bugs: http://mzl.la/1bpApKp
Here's a list of bugs we've recently flagged as being "easy-ish" and that are sort of important for the ux when using the tools: http://mzl.la/1K1YrXo 
But that doesn't mean you should only look there, a lot of bugs aren't flagged with a mentor, but it's easy to find one if found a bug you'd like to work on. 
Bugs Ahoy is also a good tool for finding bugs: http://www.joshmatthews.net/bugsahoy/?devtools=1&js=1&html=1&unowned=1
Amazing ! Thanks for the mentoring & support on this bug, it was fun :)
https://hg.mozilla.org/mozilla-central/rev/b4ad268f6938
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.