[Ruleview] names and values are not being autocompleted correctly if you click a suggestion in popup (in some cases)

RESOLVED FIXED in Firefox 45

Status

()

Firefox
Developer Tools: CSS Rules Inspector
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: arni2033, Assigned: tromey)

Tracking

unspecified
Firefox 45
Points:
---

Firefox Tracking Flags

(firefox45 fixed)

Details

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

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

3 years ago
Created attachment 8628334 [details]
[Ruleview] names and values are not being autocompleted correctly if you click a suggestion in popup (in some cases).webm

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".
(Reporter)

Updated

3 years ago
See Also: → bug 1192486
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [polish-backlog]
Whiteboard: [polish-backlog] → [polish-backlog][difficulty=medium]
(Assignee)

Comment 1

2 years ago
Looks like a little buglet in inplace-editor's _onBlur.
Assignee: nobody → ttromey
Status: NEW → ASSIGNED
(Assignee)

Comment 2

2 years ago
Created attachment 8678166 [details] [diff] [review]
fix css property name completion
(Assignee)

Comment 3

2 years ago
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)
Blocks: 1210159
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+
(Assignee)

Comment 5

2 years ago
(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.
(Assignee)

Comment 6

2 years ago
Created attachment 8678823 [details] [diff] [review]
fix css property name completion

Fix for review; add r=pbrosset.
Attachment #8678166 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8678823 - Flags: review+
(Assignee)

Comment 8

2 years ago
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.
(Assignee)

Comment 9

2 years ago
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.
(Assignee)

Comment 10

2 years ago
Created attachment 8679514 [details] [diff] [review]
fix css property name completion

Just check contentType.
Attachment #8678823 - Attachment is obsolete: true
(Assignee)

Comment 11

2 years ago
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+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 14

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4c8e3843cce7
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox45: --- → fixed
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
status-b2g-v2.5: --- → ---
(Reporter)

Updated

2 years ago
Has STR: --- → yes
(Reporter)

Updated

2 years ago
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.