Closed Bug 619598 Opened 9 years ago Closed 9 years ago

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

Categories

(DevTools :: General, defect, P3)

defect

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: mossop, Assigned: msucan)

References

Details

(Whiteboard: [console-1])

Attachments

(1 file, 3 obsolete files)

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.
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.
Assignee: nobody → pwalton
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.
Blocks: devtools4
Priority: -- → P3
Assignee: pwalton → nobody
Assignee: nobody → mihai.sucan
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.
Attached patch proposed patch (obsolete) — Splinter Review
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!
Attachment #508873 - Attachment is obsolete: true
Attachment #509215 - Flags: feedback?(rcampbell)
Status: NEW → ASSIGNED
OS: Mac OS X → All
Hardware: x86 → All
Whiteboard: [patchclean:0202]
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)
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.
(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 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!
Attachment #509215 - Flags: feedback?(rcampbell) → feedback+
(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.
(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.
Attached patch updated patch (obsolete) — Splinter Review
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.
Attachment #509215 - Attachment is obsolete: true
Attachment #510017 - Flags: review?(sdwilsh)
Whiteboard: [patchclean:0202] → [patchclean:0205]
Duplicate of this bug: 632834
Duplicate of this bug: 634408
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
Attachment #510017 - Flags: review?(sdwilsh) → review+
Thanks for the review+ Shawn! Updated the patch as requested.
Whiteboard: [patchclean:0205] → [patchclean:0309]
Attachment #510017 - Attachment is obsolete: true
Whiteboard: [patchclean:0309] → [patchclean:0309][checkin][needs approval?]
Whiteboard: [patchclean:0309][checkin][needs approval?] → [patchclean:0309][checkin][needs approval?] [console-1]
This patch still applies cleanly, even on top of the patch from bug 618311. All tests pass locally.
Whiteboard: [patchclean:0309][checkin][needs approval?] [console-1] → [patchclean:0323][checkin][console-1]
Duplicate of this bug: 618328
Whiteboard: [patchclean:0323][checkin][console-1] → [patchclean:0323][merge-m-c][console-1]
Comment on attachment 518103 [details] [diff] [review]
[in-devtools] updated patch

checked into devtools:

http://hg.mozilla.org/projects/devtools/rev/7e72b469fd10
Attachment #518103 - Attachment description: updated patch → [in-devtools] updated patch
Whiteboard: [patchclean:0323][merge-m-c][console-1] → [console-1][merge-m-c]
http://hg.mozilla.org/mozilla-central/rev/7e72b469fd10
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [console-1][merge-m-c] → [console-1]
Verified on:
Build identifier: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.2a1pre) Gecko/20110405 Firefox/4.2a1pre
Status: RESOLVED → VERIFIED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.