619598 - Up and Down does not retrieve command history in the same way as other consoles

Status: VERIFIED FIXED

(DevTools :: General, defect, P3)

Description

[Dave Townsend [:mossop]]

If I push up then the previous entered command is shown bu the cursor is then at the start of the line. On ever other console it would be at the end. Pushing down takes you to the end of the line when I would expect it to move you to the next command.

It seems that if you are at the start of the line pushing up will move to the previous command in history otherwise pushing up will move the cursor to the start of the line.

If you are at the end of the list then pushing down will move to the next command in history otherwise pushing down will move the cursor to the end of the line.

This behaviour all seems broken compared to other consoles.

Comment 1

[Kevin Dangoor]

From talking with Patrick about this last week, it sounded like the current behavior is in place to deal with some aspects of the command line accepting multi-line input. I've assigned this to Patrick to at least comment on and see if we can work out if there are better feasible approaches.

Comment 2

[Patrick Walton (:pcwalton)]

This is done to work around the fact that up and down have an overloaded meaning with multiline input. For example, up can mean "go to the previous line" and can also mean "go back in the history". We distinguish the two cases by going back in the history if and only if the cursor is at the start of the text field.

To fix this we would need a way to determine whether the cursor is on the first line of a multi-line input box.

Comment 3

[Adam]

Attachment: Split the value on new lines, and check if cursor is on start or end line.

This patch puts the behavior in parity with Chromium's console and other consoles I've used.

It uses String.split('\n') to detect the line position of the cursor when there are multiple lines.  The only problem is with long lines that wrap, which are still treated as single lines.  Chromium's console behaves in the same way, as do terminal-based consoles (Bash, Python, etc.), but whether this is acceptable for the Web Console may need to be discussed.

Comment 4

[Mihai Sucan [:msucan]]

Attachment: proposed patch

This is the proposed patch.

Problem description: currently the code always checks if the caret is at the very start or at the very end of the input string (even if it's single line or multi-line, doesn't matter). If that is the case, then the user goes up/down in the history.

The above problem is also coupled with code that does not put the caret at the end of the input value ... when you go up in the history.

We do not check if the user is in the first line (or last line) of the input because we do not want to go up/down in the history when a really long line is wrapped. Just doing that check would effectively prevent the user from going up/down in a single very long line that is visually wrapped on multiple lines. See bug 594497.

The entire situation makes the command line feel unnatural, compared to other command lines.

The situation in Chromium is as follows: they check if the caret is in the first or last line, they do not deal with single lines that visually wrap, so you always navigate through the history.

The proposed fix and solution does the following:

- always moves the caret at the end of input, when you go up/down in the history.

- goes up/down through history if the caret is at location 0 or at the end (value.length).

- if you are somewhere in the middle of a single line (first/last doesn't matter), the default browser behavior is allowed. This allows you to move in a very long single line, even if it's the first/last line of the input string.

Overall, this makes the Web Console feel natural (you can go up/down as you are used in a normal console), while when you are in the middle of the line ... you can move even if it's wrapped.

(Rather than reading this comment, testing the patch as a user will show it's better and more natural. :) )

Looking forward for the feedback. Thanks!

Comment 5

[Mihai Sucan [:msucan]]

Comment on attachment 508873 [details] [diff] [review]
Split the value on new lines, and check if cursor is on start or end line.

Feedback for Adam's patch:

+            if (aEvent.cancelable && self.caretAtFirstLineOfInput()) {

Why do you make this change?

+    let lines = this.inputNode.value.split('\n');
+    let startPosition = this.inputNode.selectionStart;
+    return startPosition == this.inputNode.selectionEnd &&
+        (lines.length == 1 || startPosition <= lines[0].length);

I think it would be better to do let pos = this.inputNode.value.indexOf("\n") and check if this.inputNode.selectionStart <= pos. This would be a quicker way to check if the caret is within the first line, without splitting the whole value into an array of lines.

The same goes for caretAtLastLineOfInput().

(you do not need to update the patch)

Comment 6

[Adam]

Thank you for the feedback, Mihai.

The first change was a wrong assumption on my part: the neighboring cases different uses of checking/not checking aEvent.cancelable before calling aEvent.preventDefault() confused me a bit.

As I understand it now, the extra checking of cancelability is only needed if something that depends on canceling is called (like the repositioning of the caret after the update), correct?  It doesn't matter either way if only preventDefault() is called.

For the proposed patch, the use case that's still missing is the ability to keep going up in the history on a single press if you run into a multi-line input.

Why not go with a single "checkCaretCanGoHistory" with the return being:

return node.selectionStart == 0 || node.selectionStart == node.value.length;

That would be simpler, since the user has to move the caret left before moving within a long line, they would get the same for multi-line.

Comment 7

[Mihai Sucan [:msucan]]

(In reply to comment #6)
> Thank you for the feedback, Mihai.
> 
> The first change was a wrong assumption on my part: the neighboring cases
> different uses of checking/not checking aEvent.cancelable before calling
> aEvent.preventDefault() confused me a bit.
> 
> As I understand it now, the extra checking of cancelability is only needed if
> something that depends on canceling is called (like the repositioning of the
> caret after the update), correct?  It doesn't matter either way if only
> preventDefault() is called.

Yes. The call to checkCaretCanGoHistoryBack() tells if the user can navigate through the history, then the historyPeruse() method tells us if there has been any update to the input field value. If that's the case, and if the event default action can be canceled, then we call preventDefault().


> For the proposed patch, the use case that's still missing is the ability to
> keep going up in the history on a single press if you run into a multi-line
> input.
> 
> Why not go with a single "checkCaretCanGoHistory" with the return being:
> 
> return node.selectionStart == 0 || node.selectionStart == node.value.length;
> 
> That would be simpler, since the user has to move the caret left before moving
> within a long line, they would get the same for multi-line.

Indeed, good point. This idea came to me after I submitted the patch. :)

Let's see what other changes Robert wants. The change you suggested needs to be included. Thanks!

Comment 8

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 509215 [details] [diff] [review]
proposed patch

             // history previous
-            if (self.caretAtStartOfInput()) {
+            if (self.checkCaretCanGoHistoryBack()) {

not a huge fan of that method name. Maybe canCaretGoPrevious() ?

...

   /**
-   * Check if the caret is at the start of the input.
+   * Check if the caret is at a location that can allow going back in history
+   * when the user presses the Up arrow key.

supernit:
... location that allows selecting previous item in history ...

    * @returns boolean
-   *          True if the caret is at the start of the input.
+   *          True if the caret is at a location that is acceptable for going

same or similar wording. Do we need to repeat what we're returning?

+   *          back in the history, false otherwise.
    */
-  caretAtStartOfInput: function JST_caretAtStartOfInput()
+  checkCaretCanGoHistoryBack: function JST_caretAtStartOfInput()

nit: method name as above, caretCanGoPrevious().

Not a nit: Method signatures have to match their property names. JST_caret...

-    return this.inputNode.selectionStart == this.inputNode.selectionEnd &&
-        this.inputNode.selectionStart == 0;
+    let node = this.inputNode;
+    if (node.selectionStart != node.selectionEnd) {
+      return false;
+    }

nit: add a comment here explaining what this is. "Is there any text selected?"

+    let multiline = /[\r\n]/.test(node.value);
+    return node.selectionStart == 0 ? true :
+           node.selectionStart == node.value.length && !multiline;
   },

Do we have any /r in the text area? If not, we can make multiline =  indexOf("\n") != -1.

-   * Check if the caret is at the end of the input.
+   * Check if the caret is at a location that can allow going forward in history

nit! ... location that allows selecting next item in ...

+   * when the user presses the Down arrow key.
    *
    * @returns boolean
-   *          True if the caret is at the end of the input, or false otherwise.
+   *          True if the caret is at a location that is acceptable for going

... at a location that allows selecting...

Not even sure we need to repeat ourselves in the return portion of these comments.

+   *          forward in the history, false otherwise.
    */
-  caretAtEndOfInput: function JST_caretAtEndOfInput()
+  checkCaretCanGoHistoryForward: function JST_caretAtEndOfInput()

how about canCaretGoNext. Make sure the function name matches!

...
+    let multiline = /[\r\n]/.test(node.value);

same as above re: \r.

Tests look good.

f+ with the above fixed!

Comment 9

[Mihai Sucan [:msucan]]

(In reply to comment #8)
> Comment on attachment 509215 [details] [diff] [review]
> proposed patch
> 
> +    let multiline = /[\r\n]/.test(node.value);
> +    return node.selectionStart == 0 ? true :
> +           node.selectionStart == node.value.length && !multiline;
>    },
> 
> Do we have any /r in the text area? If not, we can make multiline = 
> indexOf("\n") != -1.

Afaik, we had code that dealt with multiline input in the textarea, code which assumed only \n is used, and there was a bug about making it work on mac/windows (\r). I think it's safe to assume we can have \r.

Thanks for the feedback+! Will update the patch.

Comment 10

[Mihai Sucan [:msucan]]

(In reply to comment #6)
> For the proposed patch, the use case that's still missing is the ability to
> keep going up in the history on a single press if you run into a multi-line
> input.
> 
> Why not go with a single "checkCaretCanGoHistory" with the return being:
> 
> return node.selectionStart == 0 || node.selectionStart == node.value.length;
> 
> That would be simpler, since the user has to move the caret left before moving
> within a long line, they would get the same for multi-line.

Tested this. I prefer the behavior in the current patch, actually. This is because in a multiline input I want to be able to press Up and move to the previous line, when I am at the end of the last line. With the change you suggested the code goes up in history.

Comment 11

[Mihai Sucan [:msucan]]

Attachment: updated patch

Updated the patch based on Robert's feedback. Thanks!

Asking for review from Shawn. This is a patch that polishes the Web Console functionality, and we'd like it pushed into Firefox 4.

Comment 14

[Shawn Wilsher :sdwilsh]

Comment on attachment 510017 [details] [diff] [review]
updated patch

>+++ b/toolkit/components/console/hudservice/HUDService.jsm
>   /**
>-   * Check if the caret is at the start of the input.
>+   * Check if the caret is at a location that allows selecting the previous item
>+   * in history when the user presses the Up arrow key.
>    *
>    * @returns boolean
>-   *          True if the caret is at the start of the input.
global-nit: it's @return (not plural).  If you are changing the java-doc please correct it.
nit: please provide a description for what is returned

>   /**
>-   * Check if the caret is at the end of the input.
>+   * Check if the caret is at a location that allows selecting the next item in
>+   * history when the user presses the Down arrow key.
>    *
>    * @returns boolean
>-   *          True if the caret is at the end of the input, or false otherwise.
ditto

r=sdwilsh

Comment 15

[Mihai Sucan [:msucan]]

Attachment: [in-devtools] updated patch

Thanks for the review+ Shawn! Updated the patch as requested.

Comment 16

[Mihai Sucan [:msucan]]

This patch still applies cleanly, even on top of the patch from bug 618311. All tests pass locally.

Comment 18

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 518103 [details] [diff] [review]
[in-devtools] updated patch

checked into devtools:

http://hg.mozilla.org/projects/devtools/rev/7e72b469fd10

Comment 19

[Rob Campbell [:rc] (:robcee)]

http://hg.mozilla.org/mozilla-central/rev/7e72b469fd10

Comment 20

[Simona Badau, Desktop QA]

Verified on:
Build identifier: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.2a1pre) Gecko/20110405 Firefox/4.2a1pre