Closed Bug 913955 Opened 7 years ago Closed 6 years ago
Style Inspector CSS autocomplete breaks manual entry in inplace editor
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.
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
I was able to repro it randomly when the bug has been filed, but not now.
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.
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.
Work in progress. Still need to consider edge cases such as having a space after the input selection, eg) "color : | red", and test cases.
-Halts autosuggestion if there are any non-space character in front of the caret
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.
Attachment #830004 - Flags: review?(scrapmachines) → review-
Not actively working on this bug in case anyone wants to pick it up.
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.
Summary: Style Inspector CSS autocomplete breaks manual entry → Style Inspector CSS autocomplete breaks manual entry in inplace editor
patches welcome :)
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
Comment on attachment 8391976 [details] [diff] [review] patch Review of attachment 8391976 [details] [diff] [review]: ----------------------------------------------------------------- Very simple fix for an annoying problem, r+
Attachment #8391976 - Flags: review?(mratcliffe) → review+
landed in fx-team : https://hg.mozilla.org/integration/fx-team/rev/c354ec771e52
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.
(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.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
Would this be safe to uplift to Aurora30?
its okay, as long as bug 987870 is also uplifted
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
Attachment #8391976 - Flags: approval-mozilla-aurora?
Attachment #8391976 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Looks like this landed with tests. Please correct if I am mistaken.
You need to log in before you can comment on or make changes to this bug.