Closed Bug 583816 Opened 14 years ago Closed 12 years ago

Tab should not move to the page when there's nothing to complete in the Web Console

Categories

(DevTools :: Console, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 12

People

(Reporter: enndeakin, Assigned: jhk)

References

Details

(Keywords: access, polish, Whiteboard: [fixed-in-fx-team])

Attachments

(2 files, 9 obsolete files)

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!
The tab key is used for completion. Therefore it can't be used to set the focus to the next element.
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.
(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.
Whiteboard: [kd4b4]
(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.
Marco, this probably came up in Chatzilla. What was the decision there?
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.
(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.
Whiteboard: [kd4b4] → [kd4b5]
Blocks: console-a11y
Assignee: nobody → jviereck
Whiteboard: [kd4b5]
Assignee: julian.viereck → pwalton
Status: NEW → ASSIGNED
Summary: Tab key does nothing when Web Console textfield is focused → F6 should move from the page to the Web Console input field
Changing the title of the bug to reflect what the consensus now seems to be.
Summary: F6 should move from the page to the Web Console input field → Tab should move to the page when there's nothing to complete in the Web Console
Assignee: pwalton → mihai.sucan
Blocks: devtools4b8
Attached patch proposed patch + test code (obsolete) — Splinter Review
Proposed patch and test code.
Attachment #474030 - Flags: feedback?(rcampbell)
Comment on attachment 474030 [details] [diff] [review]
proposed patch + test code

Looks good. Should work as-advertised.
Attachment #474030 - Flags: feedback?(rcampbell) → feedback+
blocking2.0: --- → ?
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!
Attachment #474030 - Flags: review?(sdwilsh)
Great polish, not a blocker.
blocking2.0: ? → -
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
Attachment #474030 - Flags: review?(sdwilsh) → review+
Attached patch updated patch (obsolete) — Splinter Review
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!
Attachment #474030 - Attachment is obsolete: true
Attachment #475034 - Flags: approval2.0?
Whiteboard: [patchclean:0914]
adding polish flag. Would like approval for this. Adding blocking request to get it triaged.
blocking2.0: - → ?
Keywords: polish
Attachment #475034 - Flags: approval2.0? → approval2.0+
Keywords: checkin-needed
Attached patch rebased patchSplinter Review
Rebased patch on top of today's mozilla-central default branch. Only test code changes were needed.
Attachment #475034 - Attachment is obsolete: true
Whiteboard: [patchclean:0914] → [patchclean:1011]
http://hg.mozilla.org/mozilla-central/rev/5b8dcf82b27a
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [patchclean:1011]
Nice polish win, but if were re-opened today, I don't think we should hold back the release for it. blocking-.
blocking2.0: ? → -
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?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
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.
Kevin and Marco: this bug is going to be fixed by the new attachment 560245 [details] [diff] [review] from bug 673148 comment 11.
Patch_1
Attachment #581310 - Flags: review?(mihai.sucan)
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!)
Attachment #581310 - Flags: review?(mihai.sucan)
Attached patch test+patch (obsolete) — Splinter Review
Kept "No Result".
Attachment #581310 - Attachment is obsolete: true
Attachment #581562 - Flags: review?(mihai.sucan)
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.
Attachment #581562 - Flags: review?(mihai.sucan)
Assignee: mihai.sucan → jigneshhk1992
Component: Developer Tools → Developer Tools: Console
QA Contact: developer.tools → developer.tools.console
Summary: Tab should 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 Console
Version: unspecified → Trunk
Attached patch Revised test (obsolete) — Splinter Review
Implemented working test.
Attachment #581562 - Attachment is obsolete: true
Attachment #581752 - Flags: review?(mihai.sucan)
Attached patch Patch_3 (obsolete) — Splinter Review
"No Result" -> "no result".
Attachment #581752 - Attachment is obsolete: true
Attachment #581755 - Flags: review?(mihai.sucan)
Attachment #581752 - Flags: review?(mihai.sucan)
Some how missed one No Result .corrected!
Attachment #581755 - Attachment is obsolete: true
Attachment #581757 - Flags: review?(mihai.sucan)
Attachment #581755 - Flags: review?(mihai.sucan)
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.
Attachment #581757 - Flags: review?(mihai.sucan) → review+
Attached patch Test+Patch (obsolete) — Splinter Review
Test changes :
Tab focus to input node added.
tab_focus.js removed.
Attachment #581757 - Attachment is obsolete: true
Attachment #582019 - Flags: review?(mihai.sucan)
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!
Attachment #582019 - Flags: review?(rcampbell)
Attachment #582019 - Flags: review?(mihai.sucan)
Attachment #582019 - Flags: review+
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!
Attachment #582019 - Flags: review?(rcampbell) → review+
Attached patch patch+test (obsolete) — Splinter Review
patch + test

changes :
- Indentation cleared
- "no result" to " ← no result"
Attachment #582019 - Attachment is obsolete: true
Attachment #582306 - Flags: review?(mihai.sucan)
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.
Attachment #582306 - Flags: review?(mihai.sucan)
Attachment #582306 - Attachment is obsolete: true
Attachment #582605 - Flags: review?(mihai.sucan)
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!
Attachment #582605 - Flags: review?(mihai.sucan) → review+
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!
Attachment #582605 - Attachment description: test+patch → [in-fx-team] test+patch
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/fe77d3b0aece
Status: REOPENED → RESOLVED
Closed: 14 years ago12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 12
Product: Firefox → DevTools
blocking2.0: - → ---
See Also: → 1472161
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: