1472161 - Hitting <kbd>Tab</kbd> in JsTerm could be more helpful

Status: RESOLVED FIXED

(DevTools :: Console, enhancement, P1)

Description

[Nicolas Chevobbe [:nchevobbe]]

At the moment, here's the strategy on the console input when the user hit the Tab key: 

- If the console is empty, we show a "<- no result" shadow text
- If the console contains only whitespaces, nothing is done
- If the user entered a non-completable string (e.g. "zwx" where no variable/function "zwx" exist in content), we show the "<- no result" shadow text
- If the autocomplete popup or autocompletion shadow text is displayed, we insert the selected item in the input

I think this is sub-optimal and we could do a better job here: 

- If console is empty, tab out
- If console has some text, but no autocomplete popup / text shown, insert an actual tab (or spaces if the user has the "Indent using spaces" preference set to true)
- if autocomplete popup / text shown, complete with selected element

Comment 1

[Brian Grinstead [:bgrins]]

I agree that the `this._inputChanged` check can lead to some inconsistent behavior. Although this was discussed in some detail in Bug 583816 so I don't want to change it without revisiting intended behavior. Marco, here are a couple of specific questions based off of Comment 0:

> If console is empty, tab out
1) When you type something in the input but then backspace it out so the field is empty again and then press <TAB> - right now that will show the "No results" message and preventDefault. But if the field is empty I'd think we should go ahead and move focus (regardless of whether input has been entered in the past). Does that make sense?

> - If console has some text, but no autocomplete popup / text shown, insert an actual tab
> (or spaces if the user has the "Indent using spaces" preference set to true)
2) What do you think about that? I frequently do something where I type `function foo() {<ENTER>` and then press <TAB> in an attempt to indent the block of the body but instead get a kind of flickering "No result" message / nothing happening. At least for that use-case having <TAB> insert a tab into the input field does seem more predictable. I don't think <TAB> is necessarily expected to autocomplete in the console unless if suggestions are visible.

Comment 2

[Brian Grinstead [:bgrins]]

Attachment: Bug 1472161 - Enter tabs in console when not at an autocomplete location;

Review commit: https://reviewboard.mozilla.org/r/254528/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/254528/

Comment 3

[Code Review Bot [:reviewbot]]

Comment on attachment 8989479 [details]
Bug 1472161 - Enter tabs in console when not at an autocomplete location;

