Last Comment Bug 583816 - Tab should not move to the page when there's nothing to complete in the Web Console
: Tab should not move to the page when there's nothing to complete in the Web C...
Status: RESOLVED FIXED
[fixed-in-fx-team]
: access, polish
Product: Firefox
Classification: Client Software
Component: Developer Tools: Console (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 12
Assigned To: Jignesh Kakadiya [:jhk]
:
: Brian Grinstead [:bgrins]
Mentors:
: 601867 (view as bug list)
Depends on:
Blocks: console-a11y devtools4b8
  Show dependency treegraph
 
Reported: 2010-08-02 12:14 PDT by Neil Deakin
Modified: 2012-02-01 13:58 PST (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-


Attachments
proposed patch + test code (8.78 KB, patch)
2010-09-10 06:53 PDT, Mihai Sucan [:msucan]
sdwilsh: review+
rcampbell: feedback+
Details | Diff | Splinter Review
updated patch (7.41 KB, patch)
2010-09-14 03:24 PDT, Mihai Sucan [:msucan]
dietrich: approval2.0+
Details | Diff | Splinter Review
rebased patch (7.20 KB, patch)
2010-10-11 13:24 PDT, Mihai Sucan [:msucan]
no flags Details | Diff | Splinter Review
Code completion modified when input is null string (1.05 KB, patch)
2011-12-13 09:45 PST, Jignesh Kakadiya [:jhk]
no flags Details | Diff | Splinter Review
test+patch (6.17 KB, patch)
2011-12-14 01:37 PST, Jignesh Kakadiya [:jhk]
no flags Details | Diff | Splinter Review
Revised test (6.23 KB, patch)
2011-12-14 12:44 PST, Jignesh Kakadiya [:jhk]
no flags Details | Diff | Splinter Review
Patch_3 (6.23 KB, patch)
2011-12-14 12:56 PST, Jignesh Kakadiya [:jhk]
no flags Details | Diff | Splinter Review
patch+ test for tab pressed when no auto complete. (6.23 KB, patch)
2011-12-14 13:01 PST, Jignesh Kakadiya [:jhk]
mihai.sucan: review+
Details | Diff | Splinter Review
Test+Patch (8.43 KB, patch)
2011-12-15 10:07 PST, Jignesh Kakadiya [:jhk]
mihai.sucan: review+
rcampbell: review+
Details | Diff | Splinter Review
patch+test (8.46 KB, patch)
2011-12-16 09:57 PST, Jignesh Kakadiya [:jhk]
no flags Details | Diff | Splinter Review
[in-fx-team] test+patch (8.39 KB, patch)
2011-12-17 15:59 PST, Jignesh Kakadiya [:jhk]
mihai.sucan: review+
Details | Diff | Splinter Review

Description Neil Deakin 2010-08-02 12:14:14 PDT
Steps:

1. Open Tools -> Web Console
2. Focus the textbox that appears
3. Press Tab

Expected:

The focus shifts to the next element (the document frame in this case)

Actual:

Nothing!
Comment 1 Julian Viereck 2010-08-02 23:56:20 PDT
The tab key is used for completion. Therefore it can't be used to set the focus to the next element.
Comment 2 Neil Deakin 2010-08-03 01:57:47 PDT
No, the tab key is used to move the focus from one element to another and is a very basic accessibility need. Right now, once that field is focused there is no obvious means for a keyboard-only user to change the focus anywhere else.

If you want to have tab auto-completion as well, it should at least happen only when there is something worth completing. I didn't look at the code, but my testing suggests that it only happens when text is entered and the caret is at the end of the line. At the least, tab focus navigation shouldn't be prevented in other circumstances.
Comment 3 Julian Viereck 2010-08-03 03:38:49 PDT
(In reply to comment #2)
> No, the tab key is used to move the focus from one element to another and is a
> very basic accessibility need. Right now, once that field is focused there is
> no obvious means for a keyboard-only user to change the focus anywhere else.

Good point.

> If you want to have tab auto-completion as well, it should at least happen only
> when there is something worth completing. I didn't look at the code, but my
> testing suggests that it only happens when text is entered and the caret is at
> the end of the line. At the least, tab focus navigation shouldn't be prevented
> in other circumstances.

I'm not sure if that's a good way to say "If there is something, then use the tab key to complete, otherwise use it to set the focus to the next element". The user might expect that there is a completion and presses tab, but against his expectations, there is no completion shown but the focus has changed.

I add Alexander Limi from the UX Team - he might have an idea how to solve this conflict.
Comment 4 David Dahl :ddahl 2010-08-03 10:17:08 PDT
(In reply to comment #2)
> If you want to have tab auto-completion as well, it should at least happen only
> when there is something worth completing. I didn't look at the code, but my
> testing suggests that it only happens when text is entered and the caret is at
> the end of the line. At the least, tab focus navigation shouldn't be prevented
> in other circumstances.

I think we need to not think of this element as a text input, but rather as a console. That being said,tab moving to next element if there is nothing in the console input is a fine comprimise.
Comment 5 David Bolter [:davidb] 2010-08-03 10:20:41 PDT
Marco, this probably came up in Chatzilla. What was the decision there?
Comment 6 Marco Zehe (:MarcoZ) 2010-08-03 10:29:56 PDT
The decision was this:

1. Tab autocompletes nicknames if one has started typing something, otherwise gives a message saying "There's nothing ot tab-complete".
2. F6 is used to move to the document containing the channel output, and again to move to the list of channel attendees. Once more, you're back in the textfield for entering a message. Shift+F6 walks in reverse order.

Looking at this, I think that F6 should be used in a similar manner. F6 already shifts focus back to the web page, and once more you get to the tab bar, and it doesn't cycle back to the entry field. So, some consistency should be created here.

Moreover, the items in the Webconsole toolbar are only accessible currently if clicking with the mouse once. I'd say there should be a way to make this toolbar tabbable so the user can somehow navigate into it to access the options, because they're not available in other ways. But this might be a separate bug.
Comment 7 David Bolter [:davidb] 2010-08-03 11:02:59 PDT
(In reply to comment #6)
> Moreover, the items in the Webconsole toolbar are only accessible currently if
> clicking with the mouse once. I'd say there should be a way to make this
> toolbar tabbable so the user can somehow navigate into it to access the
> options, because they're not available in other ways. But this might be a
> separate bug.

I filed bug 584121 for this, thanks Marco.
Comment 8 Patrick Walton (:pcwalton) 2010-09-07 17:22:16 PDT
Changing the title of the bug to reflect what the consensus now seems to be.
Comment 9 Mihai Sucan [:msucan] 2010-09-10 06:53:33 PDT
Created attachment 474030 [details] [diff] [review]
proposed patch + test code

Proposed patch and test code.
Comment 10 Rob Campbell [:rc] (:robcee) 2010-09-10 09:16:21 PDT
Comment on attachment 474030 [details] [diff] [review]
proposed patch + test code

Looks good. Should work as-advertised.
Comment 11 Mihai Sucan [:msucan] 2010-09-11 08:18:51 PDT
Comment on attachment 474030 [details] [diff] [review]
proposed patch + test code

Asking for review. This is a bit of UI accessibility fix for the Web Console. Thanks!
Comment 12 Dietrich Ayala (:dietrich) 2010-09-13 07:54:05 PDT
Great polish, not a blocker.
Comment 13 Shawn Wilsher :sdwilsh 2010-09-13 09:22:22 PDT
Comment on attachment 474030 [details] [diff] [review]
proposed patch + test code

>           case 9:
>             // tab key
>             // If there are more than one possible completion, pressing tab
>             // means taking the next completion, shift_tab means taking
>             // the previous completion.
>+            let completionResult;
note: this isn't scoped like you might expect it to be.  You'll either want to add a brace after "case 9:" or just use var.

>+   * @returns boolean true if there existed a completion for the current input,
>+   * or false otherwise.
nit: second line should line up with "boolean"

>+ * The Initial Developer of the Original Code is Mozilla Foundation.
nit: doesn't follow formatting dictated in http://www.mozilla.org/MPL/boilerplate-1.1/mpl-tri-license-c

>+ * Portions created by the Initial Developer are Copyright (C) 2010
>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s):
>+ *  Mihai Șucan <mihai.sucan@gmail.com>
This too.  See http://www.mozilla.org/MPL/boilerplate-1.1/, especially the notes section.

note: I shouldn't have to keep commenting on license issues on dev tools patches...

>+function secondTab() {
>+  isnot(inputNode.getAttribute("focused"), "true",
>+    "inputNode is no longer focused");
nit: should line up with "inputNode.getAttribute" and not the "n" in "isnot".

r=sdwilsh
Comment 14 Mihai Sucan [:msucan] 2010-09-14 03:24:08 PDT
Created attachment 475034 [details] [diff] [review]
updated patch

Thank you Shawn for your review+! I updated the patch with the changes you've requested.

I am asking for approval2.0+ because this patch is UI accessibility polish for the Web Console, and it's ready to go in. Thanks!
Comment 15 Rob Campbell [:rc] (:robcee) 2010-09-29 09:39:13 PDT
adding polish flag. Would like approval for this. Adding blocking request to get it triaged.
Comment 16 Dietrich Ayala (:dietrich) 2010-10-05 06:25:13 PDT
*** Bug 601867 has been marked as a duplicate of this bug. ***
Comment 17 Mihai Sucan [:msucan] 2010-10-11 13:24:20 PDT
Created attachment 482324 [details] [diff] [review]
rebased patch

Rebased patch on top of today's mozilla-central default branch. Only test code changes were needed.
Comment 18 David Dahl :ddahl 2010-10-11 17:40:58 PDT
http://hg.mozilla.org/mozilla-central/rev/5b8dcf82b27a
Comment 19 Dietrich Ayala (:dietrich) 2010-10-26 07:18:29 PDT
Nice polish win, but if were re-opened today, I don't think we should hold back the release for it. blocking-.
Comment 20 Panos Astithas [:past] 2011-06-15 04:03:05 PDT
There is a problem that arises from this fix: when typing in the web console and expecting autocompletion, hitting Tab may move the focus away from the console if you type fast enough, before the autocomplete popup has a chance to appear. This basically penalizes fast users, forcing them to either type slower or resort to Shift-Tab to focus back on the console. There are two solutions that I can see:

1) Use Tab only for autocompletion and use F6 for focus change as described in coment 6.
2) Use right-arrow for autocompletion in addition to Tab, as a workaround for fast users.

FWIW, both Firebug and Chrome/Safari implement the second option. Chrome/Safari doesn't even remove focus from the console when hitting Tab.

Which option would be more preferable?
Comment 21 Kevin Dangoor 2011-06-15 06:27:40 PDT
After having used the Web Console a fair bit, I agree that the tab into the document is actually a bit jarring. In another bug, Marco Zehe said that Chatzilla provides a nice precedent here using F6 to get out of the field that offers tab-based autocompletion. Even if we provide right-arrow completion, tab is what a lot of people are used to (and the tab key is easier to get to on US keyboards).

My vote is to not remove the focus on tab presses, but to use F6 to get out of the field. Supporting right-arrow as well for completion would be a nice-to-have since the others support that.
Comment 22 Marco Zehe (:MarcoZ) 2011-06-15 06:51:26 PDT
Agreed. Also, Chatzilla puts up an info thing saying "there's nothing to auto-complete" if you hit tab on an empty line. A hint like this would probably be good in case there is otherwise no indication that something happened.
Comment 23 Mihai Sucan [:msucan] 2011-09-14 14:03:06 PDT
Kevin and Marco: this bug is going to be fixed by the new attachment 560245 [details] [diff] [review] from bug 673148 comment 11.
Comment 24 Jignesh Kakadiya [:jhk] 2011-12-13 09:45:00 PST
Created attachment 581310 [details] [diff] [review]
Code completion modified when input is null string

Patch_1
Comment 25 Mihai Sucan [:msucan] 2011-12-13 10:21:37 PST
Comment on attachment 581310 [details] [diff] [review]
Code completion modified when input is null string

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

Thank you for your contribution! The patch applies cleanly and it works as desired!

There's one use-case that is weird. Try this: type window.a then Tab. It says there's nothing to autocomplete, which is not entirely true. Maybe when the popup is open and nothing is selected we should say "no selection", or we could select the first item. What do you think it makes more sense?

I think this is the next step in fixing this bug, then we need a test,

::: browser/devtools/webconsole/HUDService.jsm
@@ +5263,5 @@
>              this.acceptProposedCompletion()) {
>            aEvent.preventDefault();
>          }
> +        else {
> +	  this.updateCompleteNode("There's nothing to auto-complete");

Please make this string localizable. Call HUDService.getStr() and update webconsole.properties.

See:
browser/locales/en-US/chrome/browser/devtools/webconsole.properties

Also, the string should probably be shorter. Maybe something like "nothing to complete" with a space in front so it's not shown immediately after the string I type in the input.

(or maybe "no result"? others should chime in the bug with better ideas! please do!)
Comment 26 Jignesh Kakadiya [:jhk] 2011-12-14 01:37:31 PST
Created attachment 581562 [details] [diff] [review]
test+patch

Kept "No Result".
Comment 27 Mihai Sucan [:msucan] 2011-12-14 09:58:58 PST
Comment on attachment 581562 [details] [diff] [review]
test+patch

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

Thanks for your updated patch!

This is a good start with writing test!

Still, on my system I do get failures:

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/webconsole/test/browser/browser_webconsole_bug_583816_No_input_and_Tab_key_pressed.js | an unexpected uncaught JS exception reported through window.onerror - inputNode is undefined at chrome://mochitests/content/browser/browser/devtools/webconsole/test/browser/browser_webconsole_bug_583816_No_input_and_Tab_key_pressed.js:55
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/webconsole/test/browser/browser_webconsole_bug_583816_No_input_and_Tab_key_pressed.js | Test timed out
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/webconsole/test/browser/browser_webconsole_bug_583816_tab_focus.js | inputNode is no longer focused - Didn't expect true, but got it

Please look into why these failures happen. Did you run the tests on your system? Please read:

https://developer.mozilla.org/en/Browser_chrome_tests

It explains how you can run tests on your system. Set TEST_PATH to point to the Web Console tests folder.

Thank you! Looking forward for the updated patch!

::: browser/devtools/webconsole/HUDService.jsm
@@ +5264,3 @@
>          }
> +        else {
> +	  this.updateCompleteNode(HUDService.getStr("Autocomplete.blank"));

Please add a space in front of the string.

When I type foobarbaz then I press tab "no result" shows too close to the input I typed.

::: browser/devtools/webconsole/test/browser/browser_webconsole_bug_583816_No_input_and_Tab_key_pressed.js
@@ +36,5 @@
> + * the terms of any one of the MPL, the GPL or the LGPL.
> + *
> + * ***** END LICENSE BLOCK ***** */
> +
> +const TEST_URI = "http://example.com/browser/toolkit/components/console/hudservice/tests/browser/test-console.html";

The tab that opens in this test says:

"/browser/toolkit/components/console/hudservice/tests/browser/test-console.html was not found. "

The files have moved from toolkit/components/console/hudservice/ to browser/devtools/webconsole/.

@@ +48,5 @@
> +  openConsole();
> +
> +  HUD = HUDService.getHudByWindow(content);
> +
> +  outputNode = HUD.inputNode;

outputNode = inputNode?

@@ +51,5 @@
> +
> +  outputNode = HUD.inputNode;
> +  
> +  //Test when input is empty.
> +  is(inputNode.value, "true", "inputNode is not empty");

inputNode is undefined.

Why do you expect that the inputNode.value is "true" when the Web Console is first open?

@@ +65,5 @@
> +  HUD.jsterm.setInputValue("foobarfoo");
> +  
> +  EventUtils.synthesizeKey("VK_TAB", {});
> +
> +  executeSoon(finish);

You send the tab key event but you don't check the result.
Comment 28 Jignesh Kakadiya [:jhk] 2011-12-14 12:44:01 PST
Created attachment 581752 [details] [diff] [review]
Revised test

Implemented working test.
Comment 29 Jignesh Kakadiya [:jhk] 2011-12-14 12:56:01 PST
Created attachment 581755 [details] [diff] [review]
Patch_3

"No Result" -> "no result".
Comment 30 Jignesh Kakadiya [:jhk] 2011-12-14 13:01:20 PST
Created attachment 581757 [details] [diff] [review]
patch+ test for tab pressed when no auto complete.

Some how missed one No Result .corrected!
Comment 31 Mihai Sucan [:msucan] 2011-12-15 09:35:57 PST
Comment on attachment 581757 [details] [diff] [review]
patch+ test for tab pressed when no auto complete.

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

Thanks for your update, great work!

The test is fine now - I just a suggestion to make it more complete.

Please also remove the test file browser_webconsole_bug_583816_tab_focus.js. When I run all the tests, that test fails because it expects different behavior.

Looking forward for the updated patch. Thanks!

::: browser/devtools/webconsole/test/browser/browser_webconsole_bug_583816_No_input_and_Tab_key_pressed.js
@@ +53,5 @@
> +  var input = jsterm.inputNode;
> +  
> +  jsterm.setInputValue("");
> +  EventUtils.synthesizeKey("VK_TAB", {});
> +  is(jsterm.completeNode.value, "no result", "no result matched");

Please also add a check for the input focus, so we make sure the input is still focused:

is(input.getAttribute("focused"), "true", "input is still focused");

@@ +59,5 @@
> +  //Any thing which is not in property autocompleter
> +  jsterm.setInputValue("window.Bug583816");
> +  EventUtils.synthesizeKey("VK_TAB", {});
> +  is(jsterm.completeNode.value, "                no result", "completenode content matched");
> +  is(input.value, "window.Bug583816", "input is matched")

Same as above, add a check to make sure the input still has focus.
Comment 32 Jignesh Kakadiya [:jhk] 2011-12-15 10:07:04 PST
Created attachment 582019 [details] [diff] [review]
Test+Patch

Test changes :
Tab focus to input node added.
tab_focus.js removed.
Comment 33 Mihai Sucan [:msucan] 2011-12-15 11:12:54 PST
Comment on attachment 582019 [details] [diff] [review]
Test+Patch

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

Thank you Jignesh! Your patch is almost ready to land.

Rob: I do want you to take a look at this patch and try it out. Please test the behavior of the Tab key. Is this where we want it to be? Thanks!
Comment 34 Rob Campbell [:rc] (:robcee) 2011-12-16 08:50:55 PST
Comment on attachment 582019 [details] [diff] [review]
Test+Patch

-        break;
+        else {
+	  this.updateCompleteNode(HUDService.getStr("Autocomplete.blank"));
+	}
+	aEvent.preventDefault();
+	break;

Indentation's a little funny here, but I like the behavior.

I would suggest prepending a space and/or an arrow to the "no result" completion. If it runs into a string it looks sort of odd.

e.g., "window.sno result" does not parse well
"window.s ← no result" provides a bit of separation making the text easier to read.

either way, r+ for the improved behavior. Thanks!
Comment 35 Jignesh Kakadiya [:jhk] 2011-12-16 09:57:04 PST
Created attachment 582306 [details] [diff] [review]
patch+test

patch + test

changes :
- Indentation cleared
- "no result" to " ← no result"
Comment 36 Mihai Sucan [:msucan] 2011-12-17 12:16:41 PST
Comment on attachment 582306 [details] [diff] [review]
patch+test

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

Patch looks good, thanks for your update. If I type window then Tab nothing happens, no "no result" message. Can you look into this?


The patch causes test failures now:

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/webconsole/test/browser/browser_webconsole_bug_583816_No_input_and_Tab_key_pressed.js |  ← no result - matched - Got � no result, expected  ← no result
TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/webconsole/test/browser/browser_webconsole_bug_583816_No_input_and_Tab_key_pressed.js | completenode content - matched - Got                 � no result, expected                  ← no result

Please fix the test failures.
Comment 37 Jignesh Kakadiya [:jhk] 2011-12-17 15:59:38 PST
Created attachment 582605 [details] [diff] [review]
[in-fx-team] test+patch
Comment 38 Mihai Sucan [:msucan] 2011-12-18 07:40:11 PST
Comment on attachment 582605 [details] [diff] [review]
[in-fx-team] test+patch

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

Patch looks good now. Thank you Jignesh for your contribution!
Comment 39 Mihai Sucan [:msucan] 2011-12-20 10:19:28 PST
Comment on attachment 582605 [details] [diff] [review]
[in-fx-team] test+patch

Landed:
https://hg.mozilla.org/integration/fx-team/rev/fe77d3b0aece

Thank you Jignesh for your contribution!
Comment 40 Ed Morley [:emorley] 2011-12-21 11:57:28 PST
https://hg.mozilla.org/mozilla-central/rev/fe77d3b0aece

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