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

RESOLVED FIXED

Status

()

Firefox
Developer Tools
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: pcwalton, Assigned: msucan)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 beta8+)

Details

(Whiteboard: [patchclean:1013])

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

8 years ago
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).
(Reporter)

Updated

8 years ago
Depends on: 588967

Updated

8 years ago
Assignee: pwalton → mihai.sucan
Blocks: 594225
(Assignee)

Comment 2

8 years ago
Created attachment 474526 [details] [diff] [review]
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.
Attachment #474526 - Flags: feedback?(pwalton)
(Reporter)

Comment 3

8 years ago
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
Attachment #474526 - Flags: feedback?(pwalton) → feedback+
(Assignee)

Comment 4

8 years ago
(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.
(Assignee)

Comment 5

8 years ago
Created attachment 475050 [details] [diff] [review]
updated patch

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

Asking for review from Shawn now.
Attachment #474526 - Attachment is obsolete: true
Attachment #475050 - Flags: review?(sdwilsh)
(Assignee)

Updated

8 years ago
Whiteboard: [patchclean:0914]
(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 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.
Attachment #475050 - Flags: review?(sdwilsh) → review+
(Assignee)

Comment 8

8 years ago
Created attachment 479026 [details] [diff] [review]
updated patch

Thanks Shawn for your review+!

This is the updated patch with the requested changes. Asking again for feedback from Patrick.
Attachment #475050 - Attachment is obsolete: true
Attachment #479026 - Flags: feedback?(pwalton)
(Assignee)

Updated

8 years ago
Whiteboard: [patchclean:0914] → [patchclean:0928]
(Reporter)

Comment 9

8 years ago
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.
Attachment #479026 - Flags: feedback?(pwalton) → feedback+
(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 :)
(Assignee)

Comment 11

8 years ago
(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.
(Assignee)

Comment 12

8 years ago
Created attachment 479523 [details] [diff] [review]
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!
Attachment #479026 - Attachment is obsolete: true
Attachment #479523 - Flags: approval2.0?
(Assignee)

Updated

8 years ago
Whiteboard: [patchclean:0928] → [patchclean:0929]
(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|
Whiteboard: [patchclean:0929] → [patchclean:0929][needs-approval]
Version: unspecified → Trunk
blocking2.0: --- → beta8+
Whiteboard: [patchclean:0929][needs-approval] → [patchclean:0929]
(Assignee)

Comment 14

8 years ago
Created attachment 482949 [details] [diff] [review]
[checked-in] rebased patch

Rebased patch on top of the latest mozilla-central.
Attachment #479523 - Attachment is obsolete: true
Attachment #479523 - Flags: approval2.0?
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
Whiteboard: [patchclean:0929] → [patchclean:1013]
Comment on attachment 482949 [details] [diff] [review]
[checked-in] rebased patch

http://hg.mozilla.org/mozilla-central/rev/5358162f8111
Attachment #482949 - Attachment description: rebased patch → [checked-in] rebased patch
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.