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)

Categories

(DevTools :: Inspector: Rules, defect)

defect
Not set

Tracking

(firefox45 fixed)

RESOLVED FIXED
Firefox 45
Tracking Status
firefox45 --- fixed

People

(Reporter: arni2033, Assigned: tromey)

References

Details

(Whiteboard: [polish-backlog][difficulty=medium])

Attachments

(2 files, 2 obsolete files)

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".
See Also: → 1192486
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [polish-backlog]
Whiteboard: [polish-backlog] → [polish-backlog][difficulty=medium]
Looks like a little buglet in inplace-editor's _onBlur.
Assignee: nobody → ttromey
Status: NEW → ASSIGNED
Attached patch fix css property name completion (obsolete) — Splinter Review
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.
Attached patch fix css property name completion (obsolete) — Splinter Review
Fix for review; add r=pbrosset.
Attachment #8678166 - Attachment is obsolete: true
Attachment #8678823 - Flags: review+
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+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4c8e3843cce7
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion
Has STR: --- → yes
Component: Developer Tools: Inspector → Developer Tools: CSS Rules Inspector
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.