965158 - Up/down arrows should navigate through the autocompletion suggestions

Status: RESOLVED FIXED

(DevTools :: Source Editor, defect)

Description

[Panos Astithas (he/him) [:past] (please ni?)]

Currently pressing the up or down arrows while an autocompletion popup is displayed closes the popup and moves the cursor to a different line. That is totally unexpected and contrary to the behavior of our other autocompletion popups (console, gcli, style editor) or editors like Sublime Text.

Comment 1

[Panos Astithas (he/him) [:past] (please ni?)]

> popups (console, gcli, style editor)

Er, I meant style *inspector*.

Comment 2

[Girish Sharma [:Optimizer]]

(In reply to Panos Astithas [:past] from comment #0)
> Currently pressing the up or down arrows while an autocompletion popup is
> displayed closes the popup and moves the cursor to a different line. That is
> totally unexpected and contrary to the behavior of our other autocompletion
> popups (console, gcli, style editor) or editors like Sublime Text.

The thing is that the UP/DOWN keys in all of these situations have no other meaning. They are free.

There is some discussion in Bug 879028 about this . I have commented there again to clarify the expected behavior once again.

Comment 3

[Girish Sharma [:Optimizer]]

(moving this to source editor component, as the feature lies there)
(adding Nick and Joe to the discussion)

Comment 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

This is exactly how I would expect it to work. How else would you navigate the caret?

Comment 5

[Joe Walker [:jwalker]]

(In reply to Nick Fitzgerald [:fitzgen] from comment #4)
> This is exactly how I would expect it to work. How else would you navigate
> the caret?

The common answer fmor other editors is "not when you're using completion". So I'm concerned that we're breaking user expectations here.

I think this should have input from Darrin. CCed.

Comment 6

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

I would also expect Up / Down to navigate a completion list while it is visible.  If I want to revert back to text navigation, I would to expect to have to first exit the completion list via Esc or something.

Comment 7

[Victor Porof [:vporof][:vp]]

How about just respecting what our other autocompletion lists do, at least. The one in the console (which I'd imagine is the most prominently used) has UP/DOWN navigation. QED? :)

Comment 8

[Darrin Henein [:darrin]]

(In reply to J. Ryan Stinnett [:jryans] from comment #6)
> I would also expect Up / Down to navigate a completion list while it is
> visible.  If I want to revert back to text navigation, I would to expect to
> have to first exit the completion list via Esc or something.

What he said. And others. I think this is a common pattern (navigate a list when shown, navigate lines otherwise). From my experience with xcode, sublime, some vim plugins, etc this is what I've come to expect, so there's my two cents :)

Comment 9

[Girish Sharma [:Optimizer]]

Attachment: UP-DOWN

Nick - Sorry bro. :P

Adds UP/DOWN navigation support. Added a couple of checks for the same.

Also fixed the issue when you close the popup using Escape key, and split console pops up too :| (it was annoying, I had to fix)

A very quick review, heather :)

try run : https://tbpl.mozilla.org/?tree=Try&rev=5de44c8ebba6

Comment 10

[Anton Kovalyov (:anton)]

Comment on attachment 8368149 [details] [diff] [review]
UP-DOWN

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

::: browser/devtools/sourceeditor/autocomplete.js
@@ +163,5 @@
>  function onEditorKeypress(ed, event) {
>    let private = privates.get(ed);
>    switch (event.keyCode) {
> +    case event.DOM_VK_ESCAPE:
> +      event.preventDefault();

Does this conflict with the VIM mode in any way? (I can't test it myself right now)

Comment 11

[Girish Sharma [:Optimizer]]

(In reply to Anton Kovalyov (:anton) from comment #10)
> Comment on attachment 8368149 [details] [diff] [review]
> UP-DOWN
> 
> Review of attachment 8368149 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/sourceeditor/autocomplete.js
> @@ +163,5 @@
> >  function onEditorKeypress(ed, event) {
> >    let private = privates.get(ed);
> >    switch (event.keyCode) {
> > +    case event.DOM_VK_ESCAPE:
> > +      event.preventDefault();
> 
> Does this conflict with the VIM mode in any way? (I can't test it myself
> right now)

If you don't want navigation when popup is open, I am assuming that you dont want any other action too.

Comment 12

[Girish Sharma [:Optimizer]]

Attachment: UP-DOWN-2

Added an additional check for the popup being open before preventDefaulting escape key.

Previous try was bad as some other prema orange got involved.

new try :  https://tbpl.mozilla.org/?tree=Try&rev=d1176e183f62

Comment 13

[Girish Sharma [:Optimizer]]

landed in fx-team - https://hg.mozilla.org/integration/fx-team/rev/70b2641cc32c

Comment 14

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

https://hg.mozilla.org/mozilla-central/rev/70b2641cc32c