Closed Bug 873250 Opened 6 years ago Closed 6 years ago

Having enter select autocomplete is annoying.

Categories

(DevTools :: Console, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 24

People

(Reporter: potch, Assigned: rcampbell)

Details

Attachments

(1 file, 1 obsolete file)

When I'm debugging things, I tend to have short variable names. I try to interact with these variables in the console, to the following effect:

> var a = complicatedLineOfJavaScript.getComplexValue();
> a[ddEventListener]
(presses enter to see value of 'a')
> addeventListener
function addEventListener() {
    [native code]
}

at this point I get sad.
taking this because we are all saddened by this particular usability nugget.
Assignee: nobody → rcampbell
Priority: -- → P2
Attached patch enter-key (obsolete) — Splinter Review
This does the most basic thing: Enter Key always Evaluates. Now to autocomplete, you have to press a tab key, even with the popup open.

I tried a simple variant where if a user presses up or down arrows, it autocompletes on enter, but it was adding more logic rather than taking away. I think simpler is better.

The one slightly annoying case with this is that you will now have to press three keys to select an autocompletion and evaluate it. So to inspect a variable in scope, you might have to select Up then Tab then Enter.
Attachment #756959 - Flags: review?(mihai.sucan)
Status: NEW → ASSIGNED
FWIW, Firebug solves this by letting Enter evaluate iff the selected completion is an exact match (http://code.google.com/p/fbug/issues/detail?id=4931). It has worked pretty well for us.
(In reply to Rob Campbell [:rc] (:robcee) from comment #3)
> Created attachment 756959 [details] [diff] [review]
> enter-key
> 
> This does the most basic thing: Enter Key always Evaluates. Now to
> autocomplete, you have to press a tab key, even with the popup open.
> 
> I tried a simple variant where if a user presses up or down arrows, it
> autocompletes on enter, but it was adding more logic rather than taking
> away. I think simpler is better.

Agreed. Simple is better here. We should never break user's expectation for what Enter does.


> The one slightly annoying case with this is that you will now have to press
> three keys to select an autocompletion and evaluate it. So to inspect a
> variable in scope, you might have to select Up then Tab then Enter.

I tested this case and it seems fine for me with the patch applied.
Comment on attachment 756959 [details] [diff] [review]
enter-key

Review of attachment 756959 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you for the patch! r+ with the comments below addressed.

::: browser/devtools/webconsole/test/browser_webconsole_bug_585991_autocomplete_keys.js
@@ +199,1 @@
>           "completion was successful after VK_RETURN");

Shouldn't this code also check that the string that was executed is not foobar.valueOf?  Please also check that the last history entry is the .value string.

This change alone makes the test confusing.

::: browser/devtools/webconsole/webconsole.js
@@ +3865,4 @@
>          }
>          break;
>  
> +      // Bug 873250 - always enter, ignore autocomplete unless inputUpdated

inputUpdated? This looks like a remnant of some older patch version.
Attachment #756959 - Flags: review?(mihai.sucan) → review+
(In reply to Mihai Sucan [:msucan] from comment #6)
> Comment on attachment 756959 [details] [diff] [review]
> enter-key
> 
> Review of attachment 756959 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thank you for the patch! r+ with the comments below addressed.
> 
> :::
> browser/devtools/webconsole/test/
> browser_webconsole_bug_585991_autocomplete_keys.js
> @@ +199,1 @@
> >           "completion was successful after VK_RETURN");
> 
> Shouldn't this code also check that the string that was executed is not
> foobar.valueOf?  Please also check that the last history entry is the .value
> string.

maybe. I was going for simple here. Will update.

> This change alone makes the test confusing.
> 
> ::: browser/devtools/webconsole/webconsole.js
> @@ +3865,4 @@
> >          }
> >          break;
> >  
> > +      // Bug 873250 - always enter, ignore autocomplete unless inputUpdated
> 
> inputUpdated? This looks like a remnant of some older patch version.

yes, forgot to fix that comment before I qrefed.

thanks!
Attached patch enter-key v2Splinter Review
updated the unittest.
Attachment #756959 - Attachment is obsolete: true
Whiteboard: [land-in-fx-team]
https://hg.mozilla.org/integration/fx-team/rev/1095f46b0220
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/1095f46b0220
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 24
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.