896181 - Autocomplete CSS properties and values in markup view

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Girish Sharma [:Optimizer]]

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.

Comment 1

[Girish Sharma [:Optimizer]]

err. make that CONTENT_TYPES.CSS_MIXED

Comment 2

[Girish Sharma [:Optimizer]]

Attachment: patch v0.1

Adds a popup and suggestions to the markup panel when you are in a style = "" thing.

Comment 3

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

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++) {

Comment 4

[Girish Sharma [:Optimizer]]

Attachment: patch v0.2

Addressed review comments. Still needs tests. Will write some tomorrow.

Comment 5

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

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.

Comment 6

[Girish Sharma [:Optimizer]]

Attachment: Patch v3.0

Adds test. Please push it to try as I cannot atm

Comment 7

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

I will review this on a green try:
https://tbpl.mozilla.org/?tree=Try&rev=c39bf5c9e8ca

Comment 8

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

https://tbpl.mozilla.org/?tree=Try&rev=e369cb25511a

Comment 9

[Girish Sharma [:Optimizer]]

landed in fx-team : https://hg.mozilla.org/integration/fx-team/rev/2a567b5e955d

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2a567b5e955d