971276 - Hitting enter should insert the currently selected item from an autocomplete list in the Style Editor

Status: VERIFIED FIXED

(DevTools :: Style Editor, defect, P1)

Description

[Jeff Griffiths (:canuckistani) (:⚡︎)]

This is a follow-up for the css autocomplete patch in 29, and I personally think a fix should be uplifted to 29.

In most tools that provide css autocomplete ( brackets, chrome, ST2 ) hitting enter while a completion list is showing results in the currently selected item being used. Currently we add a newline, which is exactly what most developers used to the behaviour of other tools don't expect.

What other tools actually do is this complete the term and then add ': ' to the end, placing the cursor after the space.

Comment 1

[Patrick Brosset <:pbro>]

Also, worth noting the wanted behavior is exactly the one we have in the inspector rule-view, so it would be consistent to change the style-editor autocomplete to this too.

Comment 2

[Girish Sharma [:Optimizer]]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #1)
> Also, worth noting the wanted behavior is exactly the one we have in the
> inspector rule-view, so it would be consistent to change the style-editor
> autocomplete to this too.

This might lead to the same issues that rule-view is facing. Basically fast typing + slow machine = race conditions.

Also, one thing to note here is that no other text editor inserts the suggestion automatically, at least what all I have used, (ST2, notepad++, Chrome devtools, Eclippse, IntelliJ etc.)

Comment 3

[Brian Grinstead [:bgrins]]

> Also, one thing to note here is that no other text editor inserts the
> suggestion automatically, at least what all I have used, (ST2, notepad++,
> Chrome devtools, Eclippse, IntelliJ etc.)

This is not meant to automatically insert a suggestion, it is to add it only the user presses enter.

Comment 4

[Girish Sharma [:Optimizer]]

Ah I see. Well, comment 1 suggested "exactly like rule-view" So I thought auto insert is also intended.

This is a very quick n easy one.
Please mind as I take it :)

Comment 5

[Brian Grinstead [:bgrins]]

(In reply to Girish Sharma [:Optimizer] from comment #4)
> Ah I see. Well, comment 1 suggested "exactly like rule-view" So I thought
> auto insert is also intended.
> 
> This is a very quick n easy one.
> Please mind as I take it :)

Thanks :).  Yeah, basically pressing Enter should do the same thing as pressing Tab when an autocomplete list is opened.

I also notice that the 'cycling' behavior currently in the Style Editor is a bit different than some other tools I've used.  As I up/down through the list the full suggestion gets entered in place.  What I'm used to is that no extra characters would be added into the editor until I press enter/tab.  Not as big of a deal though, and probably a separate discussion / bug.

Comment 6

[Girish Sharma [:Optimizer]]

Attachment: patch

Made so that enter will insert the first suggestion and close the popup. If the user has already inserted a suggestion once (like by pressing tab or up/down), enter will simply close the popup and not place a new line.

Comment 7

[Heather Arthur [:harth]]

Comment on attachment 8375681 [details] [diff] [review]
patch

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

Works well.

I'm with Brian that I'm not used to up/down pre-populating the editor.

::: browser/devtools/sourceeditor/autocomplete.js
@@ +28,5 @@
>      autoSelect: true
>    });
>  
> +  let cyclingMethod = (reverse) => {
> +    return function() {

This is a bit roundabout. What about just:

function cycle(reverse) {
  cycleSuggestions(ed, reverse);
}

keyMap = {
  "tab": cycle,
  "up": cycle.bind(this, true)
}

Comment 8

[Girish Sharma [:Optimizer]]

Attachment: comments addressed

(In reply to Heather Arthur [:harth] from comment #7)
> Comment on attachment 8375681 [details] [diff] [review]
> patch
> 
> Review of attachment 8375681 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Works well.
> 
> I'm with Brian that I'm not used to up/down pre-populating the editor.

Lets have a separate bug for that discussion :)

> ::: browser/devtools/sourceeditor/autocomplete.js
> @@ +28,5 @@
> >      autoSelect: true
> >    });
> >  
> > +  let cyclingMethod = (reverse) => {
> > +    return function() {
> 
> This is a bit roundabout. What about just:
> 
> function cycle(reverse) {
>   cycleSuggestions(ed, reverse);
> }
> 
> keyMap = {
>   "tab": cycle,
>   "up": cycle.bind(this, true)
> }

Neat. Done!.

try push : https://tbpl.mozilla.org/?tree=Try&rev=741091da8cf7

Comment 9

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to Girish Sharma [:Optimizer] from comment #8)
...
> try push : https://tbpl.mozilla.org/?tree=Try&rev=741091da8cf7

Works for me - thanks!

Comment 10

[Girish Sharma [:Optimizer]]

rebased and landed in fx-team : https://hg.mozilla.org/integration/fx-team/rev/1e176b92c297

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/1e176b92c297

Comment 12

[Girish Sharma [:Optimizer]]

Comment on attachment 8375714 [details] [diff] [review]
comments addressed

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 717369
User impact if declined: Muscle memory to enter select the suggestion will not work for most users
Testing completed (on m-c, etc.): m-c, fx-team
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none

One note that the patch that got landed has a dependency on Bug 969247 while the attached patch to this bug (I mean this patch) is independent of that bug, so exactly this patch should be uplifted.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2ac2e3a81f7b

Comment 14

[Girish Sharma [:Optimizer]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #13)
> https://hg.mozilla.org/releases/mozilla-aurora/rev/2ac2e3a81f7b

Hi Ryan, this is the wrong patch that landed . I requested in comment 12 to land the attached patch (attachment 8375714 [details] [diff] [review]) to aurora instead of the actual patch that landed in fx-team and them mc as the landed patch was rebased on top of Bug 969247.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1e578a03b491
https://hg.mozilla.org/releases/mozilla-aurora/rev/c6c359ce5f02

Comment 16

[Bogdan Maris, Desktop Test Engineering]

Reproduced the initial issue on older nightly build and verified that the issue is fixed using latest Aurora on Windows 7 64bit, Mac OS X 10.9.1 and Ubuntu 12.04 32bit.

Comment 17

[Paul Silaghi, QA [:pauly]]

Verified fixed 30.0a2 (2014-03-18), Win 7 x64.