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

RESOLVED FIXED in Firefox 28

Status

P3
normal
RESOLVED FIXED
5 years ago
4 months ago

People

(Reporter: basta, Assigned: jacksonisaac2008)

Tracking

unspecified
Firefox 28
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][mentor=msucan][lang=js])

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

5 years ago
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.
STR:

Open console

Blur command input

Select debugger panel

Select console panel

Expected:

Command input has focus

Actual:

Command input doesn't have focus
Priority: -- → P3
Whiteboard: [good first bug][mentor=msucan@mozilla.com][lang=js]
(Assignee)

Comment 2

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

5 years ago
JacksonIsaac is working on this.
Assignee: nobody → jacksonisaac2008
Status: NEW → ASSIGNED
(Assignee)

Comment 5

5 years ago
Created attachment 823212 [details] [diff] [review]
Fixes the Bug #891581.

This patch fixes the Bug #891581. The focus is again set to the input field after blur.
Attachment #823212 - Flags: review?
(Assignee)

Updated

5 years ago
Attachment #823212 - Flags: review? → review?(mihai.sucan)
(Assignee)

Comment 6

5 years ago
Created attachment 823948 [details] [diff] [review]
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.
Attachment #823212 - Attachment is obsolete: true
Attachment #823212 - Flags: review?(mihai.sucan)
Attachment #823948 - Flags: review?(mihai.sucan)
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().
Attachment #823948 - Flags: review?(mihai.sucan)
(Assignee)

Comment 8

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

Comment 9

5 years ago
Created attachment 825881 [details] [diff] [review]
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
Attachment #823948 - Attachment is obsolete: true
Attachment #825881 - Flags: review?(mihai.sucan)
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.
Attachment #825881 - Flags: review?(mihai.sucan) → feedback+
(Assignee)

Comment 11

5 years ago
Created attachment 826297 [details] [diff] [review]
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.
Attachment #825881 - Attachment is obsolete: true
Attachment #826297 - Flags: review?(mihai.sucan)
(Assignee)

Comment 12

5 years ago
Created attachment 826407 [details] [diff] [review]
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
Attachment #826297 - Attachment is obsolete: true
Attachment #826297 - Flags: review?(mihai.sucan)
Attachment #826407 - Flags: review?(mihai.sucan)
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 <jacksonisaac2008@gmail.com>
> + *
> + * ***** 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.
Attachment #826407 - Flags: review?(mihai.sucan)
(Assignee)

Comment 14

5 years ago
Created attachment 827304 [details] [diff] [review]
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
Attachment #826407 - Attachment is obsolete: true
Attachment #827304 - Flags: review?(mihai.sucan)
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.
Attachment #827304 - Flags: review?(mihai.sucan) → review+
Landed in fx-team: https://hg.mozilla.org/integration/fx-team/rev/25b9ce4cb63e
OS: Mac OS X → All
Hardware: x86 → All
Whiteboard: [good first bug][mentor=msucan@mozilla.com][lang=js] → [good first bug][mentor=msucan][lang=js][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/25b9ce4cb63e
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [good first bug][mentor=msucan][lang=js][fixed-in-fx-team] → [good first bug][mentor=msucan][lang=js]
Target Milestone: --- → Firefox 28

Updated

4 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.