[toolbox] Tools shortcuts should work from the toolbox window

RESOLVED FIXED in Firefox 20

Status

()

Firefox
Developer Tools: Framework
P2
normal
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: paul, Assigned: Optimizer)

Tracking

Trunk
Firefox 21
x86
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox20 fixed)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

6 years ago
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.
(Reporter)

Updated

6 years ago
Priority: -- → P2
Component: Developer Tools → Developer Tools: Framework
(Reporter)

Updated

6 years ago
Priority: P2 → P1
(Reporter)

Updated

6 years ago
Priority: P1 → P2
(Assignee)

Comment 1

6 years ago
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)
(Reporter)

Comment 2

6 years ago
(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)
(Assignee)

Updated

6 years ago
QA Contact: developer.tools → scrapmachines
(Assignee)

Comment 3

6 years ago
Every Single Time -_-
Assignee: nobody → scrapmachines
QA Contact: scrapmachines
(Assignee)

Comment 4

6 years ago
Created attachment 697544 [details] [diff] [review]
WIP 1

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)
(Assignee)

Comment 5

6 years ago
Created attachment 698292 [details] [diff] [review]
patch v1.0

Works :)
Attachment #697544 - Attachment is obsolete: true
Attachment #697544 - Flags: feedback?(paul)
Attachment #698292 - Flags: review?(paul)
(Reporter)

Comment 6

5 years ago
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-
(Assignee)

Comment 7

5 years ago
(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.
(Reporter)

Comment 8

5 years ago
(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?
(Assignee)

Comment 9

5 years ago
(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.
(Reporter)

Comment 10

5 years ago
(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.
(Reporter)

Comment 11

5 years ago
(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.
(Assignee)

Comment 12

5 years ago
(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.
(Assignee)

Comment 13

5 years ago
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.
(Reporter)

Comment 14

5 years ago
> well in general with the current approach used in gDevToolsBrowser.

Why do you say that?
(Reporter)

Comment 15

5 years ago
> 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.
(Assignee)

Comment 16

5 years ago
Created attachment 699740 [details] [diff] [review]
Patch v2.0 with tests

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)
(Assignee)

Comment 17

5 years ago
try is green
(Reporter)

Comment 18

5 years ago
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-
(Assignee)

Comment 19

5 years ago
(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.
(Assignee)

Comment 20

5 years ago
(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.
(Assignee)

Comment 21

5 years ago
any thoughts ?
Flags: needinfo?(paul)
(Reporter)

Comment 22

5 years ago
Don't create a command element. Just add the command listener to the key.
Flags: needinfo?(paul)
(Assignee)

Updated

5 years ago
Depends on: 829913
(Assignee)

Comment 23

5 years ago
Created attachment 701451 [details] [diff] [review]
patch v2.1 with tests

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)
(Assignee)

Comment 24

5 years ago
Created attachment 701452 [details] [diff] [review]
patch v2.2 with tests

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)
(Reporter)

Comment 25

5 years ago
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.
(Assignee)

Comment 26

5 years ago
(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
(Reporter)

Comment 27

5 years ago
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+
(Assignee)

Comment 28

5 years ago
Created attachment 701544 [details] [diff] [review]
patch v2.2 with tests and comment

added the comment.
Attachment #701452 - Attachment is obsolete: true
Attachment #701544 - Flags: review+
(Assignee)

Updated

5 years ago
Whiteboard: [land-in-fx-team]
https://hg.mozilla.org/integration/fx-team/rev/6d8524aaaebb
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/6d8524aaaebb
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 21
(Assignee)

Comment 31

5 years ago
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?

Updated

5 years ago
Attachment #701544 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Updated

5 years ago
Whiteboard: [land-in-aurora]
(Reporter)

Comment 32

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/71752eb1fec1
status-firefox20: --- → fixed
Whiteboard: [land-in-aurora]
You need to log in before you can comment on or make changes to this bug.