Closed Bug 820735 Opened 11 years ago Closed 11 years ago

[toolbox] Tools shortcuts should work from the toolbox window

Categories

(DevTools :: Framework, defect, P2)

x86
All
defect

Tracking

(firefox20 fixed)

RESOLVED FIXED
Firefox 21
Tracking Status
firefox20 --- fixed

People

(Reporter: paul, Assigned: Optimizer)

References

Details

Attachments

(1 file, 5 obsolete files)

STR:
- open and focus the tools in a window
- press shift-f7

The style editor should show-up, but it doesn't.

The <keys> are only present in browser.xul. They should also be in toolbox.xul.
Priority: -- → P2
Component: Developer Tools → Developer Tools: Framework
Priority: P2 → P1
Priority: P1 → P2
My 2 thoughts :

1) The keys should be added dynamically via JS as the tools can be dynamically added too, and all the keys need to be added.

2) The keys should only be added in the window mode, and removed when switching to docked mode, as then there will be 2 commands triggered on keypress in docked state.

@Paul - what say ?
Flags: needinfo?(paul)
(In reply to Girish Sharma [:Optimizer] from comment #1)
> My 2 thoughts :
> 
> 1) The keys should be added dynamically via JS as the tools can be
> dynamically added too, and all the keys need to be added.

Right. Maybe the keys can be cloned from browser.xul?

> 2) The keys should only be added in the window mode, and removed when
> switching to docked mode, as then there will be 2 commands triggered on
> keypress in docked state.

Right. In toolbox-window.xul I guess.
Flags: needinfo?(paul)
QA Contact: developer.tools → scrapmachines
Every Single Time -_-
Assignee: nobody → scrapmachines
QA Contact: scrapmachines
Attached patch WIP 1 (obsolete) — Splinter Review
This works, except for the fact that there is no gBrowser available in the toolbox window, so the shortcuts/commands just give an error on the error console.

I am stuck here now. It seems to me that this approach is not the right one. I am also looking on how to forward the event to parent window, if possible.
Attachment #697544 - Flags: feedback?(paul)
Attached patch patch v1.0 (obsolete) — Splinter Review
Works :)
Attachment #697544 - Attachment is obsolete: true
Attachment #697544 - Flags: feedback?(paul)
Attachment #698292 - Flags: review?(paul)
Comment on attachment 698292 [details] [diff] [review]
patch v1.0

This looks good to me. Please add a test.

