Closed Bug 1179318 Opened 5 years ago Closed 5 years ago
[Ruleview] names and values are not being autocompleted correctly if you click a suggestion in popup (in some cases)
If currently typed name or value is valid and is a substring of another name/value which you choose by click, then autocomplete doesn't work correctly (watch video) Steps To Reproduce: 1. Open devtools -> Inspector -> Rules 2. Create new rule: type "background" and see the autocomplete popup with suggestions 3. Click the suggestion "background-color" Result: The autocompleted rule is "back-color" or sometimes "backgroun-color" or something like that. The same applies to values like "inline", "inline-block", "inline-flex".
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [polish-backlog] → [polish-backlog][difficulty=medium]
Looks like a little buglet in inplace-editor's _onBlur.
Assignee: nobody → ttromey
Status: NEW → ASSIGNED
Comment on attachment 8678166 [details] [diff] [review] fix css property name completion The bug is that _onBlur incorrect computes "pre", leading to the wrong text being inserted. Here I think selectionStart cannot be greater than selectionEnd. So the only remaining case is selectionStart === selectionEnd. But in that case, there is no selection, and I think it still makes sense to truncate at the cursor position. So, I just removed the alternate case here.
Attachment #8678166 - Flags: review?(pbrosset)
Comment on attachment 8678166 [details] [diff] [review] fix css property name completion Review of attachment 8678166 [details] [diff] [review]: ----------------------------------------------------------------- Tested locally, worked fine. Thanks for the new test too, it looks really good. Not sure why you decided to make it reload the page at the end though? Is this catching a potential bug? ::: devtools/client/styleinspector/test/browser_ruleview_completion-new-property_03.js @@ +2,5 @@ > +/* Any copyright is dedicated to the Public Domain. > + http://creativecommons.org/publicdomain/zero/1.0/ */ > + > +"use strict"; > + Please add a comment here that explains to readers what this test intends to test. This helps people understand quickly what a test does, and I also find that it helps people know where to add more test cases when adding new features to the code.
Attachment #8678166 - Flags: review?(pbrosset) → review+
(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #4) > Tested locally, worked fine. Thanks for the new test too, it looks really > good. Not sure why you decided to make it reload the page at the end though? > Is this catching a potential bug? I copied the skeleton from a different test and forgot to delete this. I've removed it now.
Fix for review; add r=pbrosset.
Attachment #8678166 - Attachment is obsolete: true
Well, it wasn't that simple :( This regresses a test when editing a style attribute; and looking at this shows that the three CSS completers (style attributes, rule view, and style editor) disagree on some behavior. E.g., if you type "di", the first completion is "direction". If you hit ":", the rule view inserts "direction" and starts editing the value. However, the style attribute case and the style editor just insert a ":". If you hit tab, all three insert "direction". The rule view jumps to the value. The style editor actually inserts "direction: ", essentially moving to the value. The style attribute editor inserts "direction" -- but now if you hit tab again, it will replace "direction" with "display" (the second completion). A further point here is that after tab completion, nothing is selected; so inserting a ":" will not overwrite any of the completed text. One idea would be to drop this "tab cycling" behavior, since it's not consistent with the other editors. A second idea would be to select the completed text, so that another tab would replace it. However, this would mean that "di" tab ":" would overwrite the completion. A further special case could be added to salvage this idea: give ":" a special meaning just after completion. A third idea would be to track whether the last action was a completion, and fix up the completion text specially in this case. This is what I'm leaning toward.
Another option is to just make the change conditional on this.contentType. I may do this as it is simple and there are already plenty of conditions like this. Also I noticed that if the style attribute looks like style="", and you put the cursor between the quotes, then completion doesn't work. However, delete the second quote and it does.
Just check contentType.
Attachment #8678823 - Attachment is obsolete: true
Comment on attachment 8679514 [details] [diff] [review] fix css property name completion This implements the simplest idea to preserve the style attribute completion behavior, and adds a comment to explain the need for the odd condition.
Attachment #8679514 - Flags: review?(pbrosset)
Attachment #8679514 - Flags: review?(pbrosset) → review+
removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion
Component: Developer Tools: Inspector → Developer Tools: CSS Rules Inspector
You need to log in before you can comment on or make changes to this bug.