1183325 - debugger shortcut keys should work when console embedded below it has focus

Status: RESOLVED FIXED

(DevTools :: Console, defect)

Description

[Hallvord R. M. Steen [:hallvors]]

I like having the bottom-of-page console open and focused while stepping through scripts, so that I can easily check the value of variables or tweak code by running it in the console between steps.

This is not possible in Firefox devtools. The console below the debugger does not pass on shortcut keypresses like [F11] and [F10] to the debugger above, so one must switch back and forth between keyboard and mouse.

Comment 1

[Hallvord R. M. Steen [:hallvors]]

fitzgen/vporof: if you're not the right people to CC please let me know who to ask :)

Comment 2

[Brian Grinstead [:bgrins]]

The keyboard event fires on webconsole.xul and then bubbles up to toolbox.xul but will never pass through debugger.xul, which is why it doesn't work as you'd hope.  We could maybe set up the toolbox to do some magic when the split console is opened in which it triggers key events originating from the webconsole onto the currently active tool.  I could see that being a bit tricky technically and UX-wise (especially if the same keybinding is used within the console and another tool) but it seems possible.  I'd rather handle it like that than couple the tools together - right now neither the console nor the debugger know that they are being shown in split mode, and I'd prefer to keep it that way.

Comment 3

[Hallvord R. M. Steen [:hallvors]]

Sounds good to me.
Would this be the right place to add it?
https://dxr.mozilla.org/mozilla-central/source/browser/devtools/framework/toolbox.js#467
Something like (below the stuff handling the Escape key)

if(jsdebugger && ! (e.defaultPrevented || e.cancelBubble)){
    jsdebugger.dispatchEvent(e);
}

?

(The let jsdebugger = .. declaration needs to move outside of the if clause, of course).

Comment 4

[Brian Grinstead [:bgrins]]

