Closed Bug 965158 Opened 6 years ago Closed 6 years ago

Up/down arrows should navigate through the autocompletion suggestions

Categories

(DevTools :: Source Editor, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 29

People

(Reporter: past, Assigned: Optimizer)

Details

Attachments

(1 file, 1 obsolete file)

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.
> popups (console, gcli, style editor)

Er, I meant style *inspector*.
(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.
(moving this to source editor component, as the feature lies there)
(adding Nick and Joe to the discussion)
Component: Developer Tools: Style Editor → Developer Tools: Source Editor
This is exactly how I would expect it to work. How else would you navigate the caret?
(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.
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.
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? :)
(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 :)
Attached patch UP-DOWN (obsolete) — Splinter Review
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
Assignee: nobody → scrapmachines
Status: NEW → ASSIGNED
Attachment #8368149 - Flags: review?(fayearthur)
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)
(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.
Attached patch UP-DOWN-2Splinter Review
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
Attachment #8368149 - Attachment is obsolete: true
Attachment #8368149 - Flags: review?(fayearthur)
Attachment #8368426 - Flags: review?(fayearthur)
Attachment #8368426 - Flags: review?(fayearthur) → review+
landed in fx-team - https://hg.mozilla.org/integration/fx-team/rev/70b2641cc32c
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/70b2641cc32c
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 29
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.