Last Comment Bug 734061 - Web console breaks TAB navigation
: Web console breaks TAB navigation
Status: RESOLVED FIXED
[autocomplete][a11y][good-first-bug][...
: access, regression
Product: Firefox
Classification: Client Software
Component: Developer Tools: Console (show other bugs)
: Trunk
: All All
: P3 normal (vote)
: Firefox 24
Assigned To: Minarto Margoliono
:
Mentors:
Depends on:
Blocks: 735529
  Show dependency treegraph
 
Reported: 2012-03-08 06:34 PST by Mounir Lamouri (:mounir)
Modified: 2013-11-13 05:07 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
affected


Attachments
Bug 734061: Update tab behaviour to go to next element when the input is empty; r=msucan (999 bytes, patch)
2013-06-08 15:30 PDT, Minarto Margoliono
no flags Details | Diff | Splinter Review
Bug 734061: Update tab behaviour to go to next element when the input is empty; r=msucan (2.41 KB, patch)
2013-06-09 02:57 PDT, Minarto Margoliono
no flags Details | Diff | Splinter Review
Bug 734061: Update tab behaviour to go to next element when the input is empty; r=msucan (6.02 KB, patch)
2013-06-11 03:16 PDT, Minarto Margoliono
no flags Details | Diff | Splinter Review
Bug 734061: Update tab behaviour to go to next element when the input is empty; r=msucan (7.62 KB, patch)
2013-06-11 06:25 PDT, Minarto Margoliono
no flags Details | Diff | Splinter Review
Bug 734061: Update tab behaviour to go to next element when the input is empty (7.69 KB, patch)
2013-06-12 16:33 PDT, Minarto Margoliono
mihai.sucan: review+
Details | Diff | Splinter Review

Description Mounir Lamouri (:mounir) 2012-03-08 06:34:29 PST
With Nightly, if I open the web console and use TAB to navigate in the content/chrome, I got stuck in the web console prompt. Sadly, the prompt doesn't even use the TAB to show a tabulation, it's just prevent the focus to leave it. For a11y reasons, we should probably leave the prompt when TAB is pressed.

Also, this is working as expected in Firefox 10.
Comment 1 Mihai Sucan [:msucan] 2012-03-08 09:55:07 PST
Thanks for your report!

Is this with GCLI enabled or disabled?

If GCLI is disabled please see bug 583816 and also read bug 583816 comment 21 and 22.

We've been asked to do the exact opposite in the mentioned bug which was fixed for Firefox 12.
Comment 2 Mounir Lamouri (:mounir) 2012-03-09 06:08:46 PST
If the a11y team agreed on that change I guess my main concern doesn't apply. Though, it might be interesting to be able to disable that with a preference because for people who just wants to use the web console for logs, its pretty annoying.

(for GCLI, I didn't change that so I guess it's the default setting)
Comment 3 Mihai Sucan [:msucan] 2012-03-09 07:13:15 PST
Proposed fix: we could track if the user pressed anything else except Tab since the input was focused. If yes, then Tab does autocomplete, otherwise the default behavior of Tab is allowed (which means the user can Tab out of the input).

Would that be acceptable?
Comment 4 Mounir Lamouri (:mounir) 2012-03-09 07:17:07 PST
That would be an improvement for me. Though, I would like to hear what a11y has to say.
Comment 5 alexander :surkov 2012-03-09 21:08:44 PST
I'd say tab should never stuck somewhere, in general this is a bad practice. F6 option has discoverability issue. Personally if I'm as a keyboard user then I'd use a mouse when tab stucks which is upset. Screen reader users should go to the help or google it which is not really nice. But I think I don't much object if Marco is fine.
Comment 6 Trevor Saunders (:tbsaunde) 2012-03-09 21:25:02 PST
(In reply to alexander :surkov from comment #5)
> I'd say tab should never stuck somewhere, in general this is a bad practice.
> F6 option has discoverability issue. Personally if I'm as a keyboard user
> then I'd use a mouse when tab stucks which is upset. Screen reader users
> should go to the help or google it which is not really nice. But I think I
> don't much object if Marco is fine.

yeah, but you also expect that tab in a command thing completes right? So I think the proposed solution (tab changes focus if nothing is entered otherwise autocomplete)  seems fairly reasonable.
Comment 7 Rob Campbell [:rc] (:robcee) 2013-05-23 09:13:56 PDT
The solution proposed in Comment 3 still seems valid. Marking as good-first-bug.

Filter on GUNGNIR.
Comment 8 Minarto Margoliono 2013-06-08 03:27:45 PDT
Hi I'm new and interested in this bug. Can I take it?
Comment 9 Mihai Sucan [:msucan] 2013-06-08 09:20:36 PDT
Yes, thank you!

Overview of the workflow:

1. Get the Firefox source:

https://developer.mozilla.org/en-US/docs/Developer_Guide/Source_Code/Mercurial

2. Build Firefox:

https://developer.mozilla.org/en-US/docs/Simple_Firefox_build

3. Make a patch:

https://developer.mozilla.org/en-US/docs/Creating_a_patch

4. Submit the patch:

https://developer.mozilla.org/en-US/docs/Developer_Guide/How_to_Submit_a_Patch

If you read through the source code about something you do not know about, you may find documentation here:

1. Mozilla Developer Network - has a ton of info about XUL elements, HTML, JS, DOM, Gecko-specific APIs and more.

http://developer.mozilla.org/

2. MXR (Mozilla Cross-Reference) is a source code search engine - search for symbols you want to learn about, eg. nsIDocument.

http://mxr.mozilla.org/mozilla-central/

3. DXR is a smart source code search tool, similar to MXR but sometimes better.

http://dxr.mozilla.org


Here's an overview of how you can fix this bug:

1. Load browser/devtools/webconsole/webconsole.js and search for the JSTF_keyPress() method. Change the behavior of the Tab keypress handler as explained in comment #3.

2. Submit the patch and ask me for feedback.

After that you will need to write a test. Copy one of the existing tests from the webconsole/tests folder and rename it as you see fit. Add the new test file name in Makefile.in. Run mach again, then run the test with mach. Edit the new script such that it tests the new behavior of Tab in the webconsole input.

For more information about writing tests please see: https://developer.mozilla.org/en-US/docs/Browser_chrome_tests

I'm looking forward for your patch. Thank you!
Comment 10 Minarto Margoliono 2013-06-08 15:30:11 PDT
Created attachment 760156 [details] [diff] [review]
Bug 734061: Update tab behaviour to go to next element when the input is empty; r=msucan

I check for whitespace using trim so it will let the tab through if you have only whitespace characters.

Will do the test case later. I still need to read about it.
Comment 11 Minarto Margoliono 2013-06-09 02:57:23 PDT
Created attachment 760187 [details] [diff] [review]
Bug 734061: Update tab behaviour to go to next element when the input is empty; r=msucan

This seem related to bug 583816, and my patch earlier broke that bug's test case.

Should I update the test case like attached new patch, or should i move that first set of check to its own js test page?
Comment 12 Mihai Sucan [:msucan] 2013-06-10 13:08:24 PDT
Comment on attachment 760187 [details] [diff] [review]
Bug 734061: Update tab behaviour to go to next element when the input is empty; r=msucan

This patch makes Tab move to the next element only if the input is empty. In comment 3 I suggested you check if the user has typed anything since he last focused the input. If yes, then you only do completion. If not, then you allow the default action (move focus to the next element).

(when you submit a patch, please set the review flag to "?" and type :msucan to properly ask me for review)
Comment 13 Mihai Sucan [:msucan] 2013-06-10 13:10:27 PDT
(In reply to lie.r.min.g from comment #11)
> Created attachment 760187 [details] [diff] [review]
> Bug 734061: Update tab behaviour to go to next element when the input is
> empty; r=msucan
> 
> This seem related to bug 583816, and my patch earlier broke that bug's test
> case.
> 
> Should I update the test case like attached new patch, or should i move that
> first set of check to its own js test page?

After the user types something, then Tab, the cursor shall not move to any other element, because in such cases it is clear the user wants to get a completion. Comment #3 should cover this case and the test should pass.
Comment 14 Minarto Margoliono 2013-06-11 03:16:44 PDT
Created attachment 760806 [details] [diff] [review]
Bug 734061: Update tab behaviour to go to next element when the input is empty; r=msucan
Comment 15 Minarto Margoliono 2013-06-11 06:25:20 PDT
Created attachment 760871 [details] [diff] [review]
Bug 734061: Update tab behaviour to go to next element when the input is empty; r=msucan

Sorry, I Forgot to run 'hg add' for the test case before creating the previous patch
Comment 16 Mihai Sucan [:msucan] 2013-06-12 10:16:24 PDT
Comment on attachment 760871 [details] [diff] [review]
Bug 734061: Update tab behaviour to go to next element when the input is empty; r=msucan

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

This is better. Thanks for the updated patch - getting really close to final.

General comments:

1. This patch has trailing whitespaces. Please remove them.
2. If I focus the input and I simply move the cursor or if I just press Shift or some other modifier, then I press Tab it doesn't move focus. This probably happens because |this.changed| is always set to true in the keypress event handler (even if the input value doesn't change).

Minor comments below.

::: browser/devtools/webconsole/test/browser_webconsole_bug_734061_No_input_change_and_Tab_key_pressed.js
@@ +17,5 @@
> +  var jsterm = hud.jsterm;
> +  var input = jsterm.inputNode;
> +
> +  EventUtils.synthesizeKey("VK_TAB", {});
> +  is(input.getAttribute("focused"), "", "focus moved away");

Please also check that the input is focused before pressing Tab.

::: browser/devtools/webconsole/webconsole.js
@@ +2830,5 @@
>    /**
> +   * Indicate input node changed since last focus
> +   * @type boolean
> +   */
> +  changed:false,

Please rename this to _inputChanged and also add a space after a colon.

@@ +2883,5 @@
>      this.inputNode = doc.querySelector(".jsterm-input-node");
>      this.inputNode.addEventListener("keypress", this._keyPress, false);
>      this.inputNode.addEventListener("input", this._inputEventHandler, false);
>      this.inputNode.addEventListener("keyup", this._inputEventHandler, false);
> +    this.inputNode.addEventListener("focus", this._focusEventHandler, true);

Do you need this to be a capturing event? in destroy() the removeEventListener() call doesn't match the call here, |useCapture| argument is |false| there. Please make sure that addEventListener and removeEventListener calls match.

@@ +3854,5 @@
>        this.complete(this.COMPLETE_HINT_ONLY);
>        this.lastInputValue = this.inputNode.value;
>      }
> +    
> +    this.changed = true;

Please only set |changed| to true when lastInputValue != inputNode.value.
Comment 17 Minarto Margoliono 2013-06-12 16:33:49 PDT
Created attachment 761784 [details] [diff] [review]
Bug 734061: Update tab behaviour to go to next element when the input is empty

Updated according review and added pressing right before pressing tab in last test.

What should we do about setInputValue? if i put it inlide if statement as well it vill failing the other test I mentioned earlier
Comment 18 Mihai Sucan [:msucan] 2013-06-14 11:37:39 PDT
Comment on attachment 761784 [details] [diff] [review]
Bug 734061: Update tab behaviour to go to next element when the input is empty

This looks good. Thanks for the update!

I pushed the patch to the try servers: https://tbpl.mozilla.org/?tree=Try&rev=a1f52bbe9a43

If results are green, I will land the patch.
Comment 19 Mihai Sucan [:msucan] 2013-06-17 04:27:13 PDT
Try push was green. Thanks for the patch!

Landed in fx-team:
https://hg.mozilla.org/integration/fx-team/rev/2347bd9390d1
Comment 20 Tim Taubert [:ttaubert] 2013-06-18 03:38:25 PDT
https://hg.mozilla.org/mozilla-central/rev/2347bd9390d1

Note You need to log in before you can comment on or make changes to this bug.