594497 - Up and down keyboard presses should only trigger history when the cursor is at the start or end in the Web Console

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Patrick Walton (:pcwalton)]

Currently, the up and down arrow keys move through the history if the cursor is before the first newline or after the last newline. This isn't actually the proper logic: because the console input box has word wrap (or will once bug 588967 lands), the cursor can be before the first newline but actually on e.g. the second line.

I propose making the cursor only trigger a history action when it's at the very start or end of the contents of the input field. This enables the proper behavior when the cursor is on a word-wrapped line, and keeps the same behavior for the most common history case (pressing up when there is no input in the field).

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Potentially useful reference to what the autocomplete code does here:
http://hg.mozilla.org/mozilla-central/annotate/421427ca9809/toolkit/components/autocomplete/src/nsAutoCompleteController.cpp#l446

Comment 2

[Mihai Sucan [:msucan]]

Attachment: proposed fix and test code

Proposed fix and test code.

Thanks Gavin for your link. I had the fix already done on Friday evening, only got to write the test code today. I hope what I did is fine. To me, as a user, it's fine.

Looking forward to your feedback, Patrick.

Comment 3

[Patrick Walton (:pcwalton)]

Comment on attachment 474526 [details] [diff] [review]
proposed fix and test code

>diff --git a/toolkit/components/console/hudservice/HUDService.jsm b/toolkit/components/console/hudservice/HUDService.jsm
>+    let result = false;
>+    if (node.selectionStart == node.selectionEnd &&
>+        node.selectionStart == 0) {
>+      result = true;
>+    }
>+    return result;

How about just:
    return node.selectionStart == node.selectionEnd &&
        node.selectionStart == 0;

>+    let result = false;
>+    if (node.selectionStart == node.selectionEnd &&
>+        node.selectionStart == node.value.length) {
>+      result = true;
>+    }
>+    return result;

Likewise.

