891581 - When switching to Console tab, focus should be given to console input field

Status: RESOLVED FIXED

(DevTools :: Console, defect, P3)

Description

[Matt Basta [:basta]]

When the console is opened, focus should automatically be given to the field to type a command. The input box is the only place in the console that you'd ever want to type, so setting focus makes a lot of sense.

Comment 1

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

STR:

Open console

Blur command input

Select debugger panel

Select console panel

Expected:

Command input has focus

Actual:

Command input doesn't have focus

Comment 2

[Jackson Isaac]

@Nick Fitzgerald

It is working fine here. Do I need to click on the [object Function] after typing blur. If I select [object Function] and then follow the steps after blur command input, it seems to be not focused.

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to jacksonisaac2008 from comment #2)
> @Nick Fitzgerald
> 
> It is working fine here. Do I need to click on the [object Function] after
> typing blur. If I select [object Function] and then follow the steps after
> blur command input, it seems to be not focused.

Should have been more clear. By "blur the command input" I mean "unfocus the input area", so typing "blur" in and then inspecting that function will unfocus the input, but you could also just click somewhere in the logs.

Comment 4

[mjh563]

JacksonIsaac is working on this.

Comment 5

[Jackson Isaac]

Attachment: Fixes the Bug #891581.

This patch fixes the Bug #891581. The focus is again set to the input field after blur.

Comment 6

[Jackson Isaac]

Attachment: Fixes Bug #891581 - Fix the focus to input field when webconsole tab is opened/selected from other tab.

The previous patch was not focusing inside the iFrame tabs i.e. NET CSS etc. This patch fixes that problem.

Comment 7

[Mihai Sucan [:msucan]]

Comment on attachment 823948 [details] [diff] [review]
Fixes Bug #891581 - Fix the focus to input field when webconsole tab is opened/selected from other tab.

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

Thanks for your patch.

::: browser/devtools/webconsole/webconsole.js
@@ +555,5 @@
>      this.jsterm = new JSTerm(this);
>      this.jsterm.init();
>      this.jsterm.inputNode.focus();
> +
> +    let gDevTools = Cu.import("resource:///modules/devtools/gDevTools.jsm", {}).gDevTools;

Please add the gDevTools import at the top of the webconsole.js file, using the lazy importer.

