cheesypoof Reporter
	Description • 11 years ago

The style inspector's autocomplete can butcher one's attempt to manually enter a CSS value. Let's say I wish to add a 'background-clip' property to an element... I click on some of the whitespace in the style inspector panel, type 'background-clip', and hit enter. From this point if I type (quite rapidly) 'content', it produces 'content-boxt-boxent-box'. I have also seen 'content-boxnt-boxnt-box', 'content-box-box-box-box-box', and 'content-boxnt-box'. I can also produce similar behavior for 'display' when I type 'inline'. The common factor seems to be a '-' in the list of potential property values.

	Petruta Horea [:phorea], Desktop QA
	Comment 1 • 11 years ago

Couldn't reproduce on latest Aurora (20131007004003) on Ubuntu 13.04 and Win8 32-bit.

Can you reproduce this issue with a clean profile on latest Nightly? Thank you
http://support.mozilla.org/en-US/kb/profile-manager-create-and-remove-firefox-profiles

	Loic
	Comment 2 • 11 years ago

I was able to repro it randomly when the bug has been filed, but not now.

	cheesypoof Reporter
	Comment 3 • 11 years ago

I am still able to reproduce this issue, including with a new Nightly profile. Nevertheless, it does require me to type 'content' very quickly to see this behavior.

	Gabriel [:gl] (ΦωΦ)
	Comment 4 • 11 years ago

I was able to reproduce this in bug 934695:
If the css value is "red" and I move my input selection to "r|ed", and continue typing "e", then the autocomplete will fill in "reded".

I can work on this after 934695. Relevant section of the code is in inplace-editor.js#_maybeSuggestCompletion. For the query, we should check if there is any characters after the input selection, and if there is halt the autocompletion.

	Gabriel [:gl] (ΦωΦ)
	Comment 5 • 11 years ago

Work in progress. Still need to consider edge cases such as having a space after the input selection, eg) "color : | red", and test cases.

	Gabriel [:gl] (ΦωΦ)
	Comment 7 • 11 years ago

-Halts autosuggestion if there are any non-space character in front of the caret

	Girish Sharma [:Optimizer] Assignee
	Comment 8 • 11 years ago

Comment on attachment 830004 [details] [diff] [review]
913955.patch

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

I am afraid that this issue is a bit tricky. There are two things happening here :

 - User clicking in the middle of a word and starting to type, which leads to autocopmletion being triggered and thus things like "pad[ding]ing" happening (things in [] are selected)
 - User typing too fast that the autocompletion is not able to cope up and thus things like in the first comment's screenshot happen.

Here we cannot simply stop autocompleting as the completion here automatically types in the first entry as you type, so if you are typing "pad" , "ding" will automatically be added (like "pad[ding]" ) as you type. Thus, you will *always* have a character after the cursor without a space.

So we have to do two things here :
 - Do not autocomplete where there is non-selected text after the cursor. i.e. selectionStart == selectionEnd && no space after selectionStart
 - Somehow sync up suggesting autocopmletion and the user typing rapidly so that no race conditions like in #1's screenshot happen.

	Gabriel [:gl] (ΦωΦ)
	Comment 9 • 11 years ago

Not actively working on this bug in case anyone wants to pick it up.

	Brian Grinstead [:bgrins]
	Comment 10 • 11 years ago

I have the same issue as in Comment 1 somewhat often, and it is a bad experience.

> - Do not autocomplete where there is non-selected text after the cursor. i.e. selectionStart == selectionEnd && no space after selectionStart

Even if we just caught this particular type of error case and closed the popup it would be better than having stuff inserted into the middle of your word.

I also wonder if handling this would also fix the race condition problem, if it is just sporadically causing this same issue when typing too fast.

I think a good first fix would be the same as Attachment 830004 [details] [diff] but with the added check that the selection is collapsed, which would at least fix the case in Comment 4.

	Girish Sharma [:Optimizer] Assignee
	Comment 11 • 11 years ago

patches welcome :)

	Girish Sharma [:Optimizer] Assignee
	Comment 12 • 11 years ago

This fixes both the issues happening on slow machines:

1) Race condition when the timeout for input "r" is called when the input is already "re", thus creating situations like "reded"

2) When the user is typing something in between already entered text like "re|d" where | is the cursor.

Added a test to check for the second str

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

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]
	Comment 13 • 11 years ago

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

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

Very simple fix for an annoying problem, r+

	Girish Sharma [:Optimizer] Assignee
	Comment 14 • 11 years ago

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

	Brian Grinstead [:bgrins]
	Comment 15 • 11 years ago

Weird issue with this (or maybe Bug 912189):

* When I press cmd+a it inserts -moz-animation into the front of the editor regardless of where the cursor is or what is already in the box.  So if I press three times I see -moz-animation-moz-animation-moz-animation.  Same thing happens with ctrl+a (on mac), but it only inserts it once.

	Girish Sharma [:Optimizer] Assignee
	Comment 16 • 11 years ago

(In reply to Brian Grinstead [:bgrins] from comment #15)
> Weird issue with this (or maybe Bug 912189):
> 
> * When I press cmd+a it inserts -moz-animation into the front of the editor
> regardless of where the cursor is or what is already in the box.  So if I
> press three times I see -moz-animation-moz-animation-moz-animation.  Same
> thing happens with ctrl+a (on mac), but it only inserts it once.

ugh. This was an already existing issue/defect, which got exposed by Bug 912189 now as empty editors could also suggest now.

Path in bug 987870 fixes this.

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 17 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/c354ec771e52

	Ryan VanderMeulen [:RyanVM]
	Comment 18 • 11 years ago

Would this be safe to uplift to Aurora30?

	Girish Sharma [:Optimizer] Assignee
	Comment 19 • 11 years ago

its okay, as long as bug 987870 is also uplifted

	Girish Sharma [:Optimizer] Assignee
	Comment 20 • 11 years ago

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

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 912189
User impact if declined: This bug fixes an issue introduced by 912189. Also, this patch is needed as a part of a bigger queue so as to uplift the chunks-by-dir feature of tests to aurora. see https://tbpl.mozilla.org/?tree=Try&rev=bfee331a418c
Testing completed (on m-c, etc.): mc
Risk to taking this patch (and alternatives if risky): none
String or IDL/UUID changes made by this patch: none

	Ryan VanderMeulen [:RyanVM]
	Comment 21 • 11 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/a89216cc58e0

	u279076
	Comment 22 • 10 years ago

Looks like this landed with tests. Please correct if I am mistaken.