Autocomplete CSS properties and values in markup view

RESOLVED FIXED in Firefox 25

Status

()

Firefox
Developer Tools: Inspector
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: Optimizer, Assigned: Optimizer)

Tracking

unspecified
Firefox 25
x86_64
Windows 7
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
Markup View also uses inplace editor to edit html. Thus, it will not be a lot much work to port autocompletion there.

the CONTENT_TYPES.MIXED_CONTENT in inplace-editor is specifically for that.
(Assignee)

Comment 1

5 years ago
err. make that CONTENT_TYPES.CSS_MIXED
(Assignee)

Comment 2

5 years ago
Created attachment 780314 [details] [diff] [review]
patch v0.1

Adds a popup and suggestions to the markup panel when you are in a style = "" thing.
Assignee: nobody → scrapmachines
Status: NEW → ASSIGNED
Attachment #780314 - Flags: feedback?(mratcliffe)
Comment on attachment 780314 [details] [diff] [review]
patch v0.1

Review of attachment 780314 [details] [diff] [review]:
-----------------------------------------------------------------

This is well on it's way.

A few points and nits to address.

This patch also needs a test.

::: browser/devtools/markupview/markup-view.js
@@ +73,5 @@
>  
>    this._initPreview();
> +
> +  // Initialize static autocomplete popup
> +  getAutocompletePopup(this.doc.defaultView.parent.document);

getAutocompletePopup(this._frame.ownerDocument);

@@ +1130,5 @@
>      element: this.newAttr,
>      trigger: "dblclick",
>      stopOnReturn: true,
> +    contentType: InplaceEditor.CONTENT_TYPES.CSS_MIXED,
> +    popup: getAutocompletePopup(this.doc.defaultView.parent.document),

getAutocompletePopup(this._frame.ownerDocument);

@@ +1224,5 @@
>          trigger: "dblclick",
>          stopOnReturn: true,
>          selectAll: false,
> +        contentType: InplaceEditor.CONTENT_TYPES.CSS_MIXED,
> +        popup: getAutocompletePopup(this.doc.defaultView.parent.document),

getAutocompletePopup(this._frame.ownerDocument);

@@ +1492,5 @@
>    "chrome://browser/locale/devtools/inspector.properties"
>  ));
> +
> +let _autocompletePopup = null;
> +function getAutocompletePopup(doc) {

Please move this method inside the MarkupView prototype, make it a getter and change _autocompletePopup to this._autocompletePopup. You should then destroy it from the destroy method.

::: browser/devtools/shared/inplace-editor.js
@@ +752,5 @@
>        cycling = true;
>        prevent = true;
>        increment > 0 ? this.popup.selectPreviousItem()
>                      : this.popup.selectNextItem();
> +      let input = this.input, pre = input.value.slice(0, input.selectionStart),

Each let should be on it's own line.

@@ +900,5 @@
> +        list = list.concat(CSSPropertyList);
> +      } else if (this.contentType == CONTENT_TYPES.CSS_VALUE) {
> +        list = domUtils.getCSSValuesForProperty(this.property.name).sort();
> +      } else if (this.contentType == CONTENT_TYPES.CSS_MIXED &&
> +                 query.match(/^\s*style\s*=/)) {

test is more efficient than match and designed for this purpose, use:
/^\s*style\s*=/.test(query);

@@ +902,5 @@
> +        list = domUtils.getCSSValuesForProperty(this.property.name).sort();
> +      } else if (this.contentType == CONTENT_TYPES.CSS_MIXED &&
> +                 query.match(/^\s*style\s*=/)) {
> +        // Detecting if cursor is at property or value;
> +        let match = query.match(/([:;\"\'=]?)\s*([^\"\';:=\s]+)$/);

You don't need to escape quotes in bracket expressions. This can be:
let match = query.match(/([:;"'=]?)\s*([^"';:=\s]+)$/);

@@ +906,5 @@
> +        let match = query.match(/([:;\"\'=]?)\s*([^\"\';:=\s]+)$/);
> +        if (match && match.length == 3) {
> +          if (match[1] == ":") { // We are in CSS value completion
> +            let propertyName =
> +              query.match(/[;\"\'=]\s*([^:;\"\'=\s]+)\s*:\s*[^;:\s\"\']+$/)[1];

You don't need to escape quotes in bracket expressions.

@@ +934,5 @@
>        if (!this.popup) {
>          return;
>        }
>        let finalList = [], i = 0, count = 0, length = list.length;
>        for (; i < length && count < MAX_POPUP_ENTRIES; i++) {

Try to stick to one let variable per line (of course, multiple inside a for is fine as long as it doesn't get too crowded).

e.g.
let finalList = [];
let length = list.length;
for (let i = 0, count = 0; i < length && count < MAX_POPUP_ENTRIES; i++) {
Attachment #780314 - Flags: feedback?(mratcliffe)
(Assignee)

Comment 4

5 years ago
Created attachment 780600 [details] [diff] [review]
patch v0.2

Addressed review comments. Still needs tests. Will write some tomorrow.
Attachment #780314 - Attachment is obsolete: true
Attachment #780600 - Flags: review?(mratcliffe)
Comment on attachment 780600 [details] [diff] [review]
patch v0.2

Review of attachment 780600 [details] [diff] [review]:
-----------------------------------------------------------------

Lookin good.

Not r+ing until you have added the test.
Attachment #780600 - Flags: review?(mratcliffe) → feedback+
(Assignee)

Comment 6

5 years ago
Created attachment 783828 [details] [diff] [review]
Patch v3.0

Adds test. Please push it to try as I cannot atm
Attachment #780600 - Attachment is obsolete: true
Attachment #783828 - Flags: review?(mratcliffe)
Attachment #783828 - Flags: review?(mratcliffe) → review+
(Assignee)

Comment 9

5 years ago
landed in fx-team : https://hg.mozilla.org/integration/fx-team/rev/2a567b5e955d
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/2a567b5e955d
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 25

Updated

4 years ago
Blocks: 916862

Updated

4 years ago
Blocks: 916763

Updated

4 years ago
No longer blocks: 916763
Depends on: 916763
You need to log in before you can comment on or make changes to this bug.