@@ -555,5 @@
>      this.jsterm = new JSTerm(this);
>      this.jsterm.init();
>      this.jsterm.inputNode.focus();
> -    doc.addEventListener('focus', () => {
> -        this.jsterm.inputNode.focus();

This patch seems to be on top of a previous patch in your local codebase. This patch does not apply cleanly for me.

Please rebase this patch on top of a clean mozilla-central codebase.

@@ +557,5 @@
>      this.jsterm.inputNode.focus();
> +
> +    let gDevTools = Cu.import("resource:///modules/devtools/gDevTools.jsm", {}).gDevTools;
> +    let toolbox = gDevTools.getToolbox(this.owner.target);
> +    toolbox.on("webconsole-selected", () => {

You need to also remove the event listener in JST_destroy().

Comment 8

[Jackson Isaac]

(In reply to Mihai Sucan [:msucan] from comment #7)
> Comment on attachment 823948 [details] [diff] [review]
> Fixes Bug #891581 - Fix the focus to input field when webconsole tab is
> opened/selected from other tab.
> 
> Review of attachment 823948 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for your patch.
Happy to fix this :) Will update the patch after rebasing the local repo.
> 
> ::: browser/devtools/webconsole/webconsole.js
> @@ +555,5 @@
> >      this.jsterm = new JSTerm(this);
> >      this.jsterm.init();
> >      this.jsterm.inputNode.focus();
> > +
> > +    let gDevTools = Cu.import("resource:///modules/devtools/gDevTools.jsm", {}).gDevTools;
> 
> Please add the gDevTools import at the top of the webconsole.js file, using
> the lazy importer.

Okay I'll add this there.

> 
> @@ -555,5 @@
> >      this.jsterm = new JSTerm(this);
> >      this.jsterm.init();
> >      this.jsterm.inputNode.focus();
> > -    doc.addEventListener('focus', () => {
> > -        this.jsterm.inputNode.focus();
> 
> This patch seems to be on top of a previous patch in your local codebase.
> This patch does not apply cleanly for me.
> 
> Please rebase this patch on top of a clean mozilla-central codebase.
Didn't notice that, will create a new patch on top of a new clean codebase.

> 
> @@ +557,5 @@
> >      this.jsterm.inputNode.focus();
> > +
> > +    let gDevTools = Cu.import("resource:///modules/devtools/gDevTools.jsm", {}).gDevTools;
> > +    let toolbox = gDevTools.getToolbox(this.owner.target);
> > +    toolbox.on("webconsole-selected", () => {
> 
> You need to also remove the event listener in JST_destroy().

Okay, done :)

Comment 9

[Jackson Isaac]

Attachment: Fixes Bug #891581. The focus is again set to the input field after blur.

Updated the patch on clean codebase. Modified the code as per comment #7

Comment 10

[Mihai Sucan [:msucan]]

Comment on attachment 825881 [details] [diff] [review]
Fixes Bug #891581. The focus is again set to the input field after blur.

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

Patch works fine now. Please add a test in browser/devtools/webconsole/test. Copy one of the existing tests and edit it to do whatever is needed. Add the test to browser.ini. Submit the patch once you get it to run as intended on your system.

The test should open the web console, check the js input has focus, move focus to filter input, open the debugger, switch back to the console, and check the js input has focus again. That is all. Thank you!

::: browser/devtools/webconsole/webconsole.js
@@ +198,5 @@
>  
>    this.output = new ConsoleOutput(this);
>  
>    this._toggleFilter = this._toggleFilter.bind(this);
> +  this._webconsoleFocus = this._webconsoleFocus.bind(this);

Please rename the function to _panelSelected

@@ +358,5 @@
>  
>    // Used in tests.
>    _saveRequestAndResponseBodies: false,
>  
> +  _webconsoleFocus: function WCF__webconsoleFocus()

Add a jsdoc comment for the function explaining the purpose. Please make sure you follow the style of the other method comments.

Comment 11

[Jackson Isaac]

Attachment: Fixes Bug #891581. The focus is again set to the input field after blur.

Updated the patch as reviewed by msucan in comment #10.
mochitest also passed successfully.

Comment 12

[Jackson Isaac]

Attachment: Fixes Bug #891581. The focus is again set to the input field after blur.

Test file added to the patch. Also included it in browser.ini

Comment 13

[Mihai Sucan [:msucan]]

Comment on attachment 826407 [details] [diff] [review]
Fixes Bug #891581. The focus is again set to the input field after blur.

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

Thank you for the updated patch. This is almost done.

Please address the comments below.

::: browser/devtools/webconsole/test/browser.ini
@@ +210,5 @@
>  [browser_webconsole_bug_817834_add_edited_input_to_history.js]
>  [browser_webconsole_bug_821877_csp_errors.js]
>  [browser_webconsole_bug_837351_securityerrors.js]
>  [browser_webconsole_bug_846918_hsts_invalid-headers.js]
> +[browser_webconsole_bug_891581_js_field_focus.js]

Please rename the test to browser_webconsole_input_field_focus_on_panel_select.js

::: browser/devtools/webconsole/test/browser_webconsole_bug_891581_js_field_focus.js
@@ +5,5 @@
> + *
> + * Contributor(s):
> + * Jackson Isaac <[e-mail]>
> + *
> + * ***** END LICENSE BLOCK ***** */

This license boilerplate is obsolete. Please use the public domain license boilerplate from:

https://www.mozilla.org/MPL/headers/

@@ +29,5 @@
> +  
> +  is(hud.jsterm.inputNode.hasAttribute("focused"), true,
> +    "inputNode should be focused");
> +
> +  hud.jsterm.inputNode.blur();

Please set the focus to the filter input. hud.filterBox.focus();

@@ +43,5 @@
> +{
> +  gDebuggerWin = aResult.panelWin;
> +  gDebuggerController = gDebuggerWin.DebuggerController;
> +  gThread = gDebuggerController.activeThread;
> +  gStackframes = gDebuggerController.StackFrames;

You do not need all of these variables. Pleas clean up the test.

::: browser/devtools/webconsole/webconsole.js
@@ +566,5 @@
>      this.jsterm.init();
>      this.jsterm.inputNode.focus();
> +
> +    let toolbox = gDevTools.getToolbox(this.owner.target);
> +    toolbox.on("webconsole-selected", this._panelSelected);

|toolbox| is not always available. With this patch the browser console is broken and many tests fail. Please wrap this line in an if (toolbox) {...} check.

@@ +2832,5 @@
>  
>      this._destroyer = promise.defer();
>  
> +    let toolbox = gDevTools.getToolbox(this.owner.target);
> +    toolbox.off("webconsole-selected", this._panelSelected);

Same as above.

Comment 14

[Jackson Isaac]

Attachment: Fixes Bug #891581. The focus is again set to the input field after blur.

Updated the patch with final touches as reviewed in comment #13

Comment 15

[Mihai Sucan [:msucan]]

Comment on attachment 827304 [details] [diff] [review]
Fixes Bug #891581. The focus is again set to the input field after blur.

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

Thank you for the updated patch!

I will push this patch to the try servers and if the results are positive, I will land it.

Comment 16

[Mihai Sucan [:msucan]]

Try push: https://tbpl.mozilla.org/?tree=Try&rev=81a9c890838e

Comment 17

[Mihai Sucan [:msucan]]

Landed in fx-team: https://hg.mozilla.org/integration/fx-team/rev/25b9ce4cb63e

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/25b9ce4cb63e