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

VERIFIED FIXED in Firefox 29

Status

defect
P1
normal
VERIFIED FIXED
6 years ago
Last year

People

(Reporter: canuckistani, Assigned: Optimizer)

Tracking

Trunk
Firefox 30
Dependency tree / graph

Firefox Tracking Flags

(firefox29 verified, firefox30 verified)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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.
OS: Mac OS X → All
Hardware: x86 → All
Summary: hitting enter to select an item from an autocomplete list should not insert a newline. → Hitting enter should insert the currently selected item from an autocomplete list in the Style Editor
Version: 26 Branch → Trunk
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.
(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.)
> 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.
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 :)
Assignee: nobody → scrapmachines
Status: NEW → ASSIGNED
(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.
Posted patch patch (obsolete) — Splinter Review
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.
Attachment #8375681 - Flags: review?(fayearthur)
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)
}
Attachment #8375681 - Flags: review?(fayearthur) → review+
(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
Attachment #8375681 - Attachment is obsolete: true
Attachment #8375714 - Flags: review+
(In reply to Girish Sharma [:Optimizer] from comment #8)
...
> try push : https://tbpl.mozilla.org/?tree=Try&rev=741091da8cf7

Works for me - thanks!
Keywords: checkin-needed
rebased and landed in fx-team : https://hg.mozilla.org/integration/fx-team/rev/1e176b92c297
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/1e176b92c297
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 30
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.
Attachment #8375714 - Flags: approval-mozilla-aurora?
Attachment #8375714 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(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.
Keywords: verifyme
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.
Verified fixed 30.0a2 (2014-03-18), Win 7 x64.
Status: RESOLVED → VERIFIED
Keywords: verifyme
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.