https://reviewboard.mozilla.org/r/254528/#review261352


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/client/webconsole/components/JSTerm.js:1277
(Diff revision 1)
>    acceptProposedCompletion() {
>      let updated = false;
>  
>      const currentItem = this.autocompletePopup.selectedItem;
>      if (currentItem && this.lastCompletion.value) {
> -      const suffix =
> +      this.insertStringAtCursor(currentItem.label.substring(this.lastCompletion.matchProp.length));

Error: Line 1277 exceeds the maximum line length of 90. [eslint: max-len]

Comment 4

[Brian Grinstead [:bgrins]]

I put up an example of the behavior described in Comment 0. Here's a try push with binaries if you'd like to test it: https://treeherder.mozilla.org/#/jobs?repo=try&revision=001e3ec55db54fc3b935ac779da76b400086d7ce

Comment 5

[Marco Zehe (:MarcoZ)]

(In reply to Brian Grinstead [:bgrins] from comment #1)
> > If console is empty, tab out
> 1) When you type something in the input but then backspace it out so the
> field is empty again and then press <TAB> - right now that will show the "No
> results" message and preventDefault. But if the field is empty I'd think we
> should go ahead and move focus (regardless of whether input has been entered
> in the past). Does that make sense?

Yes, this makes total sense.

> > - If console has some text, but no autocomplete popup / text shown, insert an actual tab
> > (or spaces if the user has the "Indent using spaces" preference set to true)
> 2) What do you think about that? I frequently do something where I type
> `function foo() {<ENTER>` and then press <TAB> in an attempt to indent the
> block of the body but instead get a kind of flickering "No result" message /
> nothing happening. At least for that use-case having <TAB> insert a tab into
> the input field does seem more predictable. I don't think <TAB> is
> necessarily expected to autocomplete in the console unless if suggestions
> are visible.

Agree with that, too. If something is entered which clearly expects more input, and we can reliably determine that, having tab indent does make sense. Otherwise, we'd have to devise a different way of doing multiline things in the console, e. g. Shift (or cmd on mac) + Enter to insert new lines etc. So if it is clearly detectable that what was previously entered expects more input, then yes, tab should indent, not move out of the field. I guess we'll have to find out over time whether our assumptions prove to be reliable in this case.

Comment 6

[Brian Grinstead [:bgrins]]

Comment on attachment 8989479 [details]
Bug 1472161 - Enter tabs in console when not at an autocomplete location;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254528/diff/1-2/

Comment 7

[Code Review Bot [:reviewbot]]

Comment on attachment 8989479 [details]
Bug 1472161 - Enter tabs in console when not at an autocomplete location;

https://reviewboard.mozilla.org/r/254528/#review262316


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/client/webconsole/components/JSTerm.js:1268
(Diff revision 2)
>    acceptProposedCompletion() {
>      let updated = false;
>  
>      const currentItem = this.autocompletePopup.selectedItem;
>      if (currentItem && this.lastCompletion.value) {
> -      const suffix =
> +      this.insertStringAtCursor(currentItem.label.substring(this.lastCompletion.matchProp.length));

Error: Line 1268 exceeds the maximum line length of 90. [eslint: max-len]

Comment 8

[Brian Grinstead [:bgrins]]

Comment on attachment 8989479 [details]
Bug 1472161 - Enter tabs in console when not at an autocomplete location;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254528/diff/2-3/

Comment 9

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8989479 [details]
Bug 1472161 - Enter tabs in console when not at an autocomplete location;

https://reviewboard.mozilla.org/r/254528/#review262436

This looks good to me and simplifies the code, which is nice !

::: devtools/client/webconsole/components/JSTerm.js:862
(Diff revision 3)
> -          this.updateCompleteNode(l10n.getStr("Autocomplete.blank"));
> +          if (!event.shiftKey) {
> +            this.insertStringAtCursor("\t");
> +          }
>            event.preventDefault();

I wonder if we should preventDefault when the user does a shift + tab.

I'd expect it to move the focus to the previous element, but I don't have strong feeling about it.

Also, it seems a bit weird to have Shift + Tab trigger the autocompletion. This is unrelated to this patch an can go into its own bug, but to me, I feel like we should treat as Esc when there's the popup/autocomplete shown.

::: devtools/client/webconsole/test/mochitest/browser_jsterm_no_input_and_tab_key_pressed.js:25
(Diff revision 3)
>  
> +  // With empty input, tab through
>    jsterm.setInputValue("");
>    EventUtils.synthesizeKey("KEY_Tab");
> -  is(jsterm.completeNode.value, "<- no result", "<- no result - matched");
> -  is(input.value, "", "inputnode is empty - matched");
> +  is(jsterm.getInputValue(), "", "inputnode is empty - matched");
> +  ok(!hasFocus(input), "input is still focused");

nit: s/is still focused/isn't focused anymore

Comment 10

[Brian Grinstead [:bgrins]]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #9)
> Comment on attachment 8989479 [details]
> Bug 1472161 - Enter tabs in console when not at an autocomplete location;
> 
> https://reviewboard.mozilla.org/r/254528/#review262436
> 
> This looks good to me and simplifies the code, which is nice !
> 
> ::: devtools/client/webconsole/components/JSTerm.js:862
> (Diff revision 3)
> > -          this.updateCompleteNode(l10n.getStr("Autocomplete.blank"));
> > +          if (!event.shiftKey) {
> > +            this.insertStringAtCursor("\t");
> > +          }
> >            event.preventDefault();
> 
> I wonder if we should preventDefault when the user does a shift + tab.
> 
> I'd expect it to move the focus to the previous element, but I don't have
> strong feeling about it.
> 
> Also, it seems a bit weird to have Shift + Tab trigger the autocompletion.
> This is unrelated to this patch an can go into its own bug, but to me, I
> feel like we should treat as Esc when there's the popup/autocomplete shown.

Yeah - I wasn't sure what to do with Shift+Tab. I kept it as preventing default and autocompleting to be more consistent with the current implementation. I'll file a follow up bug to change this if we want to. Marco, a couple more questions:

1) Do you have an opinion on if Shift+Tab should always have the normal backwards-focus behavior (even when there is text in the field such that Tab on it's own doesn't change focus)?
2) If the autocomplete popup is opened and a user presses Shift+Tab, should we accept the selected completion, close the popup, or do nothing / something else?

Comment 12

[Brian Grinstead [:bgrins]]

Comment on attachment 8989479 [details]
Bug 1472161 - Enter tabs in console when not at an autocomplete location;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254528/diff/3-4/

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3a5920b19241
Enter tabs in console when not at an autocomplete location;r=nchevobbe

Comment 14

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/3a5920b19241

Comment 15

[Marco Zehe (:MarcoZ)]

(In reply to Brian Grinstead [:bgrins] from comment #10)
> Marco, a couple more questions:
> 
> 1) Do you have an opinion on if Shift+Tab should always have the normal
> backwards-focus behavior (even when there is text in the field such that Tab
> on it's own doesn't change focus)?

I actually don't. I think it would be OK for it to move focus backwards in this instance so you may be more easily able to review what the last console output was with a screen reader, even if you've typed something.

> 2) If the autocomplete popup is opened and a user presses Shift+Tab, should
> we accept the selected completion, close the popup, or do nothing /
> something else?

I'm a bit lost on this one. I've actually never tried that. Bringing in Jamie for further opinion as a fellow screen reader and keyboard user.

Comment 16

[James Teh [:Jamie]]

(In reply to Brian Grinstead [:bgrins] from comment #10)
> 1) Do you have an opinion on if Shift+Tab should always have the normal
> backwards-focus behavior (even when there is text in the field such that Tab
> on it's own doesn't change focus)?

I think this makes sense. Aside from Marco's point of making review easier, it feels weird to be "trapped" in the field with no way of tabbing out, and while tab is obviously used for completion, I can't think of any good reason to use shift+tab for completion.

> 2) If the autocomplete popup is opened and a user presses Shift+Tab, should
> we accept the selected completion, close the popup, or do nothing /
> something else?

That's tricky. I don't honestly think it's something people are likely to do intentionally. That said, if 1) is implemented, I think it makes sense to be consistent so that shift+tab *never* completes. It might be confusing if it does complete while the pop-up is open, but doesn't complete when it isn't. So, I'd dismiss the pop-up. I don't have a "strong" feeling about this, though.