(In reply to Hallvord R. M. Steen [:hallvors] from comment #3)
> Sounds good to me.
> Would this be the right place to add it?
> https://dxr.mozilla.org/mozilla-central/source/browser/devtools/framework/
> toolbox.js#467
> Something like (below the stuff handling the Escape key)
> 
> if(jsdebugger && ! (e.defaultPrevented || e.cancelBubble)){
>     jsdebugger.dispatchEvent(e);
> }
> 
> ?
> 
> (The let jsdebugger = .. declaration needs to move outside of the if clause,
> of course).

We would want to ensure that the key event would actually match an existing command with the debugger before dispatching (we wouldn't want to dispatch normal typing in the console, for instance).  And also we may not want this behavior for all events that it matched (imagine if pressing Backspace deleted something, we wouldn't want to automatically dispatch just because there is an existing command).

To do this, I guess each tool would need a way to whitelist a set of keycodes or <key> ids [0] that it wants to be passed to from the split console.  Note that we can't just hardcode F11, etc since the key codes are localizable [1].  Since the <key> thing is pretty XUL dependant, it might be best to instead keep a list of the keycodes (which would either need to be somehow parsed out of the dtd / moved to a properties file or read from the Debugger DOM, probably the latter).

Maybe the tool definition could include a function that the toolbox could consult when this event happens.  In other words, Tools.jsdebugger [2] could have another function like shouldStealKeyEventFromSplitConsole() and it would somehow detect if this should happen.

[0]: https://dxr.mozilla.org/mozilla-central/source/browser/devtools/debugger/debugger.xul#179
[1]: https://dxr.mozilla.org/mozilla-central/source/browser/locales/en-US/chrome/browser/devtools/debugger.dtd#198
[2]: https://dxr.mozilla.org/mozilla-central/source/browser/devtools/definitions.js#152

Comment 5

[Hallvord R. M. Steen [:hallvors]]

An alternative might be letting the debugger registering certain selected event listeners on toolbox..? If the event bubbles there from the console anyway. We don't need any coupling between the console and the debugger if we can do that.

Comment 6

[Brian Grinstead [:bgrins]]

(In reply to Hallvord R. M. Steen [:hallvors] from comment #5)
> An alternative might be letting the debugger registering certain selected
> event listeners on toolbox..? If the event bubbles there from the console
> anyway. We don't need any coupling between the console and the debugger if
> we can do that.

So you are saying the debugger would bind to keydown or whatever directly onto the toolbox doc?  I guess it'd be possible by using a capturing listener to have it stopPropagation so the webconsole never saw it.

In this case, we would still need the coupling in _splitConsoleOnKeypress, since in that case we actually *don't* want the debugger to process the ESC.

Comment 7

[Hallvord R. M. Steen [:hallvors]]

So, what about something like the code in comment #3 but adding and exposing a new method (tentative name suggestion wantsKeyInSplitConsole()) to debugger - doing:
 
> if(jsdebugger && ! (e.defaultPrevented || e.cancelBubble) && jsdebugger.wantsKeyInSplitConsole(e) ){
>     jsdebugger.dispatchEvent(e);
> }

?

Hope you don't mind being NI'd all the time.. This is a very important fix for me so apologies in advance if I'm pushing too hard ;)

Comment 8

[Hallvord R. M. Steen [:hallvors]]

If I were to write code in Tools.jsdebugger using a list of identifiers like debuggerUI.stepping.stepIn1 - how could I best look up the VK_KEY / keycode associated with this identifier? I'm thinking along the lines of 

wantsKeyInSplitConsole: function(event){
  var keysWeWant = ['debuggerUI.stepping.stepIn1', 'debuggerUI.stepping.stepOut1', ...];
  for(var i=0; i<keysWeWant; i++){
    var localizedKey = fooKeymapLookup(keysWeWant[i]);
    if(e.key === localizedKey)return true;
  }
  return false;
}

(Or vice versa - doing fooKeymapLookup(e.key) and keysWeWant.indexOf() on the result. The missing piece is where to find some fooKeymapLookup() - like functionality.)

Comment 9

[Brian Grinstead [:bgrins]]

(In reply to Hallvord R. M. Steen [:hallvors] from comment #8)
> If I were to write code in Tools.jsdebugger using a list of identifiers like
> debuggerUI.stepping.stepIn1 - how could I best look up the VK_KEY / keycode
> associated with this identifier? I'm thinking along the lines of 
> 
> wantsKeyInSplitConsole: function(event){
>   var keysWeWant = ['debuggerUI.stepping.stepIn1',
> 'debuggerUI.stepping.stepOut1', ...];
>   for(var i=0; i<keysWeWant; i++){
>     var localizedKey = fooKeymapLookup(keysWeWant[i]);
>     if(e.key === localizedKey)return true;
>   }
>   return false;
> }
> 
> (Or vice versa - doing fooKeymapLookup(e.key) and keysWeWant.indexOf() on
> the result. The missing piece is where to find some fooKeymapLookup() - like
> functionality.)

Yeah, I'm not sure since the keys are stored in a .dtd file.  It'd probably be easiest to instead read the DOM for the panel's stepOverKey. Ideally there would be a method on the <key> element to see if it would hypothetically match an event, but I'm not aware of any way to do that so you may have to just look at its keycode and modifiers and compare it with the event.

Comment 10

[Hallvord R. M. Steen [:hallvors]]

Attachment: Attempted fix for debugger shortcut keys not working with focus in console,

Comment 11

[Hallvord R. M. Steen [:hallvors]]

Attachment: Debugger shortcuts not working when focus is in split console,

removed console.log

Comment 12

[Brian Grinstead [:bgrins]]

Comment on attachment 8653729 [details] [diff] [review]
Attempted fix for debugger shortcut keys not working with focus in console,

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

::: browser/devtools/debugger/panel.js
@@ +23,5 @@
>  
>    this.handleHostChanged = this.handleHostChanged.bind(this);
>    this.highlightWhenPaused = this.highlightWhenPaused.bind(this);
>    this.unhighlightWhenResumed = this.unhighlightWhenResumed.bind(this);
>  

I worked with this a bit, and I think it may be better to do something like this instead of the wantsKeyInSplitConsole event stuff:


  // Add keys from this document's keyset to the toolbox, so they
  // can work when the split console is focused.
  let keysToClone = ["stepOverKey", "stepOverKey2", "stepInKey",
                     "stepInKey2", "stepOutKey", "stepOutKey2"];
  for (let key of keysToClone) {
    let elm = this.panelWin.document.getElementById(key);
    let cloned = elm.cloneNode();
    cloned.setAttribute("oncommand", "void(0)");
    cloned.removeAttribute("command");
    cloned.addEventListener("command", (e) => {
      // Only forward the command if the debugger is focused
      if (toolbox.currentToolId === "jsdebugger") {
        elm.doCommand();
      }
    }, true);
    toolbox.doc.getElementById("toolbox-keyset").appendChild(cloned);
  }

This way it will work regardless of the modifiers and we don't need to deal with as much event junk.  It also wouldn't require any changes to toolbox.js.

Comment 13

[Brian Grinstead [:bgrins]]

Comment on attachment 8653733 [details] [diff] [review]
Debugger shortcuts not working when focus is in split console,

See comment 12

Comment 14

[Brian Grinstead [:bgrins]]

Regarding Comment 12, I think it might make sense to add a Toolbox api for this so other tools could use it and the debugger doesn't have to dig into the toolbox DOM.  I'm thinking something like this:

toolbox.useKeyWithSplitConsole(keyElement, whichTool)

Then the debugger could call something like:

toolbox.useKeyWithSplitConsole(this.panelWin.document.getElementById("stepOverKey"), "jsdebugger")

Comment 15

[Hallvord R. M. Steen [:hallvors]]

Nicer indeed. Almost there, I have some changes that work but.. the "command" tends to move focus to the JS debugger pane, and I haven't yet found a way to either tell it not to, or steal it back when the command is done..

Comment 16

[Hallvord R. M. Steen [:hallvors]]

Attachment: 1183325.patch

This *should* work..but focus doesn't stay in the console, where I want it. Why not?

Comment 17

[Brian Grinstead [:bgrins]]

Comment on attachment 8657607 [details] [diff] [review]
1183325.patch

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

So this looks good overall, but we should probably work out this focus issue before proceeding.  Hope my comment below helps, let me know if you need any help

::: browser/devtools/framework/toolbox.js
@@ +498,5 @@
> +        var focusedElement = this.doc.commandDispatcher.focusedElement;
> +        keyElement.doCommand();
> +        // (The problem is that this code for restoring focus doesn't actually work..)
> +        DevToolsUtils.executeSoon(() => {
> +          focusedElement.focus();

I think it's something the debugger is doing as a result of the command, not the fact that the command is happening.

I log out `Services.focus.focusedElement` even after the command happens and it's still the webconsole textbox.

It seems to work if I do this instead of the executeSoon:

  var focusedElement = Services.focus.focusedElement;
  this.doc.defaultView.setTimeout(() => {
    Services.focus.focusedWindow = focusedElement.ownerDocument.defaultView;
    Services.focus.setFocus(focusedElement, Ci.nsIFocusManager.FLAG_NOSCROLL);
  }, 1000)

Which indicates to me that if we wait for the debugger to focus the editor then it works.  *probably* due to the call to DebuggerView.editor.focus() in the debugger frontend.  I think it'll be hard to come up with a really general solution without changing the debugger frontend.  In other words, all the focus stuff here should probably go away and then instead maybe the toolbox can have an isSplitConsoleFocused() function, and the debugger frontend can check that before calling editor.focus().  (just speculation, I haven't confirmed this is exactly what's happening, but it's what it looks like)

Comment 18

[Hallvord R. M. Steen [:hallvors]]

I've removed DebuggerView.editor.focus() from debugger-controller.js - focus now stays in the console input where I want it. I have done manual testing and also ran the "mach test browser/devtools" command with no failures - it seems OK to just drop this focus() call.

Comment 19

[Hallvord R. M. Steen [:hallvors]]

(In reply to Hallvord R. M. Steen [:hallvors] from comment #18)
> seems OK to just drop this focus() call.

I take that back - did another test run, and some tests fail because keyboard focus isn't quite where they expect. It's important for a11y and keyboard shortcuts to have focus working consistently (and indeed more consistent keyboard behaviour is exactly what this bug is all about) so I have to find a better solution. I will experiment a bit with Services.focus.focusedWindow and focusedElement (but the latter tends to return some XUL element even when the console input box has focus, not sure why) to see if I can find a way to detect whether we should not call focus() for the debugger editor.

Comment 20

[Hallvord R. M. Steen [:hallvors]]

Attachment: b1183325.patch

This patch is getting there, I think.

Comment 21

[Brian Grinstead [:bgrins]]

Comment on attachment 8660949 [details] [diff] [review]
b1183325.patch

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

Looking good - left some notes below

::: browser/devtools/debugger/debugger-controller.js
@@ +703,5 @@
>  
>      this.activeThread.fillFrames(CALL_STACK_PAGE_SIZE);
> +    // Focus the debugger editor - but only if focus is not already inside a TEXTAREA,
> +    // such as the one inside a split console pane
> +    let currentlyFocusedElement = Services.focus.focusedWindow ?

I think this condition logic should move to the toolbox level, and probably be more specific - to make sure it's only when the split console input is focused (and not just any old textarea).  Could probably get the binding parent on the focusedElement and then check it's a .jsterm-input-node

@@ +705,5 @@
> +    // Focus the debugger editor - but only if focus is not already inside a TEXTAREA,
> +    // such as the one inside a split console pane
> +    let currentlyFocusedElement = Services.focus.focusedWindow ?
> +      Services.focus.focusedWindow.document.activeElement : null;
> +    if(!(currentlyFocusedElement && currentlyFocusedElement.localName === 'textarea')){

Nit: space between 'if' and '('.  Also ')' and '{'

::: browser/devtools/debugger/panel.js
@@ +58,5 @@
>          this.target.on("thread-paused", this.highlightWhenPaused);
>          this.target.on("thread-resumed", this.unhighlightWhenResumed);
> +        // Add keys from this document's keyset to the toolbox, so they
> +        // can work when the split console is focused.
> +        let keysToClone = ["resumeKey", "resumeKey2", "stepOverKey", "stepOverKey2",

Nit: please format all of the lines in this function to fit within 80 characters

::: browser/devtools/debugger/test/mochitest/browser_dbg_split-console-keypress.js
@@ +6,5 @@
> + * debugger shortcut keys like F11 should work
> + */
> +const TAB_URL = EXAMPLE_URL + "doc_step-many-statements.html";
> +
> +function test() {

This test should be converted to use Task.async where possible to match existing tests.  The best example I can think of to base that off of is browser_dbg_break-on-next-console.js, which also needs to open up a split console

@@ +29,5 @@
> +        aThreadClient.addOneTimeListener("paused", () => {
> +          // Information for sub-tests. When 'key' is synthesized 'keyRepeat' times,
> +          // cursor should be at 'caretLine' of this test..
> +          let allTests = [
> +            {key: 'VK_F11', keyRepeat: 1, caretLine: 16},

The test case / data is a little hard to follow.  I'll look closer at it once the other changes have been made.

::: browser/devtools/framework/toolbox.js
@@ +484,5 @@
>          e.preventDefault();
>        }
>      }
>    },
>  

Please add a header comment above this function in the format of setZoom, for example

@@ +485,5 @@
>        }
>      }
>    },
>  
> +  useKeyWithSplitConsole: function(keyElement, whichTool){

Nit: include space between ){

@@ +490,5 @@
> +    let cloned = keyElement.cloneNode();
> +    cloned.setAttribute("oncommand", "void(0)");
> +    cloned.removeAttribute("command");
> +    cloned.addEventListener("command", (e) => {
> +      // Only forward the command if the debugger is focused

'if the debugger' -> 'if the tool'

@@ +491,5 @@
> +    cloned.setAttribute("oncommand", "void(0)");
> +    cloned.removeAttribute("command");
> +    cloned.addEventListener("command", (e) => {
> +      // Only forward the command if the debugger is focused
> +      if (this.currentToolId === whichTool) {

We should probably also check to make sure that the split console input is actually focused before passing the command along.

We could have a new toolbox function isSplitConsoleFocused() which can then also be used in debugger-controller.js

Comment 22

[Hallvord R. M. Steen [:hallvors]]

I have tried to add this to the Toolbox.prototype section in toolbox.js:

  /**
   * Get the focused state of the split console
   */
  isSplitConsoleFocused: function() {
    if (!this._splitConsole) {
      return false;
    }
    let focusedWin = Services.focus.focusedWindow ?
      Services.focus.focusedWindow : null;
    return focusedWin && focusedWin ===
      this.doc.querySelector("#toolbox-panel-iframe-webconsole").contentWindow;
  },

but firstly, I don't know if is correct or the best approach and secondly I'm not sure how to call that from debugger-controller.js, because I couldn't find a reference to the current toolbox instance there.

Comment 23

[Hallvord R. M. Steen [:hallvors]]

Attachment: Please have a look at the rewritten test here. I addressed the other comments too AFAIK, except for the question in the NI comment above

Comment 24

[Brian Grinstead [:bgrins]]

(In reply to Hallvord R. M. Steen [:hallvors] from comment #22)
> I have tried to add this to the Toolbox.prototype section in toolbox.js:
> 
>   /**
>    * Get the focused state of the split console
>    */
>   isSplitConsoleFocused: function() {
>     if (!this._splitConsole) {
>       return false;
>     }
>     let focusedWin = Services.focus.focusedWindow ?
>       Services.focus.focusedWindow : null;
>     return focusedWin && focusedWin ===
>      
> this.doc.querySelector("#toolbox-panel-iframe-webconsole").contentWindow;
>   },
> 
> but firstly, I don't know if is correct or the best approach

Sorry for the delay.  Perhaps we can grab `Services.focus.focusedElement` and use getBindingParent to make sure the right node is selected.

Maybe something like:

let focusedElement = Services.focus.focusedElement;
let bindingParent = focusedElement.ownerDocument.getBindingParent(focusedElement);
return bindingParent.matches(".jsterm-input-node");

Note: I haven't tested this, there may be a better way to handle it, but that's the idea.  You can also see toolbox._onFocus / browser_webconsole_split_focus.js, which has to deal with some similar stuff.

> and secondly
> I'm not sure how to call that from debugger-controller.js, because I
> couldn't find a reference to the current toolbox instance there.

Hm, not sure offhand but it looks like this._controller._toolbox = this._toolbox is being assigned in debugger/panel.js so maybe just DebuggerController._toolbox will get a reference to it.

Comment 25

[Brian Grinstead [:bgrins]]

Comment on attachment 8661175 [details] [diff] [review]
Please have a look at the rewritten test here. I addressed the other comments too AFAIK, except for the question in the NI comment above

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

This will need to be rebased on top of the changes in Bug 912121 - aka moving everything into a root /devtools folder.  See https://github.com/jryans/devtools-migrate/blob/master/README.md#migrate-locally.  If you are using mq I have a little migration you could run on the patch file like so: https://gist.github.com/bgrins/8be6ad65a2fd41602f18.

I'd like James to take a look at the test and the debugger changes for some feedback if possible.

::: browser/devtools/debugger/debugger-controller.js
@@ +701,5 @@
>          break;
>      }
>  
>      this.activeThread.fillFrames(CALL_STACK_PAGE_SIZE);
> +    // Focus the debugger editor - but only if focus is not already inside a TEXTAREA,

This should point back to the toolbox's new method as mentioned in comment 24

Comment 26

[Hallvord R. M. Steen [:hallvors]]

(In reply to Brian Grinstead [:bgrins] from comment #24)

> Sorry for the delay.  Perhaps we can grab `Services.focus.focusedElement`
> and use getBindingParent to make sure the right node is selected.

When you say "the right node" you mean the element you type into - the TEXTAREA inside the console. Right? But why is this the only element you won't move keyboard focus away from? If I've focused the "filter" search box focus should remain there, no? If for example I've selected some text in the list of console messages, decide to press [F11] and then [Ctrl]-[C] - I suppose my selection should get copied? If that focus() call runs and focuses the editor IFRAME, the copy shortcut presumably won't work.

Comment 27

[Hallvord R. M. Steen [:hallvors]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ebbeed2c19ff

Comment 28

[Hallvord R. M. Steen [:hallvors]]

Attachment: b1183325-newfilelayout.patch

Attaching a new patch for the new directory structure. I've also used isSplitConsoleFocused() method in the code that decides if focus() is called, but I have not changed the isSplitConsoleFocused() method itself per Brian's suggestions in comment 24 because I think we should override the focus the user has set as infrequently as possible. That's not to say I think the code is perfect as-is, I just think we should discuss focus handling a bit more to get it right ;)

Comment 29

[Hallvord R. M. Steen [:hallvors]]

That try run looks pretty green :)

Comment 30

[James Long (:jlongster)]

Comment on attachment 8664185 [details] [diff] [review]
b1183325-newfilelayout.patch

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

Looks cool, I thought it would be more complicated after seeing all the comments above. While I haven't actually run this to see what the behavior is like, you all have discussed it in depth so I just looked at the code changes.

Comment 31

[Hallvord R. M. Steen [:hallvors]]

Brian, any thoughts on what we discussed in comment 24 and comment 26? I think that's the last issue before we can check this in.

Comment 32

[Brian Grinstead [:bgrins]]

Comment on attachment 8664185 [details] [diff] [review]
b1183325-newfilelayout.patch

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

Regarding Comment 31, my main concern with just treating split console as being focused if anything in that window is that a user may not be aware that the 'console is focused' when they've clicked on an output node or selected some text in the console.  There's a specific use case for using the REPL while stepping that I think the patch solves well and I'm worried we might accidentally cause some issues if we broaden that.

My preference is that "is the split console focused?" is always meant to mean that the console input itself is focused (and not the filter box, variables view, etc).  But it's not a really strong opinion and I wouldn't stop it from landing because of that (it's easy to change in the future anyway if we need to).

::: devtools/client/debugger/debugger-controller.js
@@ +700,5 @@
>          }
>          break;
>      }
>      this.activeThread.fillFrames(CALL_STACK_PAGE_SIZE);
> +    // Focus the debugger editor - but only if focus is not inside a split console pane

Nit: 80 characters.  Suggested wording:

   // Focus the editor, but don't steal focus from the split console.

::: devtools/client/debugger/test/mochitest/browser_dbg_split-console-keypress.js
@@ +48,5 @@
> +    executeSoon(() => generateMouseClickInTab(gTab,
> +      "content.document.getElementById('start')"));
> +    yield waitForPause(gThreadClient);
> +
> +    // Focus the console and add event listener to track whether it looses focus

looses->loses

@@ +55,5 @@
> +    jsterm.inputNode.focus();
> +    jsterm.inputNode.addEventListener('blur',
> +      () => {consoleInputLostFocus = true;});
> +
> +    is(gThreadClient.paused, true,

Nit: this can be put onto one line

@@ +58,5 @@
> +
> +    is(gThreadClient.paused, true,
> +      "Should be paused at debugger statement.");
> +    // As long as we have test work to do..
> +    while (stepTests[0]) {

Instead of doing the while() and then stepTests.shift() and then testCounter++, could that be replaced with a for loop?

for (let i = 0; i < stepTests.length; i++) {
  let test = stepTests[i];
  ...
}

@@ +64,5 @@
> +      if (stepTests[0].keyRepeat > 0) {
> +        stepTests[0].keyRepeat --;
> +        let keyMods = stepTests[0].modifier === 'Shift' ? {shiftKey:true} : {};
> +        executeSoon(() => {EventUtils.synthesizeKey(stepTests[0].key,
> +          keyMods, gDebugger)});

I think this key should be simulated in the web console window and not the debugger, right?  Maybe the 3rd param can be omitted since the web console is focused?

@@ +84,5 @@
> +      stepTests.shift();
> +      testCounter++;
> +    }
> +    // Did focus go missing while we were stepping?
> +    is(consoleInputLostFocus, false, "Console input should not loose focus");

loose->lose

::: devtools/client/framework/toolbox.js
@@ +332,5 @@
>    },
>    /**
> +   * Get the focused state of the split console
> +   */
> +  isSplitConsoleFocused: function() {

I think we need a separate small test for isSplitConsoleFocused and useKeyWithSplitConsole.  It could be a new test: devtools/client/framework/test/browser_toolbox_split_console.js.

A good test to base that off of is something simple like browser_toolbox_getpanelwhenready.js (sorry, lots of the framework tests haven't been converted to the add_task format yet so just picking one that has).

For useKeyWithSplitConsole it could maybe add a new key to an existing tool (like inspector), where oncommand it asserts that it's been called.  Then focus split console, simulate the key combo in the console.

Comment 33

[Hallvord R. M. Steen [:hallvors]]

Attachment: debugger shortcut keys when split console has focus, ,

Comment 34

[Hallvord R. M. Steen [:hallvors]]

New tests for review (sorry, didn't make a separate commit / patch for those new files). Addressed your other comments too, except for the issue about what it means that the split console is "focused". I agree we can refine it later if it becomes an issue.
Also, there's another try run here - mostly to make sure the new tests behave well:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6e42c915194d

Comment 35

[Hallvord R. M. Steen [:hallvors]]

(I tried to push to review to see how that workflow is  https://reviewboard.mozilla.org/r/19295 - with a small additional change in the comment header of one of the new tests)

Comment 36

[Hallvord R. M. Steen [:hallvors]]

Attachment: MozReview Request: debugger shortcut keys when split console has focus, bug 1183325, r?bgrins

debugger shortcut keys when split console has focus, bug 1183325, r=bgrins

Comment 37

[Brian Grinstead [:bgrins]]

Comment on attachment 8669621 [details]
MozReview Request: debugger shortcut keys when split console has focus, bug 1183325, r?bgrins

https://reviewboard.mozilla.org/r/19295/#review19143

Looks good, thanks for getting the tests up and running!  Mostly just nits at this point, although I would like to see the tests merged into one before landing (see my comment below for the justification of this).

::: devtools/client/debugger/test/mochitest/browser_dbg_split-console-keypress.js:61
(Diff revision 1)
> +    for (let i=0, thisTest; thisTest = stepTests[i]; i++) {

Nit: `let i = 0`

::: devtools/client/framework/test/browser_toolbox_split_console.js:4
(Diff revision 1)
> +// Tests that split console focus handling and isSplitConsoleFocused

I think you could fold the code from browser_toolbox_split_console_key_delegation.js
into this test case.  The reason is that we have an existing console test that covers what this one is currently doing (minus the isSplitConsoleFocused() check) so I don't think we need a separate test file for this: https://dxr.mozilla.org/mozilla-central/source/devtools/client/webconsole/test/browser_webconsole_split_focus.js.

::: devtools/client/framework/test/browser_toolbox_split_console_key_delegation.js:10
(Diff revision 1)
> +add_task(function*() {

This test would be a bit easier to follow if it was split into two functions, something similar to:
  
let xulns = "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul";
  
add_task(function*() {

  let tab = yield addTab(URL);
  let target = TargetFactory.forTab(tab);
  gToolbox = yield gDevTools.showToolbox(target, "inspector");
  gDebugger = gToolbox.getPanel("inspector").panelWin;
  yield testUseKeyWithSplitConsole();
  yield testNormalKeyBehavior();
  yield cleanup();
});

function* testUseKeyWithSplitConsole() {
  let keyElm = gDebugger.document.createElementNS(xulns, "key");
  let commandCalled = false;
  keyElm.setAttribute("keycode", "VK_F3");
  keyElm.addEventListener("command", () => {commandCalled = true}, false);
  gDebugger.document.getElementsByTagName('keyset')[0].appendChild(keyElm);
  gToolbox.useKeyWithSplitConsole(keyElm, "inspector");
  let consoleInput = gToolbox.getPanel("webconsole").hud.jsterm.inputNode;
  consoleInput.focus();
  synthesizeKeyElement(keyElm);
  ok(commandCalled, "The shortcut key triggered the command");
}
function* testNormalKeyBehavior() {
  let keyElm = gDebugger.document.createElementNS(xulns, "key");
  let commandCalled = false;
  keyElm.setAttribute("keycode", "VK_F4");
  keyElm.setAttribute("modifiers", "control");
  keyElm.addEventListener("command", () => {commandCalled = true}, false);
  gDebugger.document.getElementsByTagName('keyset')[0].appendChild(keyElm);
  gToolbox.useKeyWithSplitConsole(keyElm, "jsdebugger");
  consoleInput.focus();
  yield EventUtils.synthesizeKey("VK_F4", {"controlKey": true});
  ok(!commandCalled, "The shortcut key should not trigger other tool's command");
}

::: devtools/client/framework/test/browser_toolbox_split_console_key_delegation.js:14
(Diff revision 1)
> +  gDebugger = gToolbox.getPanel("inspector").panelWin;

Nit: assigning a variable called 'gDebugger' for the inspector panel.  Could just name this panelWin

::: devtools/client/framework/test/browser_toolbox_split_console_key_delegation.js:27
(Diff revision 1)
> +  //yield EventUtils.synthesizeKey("VK_F3", {"controlKey": true});

Nit: commented out code

::: devtools/client/framework/test/browser_toolbox_split_console_key_delegation.js:41
(Diff revision 1)
> +  finish();

Nit: shouldn't need to call finish() with add_task

::: devtools/client/framework/toolbox.js:340
(Diff revision 1)
> +    let focusedWin = Services.focus.focusedWindow ?

Is this ternary needed?  Given the return statement, I think this is equivalent:

`let focusedWin = Services.focus.focusedWindow`

Comment 38

[Hallvord R. M. Steen [:hallvors]]

Comment on attachment 8669621 [details]
MozReview Request: debugger shortcut keys when split console has focus, bug 1183325, r?bgrins

debugger shortcut keys when split console has focus, bug 1183325, r?bgrins

Comment 39

[Brian Grinstead [:bgrins]]

Attachment: tests-folded.patch

As we discussed on irc, here's a sample of folding the two tests into one.  I think it's good, take a look at browser_toolbox_split_console.js in the patch

Comment 40

[Brian Grinstead [:bgrins]]

Comment on attachment 8669621 [details]
MozReview Request: debugger shortcut keys when split console has focus, bug 1183325, r?bgrins

See comment 39

Comment 41

[Hallvord R. M. Steen [:hallvors]]

Comment on attachment 8669621 [details]
MozReview Request: debugger shortcut keys when split console has focus, bug 1183325, r?bgrins

debugger shortcut keys when split console has focus, bug 1183325, r?bgrins

Comment 42

[Hallvord R. M. Steen [:hallvors]]

So I pushed those changes to reviewboard and also did another Try run just to be sure..
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cec939c93e46
What's next step? Adding checkin-needed keyword?

Comment 43

[Brian Grinstead [:bgrins]]

Comment on attachment 8669621 [details]
MozReview Request: debugger shortcut keys when split console has focus, bug 1183325, r?bgrins

https://reviewboard.mozilla.org/r/19295/#review19403

Comment 44

[Brian Grinstead [:bgrins]]

(In reply to Hallvord R. M. Steen [:hallvors] from comment #42)
> So I pushed those changes to reviewboard and also did another Try run just
> to be sure..
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=cec939c93e46
> What's next step? Adding checkin-needed keyword?

FYI on the try run, you will probably want to switch to: -u mochitests which should make sure it runs them on e10s as well.  I use this syntax usually to make sure all devtools tests are covered across platforms: `try: -b do -p linux,linux64,macosx64,win32 -u xpcshell,mochitests`.

I went ahead and did a new try push just to be sure before we push it: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a680ee3acefe

Comment 45

[Hallvord R. M. Steen [:hallvors]]

The new browser_dbg_split-console-keypress.js causes some timeout failures. Not sure what to do about this - when I run it locally it passes fine.. It says "SUITE-END | took 32s" but I guess that's the full time from Firefox starts?

I think given all the stepping that it's justified to request a longer timeout. 

(Another issue I'm bothered by is that stepping through source code feels slow - I'm planning to write a performance test based on this helper file, simply timing how long it takes to step through that entire script. You could of course argue that the amount of step into is excessive for this test..)

Comment 46

[Brian Grinstead [:bgrins]]

(In reply to Hallvord R. M. Steen [:hallvors] from comment #45)
> The new browser_dbg_split-console-keypress.js causes some timeout failures.
> Not sure what to do about this - when I run it locally it passes fine.. It
> says "SUITE-END | took 32s" but I guess that's the full time from Firefox
> starts?
> 
> I think given all the stepping that it's justified to request a longer
> timeout. 
> 
> (Another issue I'm bothered by is that stepping through source code feels
> slow - I'm planning to write a performance test based on this helper file,
> simply timing how long it takes to step through that entire script. You
> could of course argue that the amount of step into is excessive for this
> test..)

That could very well be it - linux debug builds on try run really slowly.  I'd suggest let's limit the amount of stepping in the test to maybe one or two commands of each type, since this test is meant to exercise the keyboard shortcut functionality and not the debugger itself.

Comment 47

[Brian Grinstead [:bgrins]]

And yes, a performance test based on something like this would be really handy

Comment 48

[Hallvord R. M. Steen [:hallvors]]

I've cut down (even further..first version had 100s :-p) on the number of key repeats, here's a new try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=035b4d8c44c5
Haven't requested a longer timeout. Yet.

Comment 49

[Hallvord R. M. Steen [:hallvors]]

Comment on attachment 8669621 [details]
MozReview Request: debugger shortcut keys when split console has focus, bug 1183325, r?bgrins

debugger shortcut keys when split console has focus, bug 1183325, r?bgrins

Comment 50

[Hallvord R. M. Steen [:hallvors]]

Brian, I don't see any errors in that try run that might be caused by this patch. Agree? Checkin ready?

Comment 51

[Brian Grinstead [:bgrins]]

Comment on attachment 8669621 [details]
MozReview Request: debugger shortcut keys when split console has focus, bug 1183325, r?bgrins

yeah, i'll check it in

Comment 52

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/45bc8c17d72e

Comment 53

[Hallvord R. M. Steen [:hallvors]]

So, now I can close this as fixed - right? :D

Comment 54

[Brian Grinstead [:bgrins]]

(In reply to Hallvord R. M. Steen [:hallvors] from comment #53)
> So, now I can close this as fixed - right? :D

No, we just wait for it to hit mozilla-central and it will be resolved at that point

Comment 55

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/45bc8c17d72e