>   },
> 
>   history: [],
> 
>   // Stores the data for the last completion.
>   lastCompletion: null,
> 
>   /**
>diff --git a/toolkit/components/console/hudservice/tests/browser/Makefile.in b/toolkit/components/console/hudservice/tests/browser/Makefile.in
>--- a/toolkit/components/console/hudservice/tests/browser/Makefile.in
>+++ b/toolkit/components/console/hudservice/tests/browser/Makefile.in
>@@ -49,16 +49,17 @@ _BROWSER_TEST_FILES = \
> 	browser_HUDServiceTestsAll.js \
> 	browser_webconsole_bug_585237_line_limit.js \
> 	browser_webconsole_bug_586388_select_all.js  \
> 	browser_webconsole_bug_588967_input_expansion.js \
> 	browser_webconsole_bug_580454_timestamp_l10n.js \
> 	browser_webconsole_netlogging.js \
> 	browser_webconsole_bug_593003_iframe_wrong_hud.js \
> 	browser_webconsole_bug_581231_close_button.js \
>+	browser_webconsole_bug_594497_history_arrow_keys.js \
> 	$(NULL)
> 
> _BROWSER_TEST_PAGES = \
> 	test-console.html \
> 	test-network.html \
> 	test-network-request.html \
> 	test-mutation.html \
> 	testscript.js \
>diff --git a/toolkit/components/console/hudservice/tests/browser/browser_webconsole_bug_594497_history_arrow_keys.js b/toolkit/components/console/hudservice/tests/browser/browser_webconsole_bug_594497_history_arrow_keys.js
>+function test() {
>+  waitForExplicitFinish();
>+
>+  gBrowser.selectedBrowser.addEventListener("load", tabLoad, true);
>+
>+  content.location = TEST_URI;
>+}

I've been following the convention of opening a new tab at the start and then closing the tab at the end of the test recently, since it seems like most of the browser and toolkit tests do that.

f+ with that

Comment 4

[Mihai Sucan [:msucan]]

(In reply to comment #3)
> Comment on attachment 474526 [details] [diff] [review]
> proposed fix and test code
> 
> >diff --git a/toolkit/components/console/hudservice/HUDService.jsm b/toolkit/components/console/hudservice/HUDService.jsm
> >+    let result = false;
> >+    if (node.selectionStart == node.selectionEnd &&
> >+        node.selectionStart == 0) {
> >+      result = true;
> >+    }
> >+    return result;
> 
> How about just:
>     return node.selectionStart == node.selectionEnd &&
>         node.selectionStart == 0;

Ha! Good one. The code went through a few iterations and obviously at the end I forgot to put all nicely as you ask. Will fix!

> >   },
> > 
> >   history: [],
> > 
> >   // Stores the data for the last completion.
> >   lastCompletion: null,
> > 
> >   /**
> >diff --git a/toolkit/components/console/hudservice/tests/browser/Makefile.in b/toolkit/components/console/hudservice/tests/browser/Makefile.in
> >--- a/toolkit/components/console/hudservice/tests/browser/Makefile.in
> >+++ b/toolkit/components/console/hudservice/tests/browser/Makefile.in
> >@@ -49,16 +49,17 @@ _BROWSER_TEST_FILES = \
> > 	browser_HUDServiceTestsAll.js \
> > 	browser_webconsole_bug_585237_line_limit.js \
> > 	browser_webconsole_bug_586388_select_all.js  \
> > 	browser_webconsole_bug_588967_input_expansion.js \
> > 	browser_webconsole_bug_580454_timestamp_l10n.js \
> > 	browser_webconsole_netlogging.js \
> > 	browser_webconsole_bug_593003_iframe_wrong_hud.js \
> > 	browser_webconsole_bug_581231_close_button.js \
> >+	browser_webconsole_bug_594497_history_arrow_keys.js \
> > 	$(NULL)
> > 
> > _BROWSER_TEST_PAGES = \
> > 	test-console.html \
> > 	test-network.html \
> > 	test-network-request.html \
> > 	test-mutation.html \
> > 	testscript.js \
> >diff --git a/toolkit/components/console/hudservice/tests/browser/browser_webconsole_bug_594497_history_arrow_keys.js b/toolkit/components/console/hudservice/tests/browser/browser_webconsole_bug_594497_history_arrow_keys.js
> >+function test() {
> >+  waitForExplicitFinish();
> >+
> >+  gBrowser.selectedBrowser.addEventListener("load", tabLoad, true);
> >+
> >+  content.location = TEST_URI;
> >+}
> 
> I've been following the convention of opening a new tab at the start and then
> closing the tab at the end of the test recently, since it seems like most of
> the browser and toolkit tests do that.

Hm, as I noticed we get different tabs for each test. But, hey, I'm fine with the change. Will open a new tab and will remove it after.

> f+ with that

Thanks for the f+! Will update the patch ASAP.

Comment 5

[Mihai Sucan [:msucan]]

Attachment: updated patch

Updated patch, based on the feedback+ comments. Thanks Patrick!

Asking for review from Shawn now.

Comment 6

[Shawn Wilsher :sdwilsh]

(In reply to comment #4)
> Hm, as I noticed we get different tabs for each test. But, hey, I'm fine with
> the change. Will open a new tab and will remove it after.
Yes, we should always be doing this.  I don't think I've been checking in the past though...

Comment 7

[Shawn Wilsher :sdwilsh]

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

>+  /**
>+   * Go up/down the history stack of input values.
>+   *
>+   * @param boolean aUpFlag flag for going up in the stack.
style in this file is to have a newline after "aUpFlag" before the descriptoin.  Please remain consistent.

Also, I think this would be much more clearer if you had two constants you passed into this method: HISTORY_BACK and HISTORY_FORWARD (or something like that.).  True and false don't convey anything about the work you are doing at the call sites, so it's not really clear what is going on.

r=sdwilsh with those changes.  Please get feedback+ from one of the dev tools folks after making that change to make sure they are OK with it, but it will not need further review.

Comment 8

[Mihai Sucan [:msucan]]

Attachment: updated patch

Thanks Shawn for your review+!

This is the updated patch with the requested changes. Asking again for feedback from Patrick.

Comment 9

[Patrick Walton (:pcwalton)]

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

>diff --git a/toolkit/components/console/hudservice/HUDService.jsm b/toolkit/components/console/hudservice/HUDService.jsm
>--- a/toolkit/components/console/hudservice/HUDService.jsm
>+++ b/toolkit/components/console/hudservice/HUDService.jsm
>@@ -4319,68 +4322,95 @@ JSTerm.prototype = {
>+    let result = true;
>+
>     // Up Arrow key
>-    if (aFlag) {
>+    if (aDirection == HISTORY_BACK) {
>       if (this.historyPlaceHolder <= 0) {
>-        return;
>+        return false;
>       }
> 
>       let inputVal = this.history[--this.historyPlaceHolder];
>       if (inputVal){
>         this.setInputValue(inputVal);
>       }
>     }
>     // Down Arrow key
>-    else {
>+    else if (aDirection == HISTORY_FORWARD) {
>       if (this.historyPlaceHolder == this.history.length - 1) {
>         this.historyPlaceHolder ++;
>         this.setInputValue("");
>-        return;
>       }
>       else if (this.historyPlaceHolder >= (this.history.length)) {
>-        return;
>+        result = false;

How about "return false;"?

>       }
>       else {
>         let inputVal = this.history[++this.historyPlaceHolder];
>         if (inputVal){
>           this.setInputValue(inputVal);
>         }
>       }
>     }
>+    else {
>+      throw new Error("Invalid argument 0");
>+    }
>+
>+    return result;

"return true"; Then "result" won't be needed.

f=me with that.

Comment 10

[Shawn Wilsher :sdwilsh]

(In reply to comment #9)
> How about "return false;"?
[snip]
> "return true"; Then "result" won't be needed.
Good catch, and I should have caught this myself.  Mozilla code style prefers early returns always :)

Comment 11

[Mihai Sucan [:msucan]]

(In reply to comment #10)
> (In reply to comment #9)
> > How about "return false;"?
> [snip]
> > "return true"; Then "result" won't be needed.
> Good catch, and I should have caught this myself.  Mozilla code style prefers
> early returns always :)

Sure, will do that. I had reviewers complaining about if () { return } else { return }, and this is why I did it like that. I have no specific preference for either approach.

Comment 12

[Mihai Sucan [:msucan]]

Attachment: updated patch

Updated patch. Thanks Patrick for your feedback+!

Asking for approval2.0+ because this patch already has f+ and r+. This patch polishes the Web Console and we'd like it checked-in. Thanks!

Comment 13

[Shawn Wilsher :sdwilsh]

(In reply to comment #11)
> Sure, will do that. I had reviewers complaining about if () { return } else {
> return }, and this is why I did it like that. I have no specific preference for
> either approach.
In that case, they would have been complaining about having the else at all.  The style guide prefers |if () { return } return|

Comment 14

[Mihai Sucan [:msucan]]

Attachment: [checked-in] rebased patch

Rebased patch on top of the latest mozilla-central.

Comment 15

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

Comment on attachment 482949 [details] [diff] [review]
[checked-in] rebased patch

http://hg.mozilla.org/mozilla-central/rev/5358162f8111