Closed Bug 916582 Opened 6 years ago Closed 4 years ago

Should not display autocompletion suggestions in a style attribute in markup view after the closing quotes

Categories

(DevTools :: Inspector, defect, P2)

defect

Tracking

(firefox48 fixed)

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- fixed

People

(Reporter: Optimizer, Assigned: jdescottes)

References

Details

Attachments

(1 file)

No description provided.
Assignee: nobody → scrapmachines
Status: NEW → ASSIGNED
OS: Windows 7 → All
Hardware: x86_64 → All
No plans to work on this for now.
Assignee: scrapmachines → nobody
Status: ASSIGNED → NEW
Priority: -- → P2
Style attribute autocompletion no longer works, see Bug 1254070.
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
Depends on: 1254070
No longer depends on: 1254070
Ignore last comment, autocompletion actually works as long as you are at the end of the input.
When editing the style attribute, do not trigger autocomplete suggestions if the style attribute
was already closed by a set of matching double or single quotes.

Added a new test and moved common test functions to a dedicated helper.

Review commit: https://reviewboard.mozilla.org/r/38641/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38641/
Attachment #8727790 - Flags: review?(ttromey)
Attachment #8727790 - Flags: review?(ttromey) → review+
Comment on attachment 8727790 [details]
MozReview Request: Bug 916582 - skip autocomplete suggestions if style attribute already closed;r=tromey

https://reviewboard.mozilla.org/r/38641/#review35371

There were a couple of minor things I didn't understand about this patch.  Otherwise looks good.

::: devtools/client/inspector/markup/test/browser_markup_css_completion_style_attribute_02.js:101
(Diff revision 1)
> +  ["VK_RETURN", "style=\"background:url(\"1\"); color:beige\"", -1, -1, false]

I'm a little confused about this line, since I would it not to work due to double quotes being used both for the style="" and for url("").

::: devtools/client/shared/inplace-editor.js:1186
(Diff revision 1)
> +        if (/^("[^"]*"|'[^']*')./.test(styleValue)) {

I don't understand the reason for the trailing "." in the regexp here.  If it's intentional then I suggest adding the explanation to the comment.
https://reviewboard.mozilla.org/r/38641/#review35371

> I'm a little confused about this line, since I would it not to work due to double quotes being used both for the style="" and for url("").

It is a bit confusing. 
For all this test, the style attribute value has been wrapped in single quotes.
Here, we are at the last step, and therefore are checking the state of the style attribute after leaving the inplace editor and committing the value.

The double quotes surrounding the attribute's value are "hardcoded" regardless of what the html really looks like.

To make it less confusing I will test the other way around with single quotes inside a double quoted attribute.

> I don't understand the reason for the trailing "." in the regexp here.  If it's intentional then I suggest adding the explanation to the comment.

Totally unintentional, thanks for catching that!
Comment on attachment 8727790 [details]
MozReview Request: Bug 916582 - skip autocomplete suggestions if style attribute already closed;r=tromey

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38641/diff/1-2/
Comment on attachment 8727790 [details]
MozReview Request: Bug 916582 - skip autocomplete suggestions if style attribute already closed;r=tromey

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38641/diff/2-3/
Comment on attachment 8727790 [details]
MozReview Request: Bug 916582 - skip autocomplete suggestions if style attribute already closed;r=tromey

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/38641/diff/3-4/
https://hg.mozilla.org/mozilla-central/rev/3a140ffd7dd9
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.