>diff --git a/browser/devtools/framework/Toolbox.jsm b/browser/devtools/framework/Toolbox.jsm
>--- a/browser/devtools/framework/Toolbox.jsm
>+++ b/browser/devtools/framework/Toolbox.jsm
>@@ -256,31 +256,76 @@ Toolbox.prototype = {
>         this.isReady = true;
> 
>         let closeButton = this.doc.getElementById("toolbox-close");
>         closeButton.addEventListener("command", this.destroy, true);
> 
>         this._buildDockButtons();
>         this._buildTabs();
>         this._buildButtons();
>+        this._addKeysToWindow();

Don't you think we should call this function only if (host.type == "window")...

>   /**
>+   * Adds the keys and commands to the Toolbox Window in window mode.
>+   *
>+   * @param {host} host
>+   *        The host for the toolbox.
>+   */
>+  _addKeysToWindow: function TBOX__addKeysToWindow(host = this._host) {
>+    if (host.type == "window") {

... and then avoid this?

>+      let doc = this.doc.defaultView.parent.document;
>+      Cu.import("resource:///modules/devtools/gDevTools.jsm", doc.defaultView);
>+      for (let [id, toolDefinition] of gDevTools._tools) {
>+        // Prevent multiple entries for the same tool.
>+        if (doc.getElementById("Tools:" + id)) {
>+          return;
>+        }

I'm not against this, but did you run in a situation where this was needed?

>+        let cmd = doc.createElement("command");
>+        cmd.id = "Tools:" + id;
>+        cmd.setAttribute("oncommand",
>+          'gDevToolsBrowser.toggleToolboxCommand(window.opener.gBrowser, "' +
>+          id + '");');

What is `opener`? I didn't know about that. parent doesn't work?

>+        doc.getElementById("toolbox-commandset").appendChild(cmd);
>+
>+        let key = null;
>+        if (toolDefinition.key) {
>+          key = doc.createElement("key");
>+          key.id = "key_" + id;
>+
>+          if (toolDefinition.key.startsWith("VK_")) {
>+            key.setAttribute("keycode", toolDefinition.key);
>+          } else {
>+            key.setAttribute("key", toolDefinition.key);
>+          }
>+
>+          key.setAttribute("oncommand",
>+            'gDevToolsBrowser.toggleToolboxCommand(window.opener.gBrowser, "' +
>+            id + '");');

key.setAttribute("command", cmd.id)

>-function WindowHost() {
>+function WindowHost(hostTab) {
>   this._boundUnload = this._boundUnload.bind(this);
> 
>+  this._parentWindow = hostTab.ownerDocument.defaultView;

Is that deleted somewhere?

Can we have one test?
Attachment #698292 - Flags: review?(paul) → review-
(In reply to Paul Rouget [:paul] from comment #6)

> Don't you think we should call this function only if (host.type ==
> "window")...

> ... and then avoid this?

I thought only 1 check would be better than 2.

> >+      let doc = this.doc.defaultView.parent.document;
> >+      Cu.import("resource:///modules/devtools/gDevTools.jsm", doc.defaultView);
> >+      for (let [id, toolDefinition] of gDevTools._tools) {
> >+        // Prevent multiple entries for the same tool.
> >+        if (doc.getElementById("Tools:" + id)) {
> >+          return;
> >+        }
> 
> I'm not against this, but did you run in a situation where this was needed?

I am not sure, just seemed like a preventive measure in case of any memory leak.

> What is `opener`? I didn't know about that. parent doesn't work?

In the mdn docs, window.opener is used whenever window is opened via openWindow or openDialog method (https://developer.mozilla.org/en-US/docs/Working_with_windows_in_chrome_code#Example_2:_Interacting_with_the_opener)

I did not try parent, but they are not the same. Try this scratchpad in Browser context on any page with a proper title.

let w = Services.ww.openWindow(window, "about:home", "_blank", "", null);
alert([w.opener.document.title, w.parent.document.title]);

> key.setAttribute("command", cmd.id)

I tried that, it was behaving weirdly. May be that is the reason it is not used http://mxr.mozilla.org/mozilla-central/source/browser/devtools/framework/gDevTools.jsm#379 too ?

> 
> >-function WindowHost() {
> >+function WindowHost(hostTab) {
> >   this._boundUnload = this._boundUnload.bind(this);
> > 
> >+  this._parentWindow = hostTab.ownerDocument.defaultView;
> 
> Is that deleted somewhere?

Is what deleted somewhere ?

> Can we have one test?

Sure.
(In reply to Girish Sharma [:Optimizer] from comment #7)
> (In reply to Paul Rouget [:paul] from comment #6)
> 
> > Don't you think we should call this function only if (host.type ==
> > "window")...
> 
> > ... and then avoid this?
> 
> I thought only 1 check would be better than 2.

Ok. Then better to just do:
if (host.type != "window") {
  return;
}
 
> > key.setAttribute("command", cmd.id)
> 
> I tried that, it was behaving weirdly. May be that is the reason it is not
> used

What went wrong?

> http://mxr.mozilla.org/mozilla-central/source/browser/devtools/framework/
> gDevTools.jsm#379 too ?

I'm actually changing this line in bug 815239.

> > 
> > >-function WindowHost() {
> > >+function WindowHost(hostTab) {
> > >   this._boundUnload = this._boundUnload.bind(this);
> > > 
> > >+  this._parentWindow = hostTab.ownerDocument.defaultView;
> > 
> > Is that deleted somewhere?
> 
> Is what deleted somewhere ?

this._parentWindow, is it deleted?
No risk for memory leak here?
(In reply to Paul Rouget [:paul] from comment #8)
> (In reply to Girish Sharma [:Optimizer] from comment #7)
> > (In reply to Paul Rouget [:paul] from comment #6)
> > 
> > > Don't you think we should call this function only if (host.type ==
> > > "window")...
> > 
> > > ... and then avoid this?
> > 
> > I thought only 1 check would be better than 2.
> 
> Ok. Then better to just do:
> if (host.type != "window") {
>   return;
> }

done.

> > > key.setAttribute("command", cmd.id)
> > 
> > I tried that, it was behaving weirdly. May be that is the reason it is not
> > used
> 
> What went wrong?
> 
> > http://mxr.mozilla.org/mozilla-central/source/browser/devtools/framework/
> > gDevTools.jsm#379 too ?
> 
> I'm actually changing this line in bug 815239.

Changed.

> > > 
> > > >-function WindowHost() {
> > > >+function WindowHost(hostTab) {
> > > >   this._boundUnload = this._boundUnload.bind(this);
> > > > 
> > > >+  this._parentWindow = hostTab.ownerDocument.defaultView;
> > > 
> > > Is that deleted somewhere?
> > 
> > Is what deleted somewhere ?
> 
> this._parentWindow, is it deleted?
> No risk for memory leak here?

Ahh.. I should make it null in the destroy() method.

About the test. I am creating the test for the toolbox docked state (of course, the toolbox will detach in the test). But I have no idea if all the tests are run with toolbox detached, how will this test behave.
(In reply to Girish Sharma [:Optimizer] from comment #9)
> About the test. I am creating the test for the toolbox docked state (of
> course, the toolbox will detach in the test). But I have no idea if all the
> tests are run with toolbox detached, how will this test behave.

Force the host to WINDOW.
(In reply to Girish Sharma [:Optimizer] from comment #5)
> ::: browser/devtools/framework/toolbox.xul
> @@ +15,5 @@
> >  
> >  <window xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
> > +
> > +  <script type="application/javascript" src="chrome://global/content/globalOverlay.js"/>
> > +
> 
> Nice.. I was thinking that instead of Cu.import-ing gDevTools.jsm in the
> patch for Bug 820735, I should also do the same ?

reply from bug 817551.

Up to you. If you manage to forward the commands to the main window, that can be nice.
(In reply to Paul Rouget [:paul] from comment #11)
> Up to you. If you manage to forward the commands to the main window, that
> can be nice.

Even if I use <script> tab instead of Cu.import, the approach would still be the same. I tried event forwarding by attaching eventListeners to the dynamically added keys in the toolbox. Then after creating new KeyboardEvent and copying all the information from the captured event to this new one and dispatching this new event to the window. It did not work, Even after I debugged it using Browser Debugger, I could not find the reason why. The listeners were never getting called for some reason.

Anyways, I think the current approach is good, and saves us the pain of recreating the event and then dispatching it to the parent window.
I found an issue in this patch, well in general with the current approach used in gDevToolsBrowser.

With this patch applied :

1. Open an undocked tool in tab1, say web console.
2. Try the shortcut of debugger from that toolbox, it works.
3. Switch to tab2 and hit the shortcut for web console, a new toolbox window appears.
4. Select the toolbox opened in step 1 (which would have debugger opened now)
5.A. Hit the shortcut for Inspector
6.A. Inspector in the toolbox opened in step 3 opens, instead of getting opened in the selected toolbox.
5.B. Hit the shortcut for Web console.
6.B. Toolbox opened in the step 3 closes as web console was selected there (this is although getting fixed in bug 818442)

this is because gDevToolsBrowser.toggleToolboxCommand method uses the selectedTab to get the target. Don't know what to do in this situation now.
> well in general with the current approach used in gDevToolsBrowser.

Why do you say that?
> this is because gDevToolsBrowser.toggleToolboxCommand method uses the selectedTab to get the target.

Thinking about it, toggleToolboxCommand is for localTab targets only.
Toolbox.xul needs to support non localtab targets.

So we need to build a new set of commands just for Toolbox.xul, that won't be based on gDevToolsBrowser.
Attached patch Patch v2.0 with tests (obsolete) — Splinter Review
This works without having the issue I mentioned previously.

Test added and pushed to try : https://tbpl.mozilla.org/?tree=Try&rev=2617478009e5
Attachment #698292 - Attachment is obsolete: true
Attachment #699740 - Flags: review?(paul)
try is green
Comment on attachment 699740 [details] [diff] [review]
Patch v2.0 with tests

The host parameter is useless.
What is void(0)?
Attachment #699740 - Flags: review?(paul) → review-
(In reply to Paul Rouget [:paul] from comment #18)
> Comment on attachment 699740 [details] [diff] [review]
> Patch v2.0 with tests
> 
> The host parameter is useless.

How will I know that its a WINDOW ?

> What is void(0)?

Its a void function call, that does nothing. This is the trick I use in my addons to properly trigger "command" event listener. Without this the handler for "command" event does not get triggered.
(In reply to Girish Sharma [:Optimizer] from comment #19)
> (In reply to Paul Rouget [:paul] from comment #18)
> > Comment on attachment 699740 [details] [diff] [review]
> > Patch v2.0 with tests
> > 
> > The host parameter is useless.
> 
> How will I know that its a WINDOW ?
 
Nevermind, got it. Its useless indeed.
any thoughts ?
Flags: needinfo?(paul)
Don't create a command element. Just add the command listener to the key.
Flags: needinfo?(paul)
Depends on: 829913
Attached patch patch v2.1 with tests (obsolete) — Splinter Review
Unfortunately, <key> and <command> XUL elements do not emit "command" event, unless they have the attribute "command" or "oncommand" set to something. So "void(0)" is required.

This patch takes care of a few more edge cases:

1) When a dynamic tool registers and the hostType is window, its key is also added.
2) When this same tool unregisters and the hostType is window, its key is removed.
3) I was returning instead of continuing when any key was already found.

Tests pass except while finishing the test, due to bug 829913, the test timeouts. This is only seen on fx-team for now and is a regression caused by bug 827083. To try the tests, you can use m-c for a while. Or wait for 829913 to get fixed.
Attachment #699740 - Attachment is obsolete: true
Attachment #701451 - Flags: review?(paul)
Attached patch patch v2.2 with tests (obsolete) — Splinter Review
minor change from previous patch. changed "window" to Toolbox.HostType.WINDOW while comparing the toolbox host type.
Attachment #701451 - Attachment is obsolete: true
Attachment #701451 - Flags: review?(paul)
Attachment #701452 - Flags: review?(paul)
This "void(0);" thing is bugging me:
1) void() is not a function, so it will break. If you really want to add something, just use ";"
2) we do use addEventListener("command") for other things, and we don't have to use this hack

... not sure what's going on here.
(In reply to Paul Rouget [:paul] from comment #25)
> This "void(0);" thing is bugging me:
> 1) void() is not a function, so it will break. If you really want to add

void(0) is a valid javascript function, you can try runnign it in scratchpad. It returns nothing and is supposed to be used for situation where you need nothing. [1]


> something, just use ";"
> 2) we do use addEventListener("command") for other things, and we don't have
> to use this hack
> 
> ... not sure what's going on here.

Yes, only xul key and xul command elements have this bug. They do not emit the events until the "oncommand" is set. Several bugs have been filed regarding this. I could find some of them here : bug 371900, bug 439682, bug 688738

Although I am surprised that it is not a well known issue. I faced it back at the time of my first add-on and searched on net to come to this hack.


[1] https://developer.mozilla.org/en-US/docs/JavaScript/Reference/Operators/void
Comment on attachment 701452 [details] [diff] [review]
patch v2.2 with tests

Interesting, I didn't know about these 2 things :)

> +        key.setAttribute("oncommand", "void(0);");

Just add "// needed. See bug 371900" ad the end of this line.
Attachment #701452 - Flags: review?(paul) → review+
added the comment.
Attachment #701452 - Attachment is obsolete: true
Attachment #701544 - Flags: review+
Whiteboard: [land-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/6d8524aaaebb
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 21
Comment on attachment 701544 [details] [diff] [review]
patch v2.2 with tests and comment

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 788977
User impact if declined: Bad UX, user will have to switch back to the main window for keyboard access. Using the toolbox window, keyboard accessibility is broken
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): minimum
String or UUID changes made by this patch: none
Attachment #701544 - Flags: approval-mozilla-aurora?
Attachment #701544 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Whiteboard: [land-in-aurora]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.