1422635 - Show variable auto-completion in inspector

Status: VERIFIED FIXED

(DevTools :: Inspector: Rules, enhancement, P3)

Description

[Rajdeep Nanua]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/62.0.3202.89 Safari/537.36

Comment 1

[Rajdeep Nanua]

Attachment: Bug 1422635: Implement CSS variable autocompletion.

Initial support for CSS variable autocompletion in ruleview.

Review commit: https://reviewboard.mozilla.org/r/204984/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/204984/

Comment 2

[Patrick Brosset <:pbro>]

Comment on attachment 8934047 [details]
Bug 1422635: Implement CSS variable autocompletion.

Sorry about forwarding this yet again but I'd feel more comfortable if Julian reviewed this. He's done more work on this code than I have.

Comment 3

[Julian Descottes [:jdescottes]]

Comment on attachment 8934047 [details]
Bug 1422635: Implement CSS variable autocompletion.

https://reviewboard.mozilla.org/r/204984/#review212118

Sorry about the delay for the review! This looks like a good start and works reasonably well.

One overall concern with this feature. If the user types : "var(--", sees the autocompletion and selects a value with TAB or ENTER, it will leave the field in a half broken state: "var\(--my-var".
It's not exactly a regression, it's just that this kind of scenario is much more likely to happen with your patch. It would be nice to either:
- add the closing parenthesis when needed
- remain in the field on TAB/ENTER if we are doing variable autocompletion
I think this should be handled as a follow up though.

Have a look at my comments for some improvements about the code.

Finally I would like to have a test to accompany this. Do you have your environment setup to run devtools tests?
If you do, can you look at browser_inplace-editor_autocomplete_01.js and try to clone it to test your new API?

::: devtools/client/inspector/rules/views/rule-editor.js:549
(Diff revision 1)
>        destroy: this._newPropertyDestroy,
>        advanceChars: ":",
>        contentType: InplaceEditor.CONTENT_TYPES.CSS_PROPERTY,
>        popup: this.ruleView.popup,
>        cssProperties: this.rule.cssProperties,
> +      variables: this.rule.elementStyle.variables,

this inplace editor has contentType CSS_PROPERTY, so it doesn't use the CSS variable autocompletion. No need to define it here.

::: devtools/client/inspector/rules/views/text-property-editor.js:231
(Diff revision 1)
>          destroy: this.updatePropertyState,
>          advanceChars: ":",
>          contentType: InplaceEditor.CONTENT_TYPES.CSS_PROPERTY,
>          popup: this.popup,
>          cssProperties: this.cssProperties,
> +        variables: this.rule.elementStyle.variables,

Same comment.

::: devtools/client/shared/inplace-editor.js:227
(Diff revision 1)
>    this.elt = options.element;
>    let doc = this.elt.ownerDocument;
>    this.doc = doc;
>    this.elt.inplaceEditor = this;
>    this.cssProperties = options.cssProperties;
> +  this.variables = options.variables;

Two things:
1- all options are documented in the JSDoc of function editableField(). Can you add an entry for this new one?

2- the inplace editor is not used only for CSS autocompletion, so the term "variables" is too vague here. Let's use cssVariables (both for the option and for the instance member)

::: devtools/client/shared/inplace-editor.js:1337
(Diff revision 1)
>            startCheckQuery = match[0];
>          } else {
>            startCheckQuery = "";
>          }
>  
> +        let varMatch = /^var\(([^\s]+$)/.exec(startCheckQuery);

Not a big deal, but if there are just spaces we could still autocomplete (e.g. "var( --" ). See what is done just above if match is falsy.

::: devtools/client/shared/inplace-editor.js:1337
(Diff revision 1)
>            startCheckQuery = match[0];
>          } else {
>            startCheckQuery = "";
>          }
>  
> +        let varMatch = /^var\(([^\s]+$)/.exec(startCheckQuery);

Add a comment here to say we are trying to match CSS variables.

::: devtools/client/shared/inplace-editor.js:1351
(Diff revision 1)
>  
>          if (query == "") {
>            // Do not suggest '!important' without any manually typed character.
>            list.splice(0, 1);
>          }
>        } else if (this.contentType == CONTENT_TYPES.CSS_MIXED &&

A good follow up (as a separate bug) would be to support CSS variable autocompletion for CSS_MIXED. CSS_MIXED is typically used when editing the markup view and typing a style attribute:
 
  `style="color: var(--some-var);"`

::: devtools/client/shared/inplace-editor.js:1507
(Diff revision 1)
> +  /**
> +   * Returns the list of all CSS variables to use for the autocompletion.
> +   *
> +   * @return {Array} array of CSS variable names (Strings)
> +   */
> +  _getVariableNames: function () {

Rename to _getCSSVariableNames

::: devtools/client/shared/inplace-editor.js:1508
(Diff revision 1)
> +   * Returns the list of all CSS variables to use for the autocompletion.
> +   *
> +   * @return {Array} array of CSS variable names (Strings)
> +   */
> +  _getVariableNames: function () {
> +    return Array.from(this.variables.keys()).sort();

Right now there are some inplace editors with the content-type CSS_VALUE that don't define the new variables option. 

For instance there is one in devtools/client/inspector/boxmodel/box-model.js

You should either:
- define variables for all inplace-editors of type CSS_VALUE
- set a default value in the constructor (for instance `this.variables = variables || new Map()`)

Comment 4

[Rajdeep Nanua]

Comment on attachment 8934047 [details]
Bug 1422635: Implement CSS variable autocompletion.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/204984/diff/1-2/

Comment 5

[Rajdeep Nanua]

Comment on attachment 8934047 [details]
Bug 1422635: Implement CSS variable autocompletion.

https://reviewboard.mozilla.org/r/204984/#review212118

Hi, I've made the requested fixes in code, but there may be a delay to get to unit tests because it is currently exam period for me.

Comment 6

[Julian Descottes [:jdescottes]]

Comment on attachment 8934047 [details]
Bug 1422635: Implement CSS variable autocompletion.

https://reviewboard.mozilla.org/r/204984/#review215080

Looks good, thanks for addressing the comments! Let me know if you think you'll have time to add a test, otherwise I can take care of this part.

::: devtools/client/shared/inplace-editor.js:1339
(Diff revision 2)
>          } else {
>            startCheckQuery = "";
>          }
>  
> +        // Check if the query to be completed is a CSS variable.
> +        let varMatch = /^var\(([^\s]+$)|(^--[^\s]*$)/.exec(startCheckQuery);

I can see that you added the second group to address my comment about autocompleting when we have "var({some space}--". I think I overlooked the impact and I think for now we should just revert to your previous implementation. Sorry about that!

Comment 7

[Gabriel Luong [:gl] (ΦωΦ)]

(In reply to Julian Descottes [:jdescottes][:julian] from comment #6)
> Comment on attachment 8934047 [details]
> Bug 1422635: Implement CSS variable autocompletion.
> 
> https://reviewboard.mozilla.org/r/204984/#review215080
> 
> Looks good, thanks for addressing the comments! Let me know if you think
> you'll have time to add a test, otherwise I can take care of this part.
> 
> ::: devtools/client/shared/inplace-editor.js:1339
> (Diff revision 2)
> >          } else {
> >            startCheckQuery = "";
> >          }
> >  
> > +        // Check if the query to be completed is a CSS variable.
> > +        let varMatch = /^var\(([^\s]+$)|(^--[^\s]*$)/.exec(startCheckQuery);
> 
> I can see that you added the second group to address my comment about
> autocompleting when we have "var({some space}--". I think I overlooked the
> impact and I think for now we should just revert to your previous
> implementation. Sorry about that!

Hey Julian,

Can you go ahead with doing what is necessary to land this.

Comment 8

[Julian Descottes [:jdescottes]]

Let's first see if Rajdeep can take care of this.

Comment 10

[Rajdeep Nanua]

Comment on attachment 8934047 [details]
Bug 1422635: Implement CSS variable autocompletion.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/204984/diff/2-3/

Comment 11

[Julian Descottes [:jdescottes]]

Attachment: Bug 1422635: Implement CSS variable autocompletion.

Initial support for CSS variable autocompletion in ruleview.

Review commit: https://reviewboard.mozilla.org/r/212902/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/212902/

Comment 12

[Julian Descottes [:jdescottes]]

Attachment: Bug 1422635 - add test for CSS variable autocomplete;

Review commit: https://reviewboard.mozilla.org/r/212904/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/212904/

Comment 13

[Julian Descottes [:jdescottes]]

Sorry Rajdeep! I totally missed your new attachment. I'll review this shortly. It looks similar to what I implemented when it comes to the test, so we should be good.

Comment 14

[Julian Descottes [:jdescottes]]

Comment on attachment 8934047 [details]
Bug 1422635: Implement CSS variable autocompletion.

https://reviewboard.mozilla.org/r/204984/#review218642

Test works well, some comments remaining before we can land. Clearing the review flag to make sure I don't miss your new version, but this looks good otherwise.

::: devtools/client/shared/inplace-editor.js:1339
(Diff revision 3)
> +        let varMatch = /^var\(([^\s]+$)|(^--[^\s]*$)/.exec(startCheckQuery);
> +
> +        if (varMatch && varMatch.length == 3) {
> +          startCheckQuery = varMatch[1] ? varMatch[1] : varMatch[2];
> +          list = this._getCSSVariableNames();
> +        } else {

As mentioned in the last review, can we revert this to your initial implementation?

  // Check if the query to be completed is a CSS variable.
  let varMatch = /^var\(([^\s]+$)/.exec(startCheckQuery);

  if (varMatch && varMatch.length == 2) {
    startCheckQuery = varMatch[1];
    list = this._getCSSVariableNames();
  } else {
    list = ["!important",
            ...this._getCSSValuesForPropertyName(this.property.name)];
  }

::: devtools/client/shared/test/browser.ini:151
(Diff revision 3)
>  [browser_inplace-editor-01.js]
>  [browser_inplace-editor-02.js]
>  [browser_inplace-editor_autocomplete_01.js]
>  [browser_inplace-editor_autocomplete_02.js]
>  [browser_inplace-editor_autocomplete_offset.js]
> +[browser_inplace-editor_autocomplete_variable.js]

please rename this to 

browser_inplace-editor_autocomplete_css_variable.js

::: devtools/client/shared/test/browser_inplace-editor_autocomplete_variable.js:39
(Diff revision 3)
> +  ["VK_DOWN", "var(--abc", 0, 2],
> +  ["VK_LEFT", "var(--abc", -1, 0],
> +];
> +
> +const mockGetCSSValuesForPropertyName = function (propertyName) {
> +  let values = {

this could just return an empty array, whatever the property name.

::: devtools/client/shared/test/browser_inplace-editor_autocomplete_variable.js:49
(Diff revision 3)
> +    ]
> +  };
> +  return values[propertyName] || [];
> +};
> +
> +const mockGetCSSVariableNames = function () {

In this case we don't need a mock, you can just create a map and use it in the constructor of the inplace editor.

Comment 15

[Rajdeep Nanua]

Comment on attachment 8934047 [details]
Bug 1422635: Implement CSS variable autocompletion.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/204984/diff/3-4/

Comment 16

[Julian Descottes [:jdescottes]]

Comment on attachment 8934047 [details]
Bug 1422635: Implement CSS variable autocompletion.

https://reviewboard.mozilla.org/r/204984/#review220632

Thanks for updated the patch again! Sorry it took so long to go through everything.
Some of my comments about the test were not addressed, but nothing blocking. Let's land this.

Comment 17

[Julian Descottes [:jdescottes]]

Comment on attachment 8934047 [details]
Bug 1422635: Implement CSS variable autocompletion.

https://reviewboard.mozilla.org/r/204984/#review218642

> In this case we don't need a mock, you can just create a map and use it in the constructor of the inplace editor.

Not fixed, can be done in a follow up.

Comment 18

[Julian Descottes [:jdescottes]]

I pushed your changeset to try, our continuous testing server: https://treeherder.mozilla.org/#/jobs?repo=try&revision=841979ad6bf38ec2b9051c2115297be31a04e214

If the test results are green after a few hours I will push this to our integration branch.

Comment 19

[Julian Descottes [:jdescottes]]

No failure detected on try, let's land! Thanks a lot for your contribution Rajdeep, your patch will be released in Firefox 60.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0569381b5e5f
Implement CSS variable autocompletion. r=jdescottes

Comment 21

[Andreea Pavel [:apavel]]

Backed out for ESlint failure at /builds/worker/checkouts/gecko/devtools/client/shared/test/browser_inplace-editor_autocomplete_css_variable.js

Push that caused the failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=0569381b5e5f0ccdfcb7083c59a92da686e14015

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=158064394&repo=autoland&lineNumber=224

Backout: https://hg.mozilla.org/integration/autoland/rev/67043dd4a707a782ef1d13febb5b0ece8be2ab24

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/19b81340410f
Implement CSS variable autocompletion. r=jdescottes

Comment 23

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/19b81340410f

Comment 24

[Sebastian Zartner [:sebo]]

Tested this in 60.0a1 (2018-01-28) and it works fine for me.

The only little issue I found is when you define a variable for a rule that doesn't apply to the element, you first have to unselect and then reselect it so that the variable gets listed. But that's bearable, I guess.

Thank you for filing this report and adding the feature, Rajdeep!

Sebastian

Comment 25

[Gabriel Luong [:gl] (ΦωΦ)]

(In reply to Sebastian Zartner [:sebo] from comment #24)
> Tested this in 60.0a1 (2018-01-28) and it works fine for me.
> 
> The only little issue I found is when you define a variable for a rule that
> doesn't apply to the element, you first have to unselect and then reselect
> it so that the variable gets listed. But that's bearable, I guess.
> 
> Thank you for filing this report and adding the feature, Rajdeep!
> 
> Sebastian

Comment 26

[Gabriel Luong [:gl] (ΦωΦ)]

(In reply to Sebastian Zartner [:sebo] from comment #24)
> Tested this in 60.0a1 (2018-01-28) and it works fine for me.
> 
> The only little issue I found is when you define a variable for a rule that
> doesn't apply to the element, you first have to unselect and then reselect
> it so that the variable gets listed. But that's bearable, I guess.
> 
> Thank you for filing this report and adding the feature, Rajdeep!
> 
> Sebastian

Filed Bug 1433939 for this. Thanks Sebo!

Comment 27

[Chris Mills [:cmills]]

I've documented this.

Note added to relevant guide article:
https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/How_to/Examine_and_edit_CSS#Edit_rules

Note added to Fx60 rel notes:
https://developer.mozilla.org/en-US/Firefox/Releases/60#Developer_tools

Let me know if this looks OK. Thanks!

Comment 28

[Rajdeep Nanua]

(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #27)
> I've documented this.
> 
> Note added to relevant guide article:
> https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/How_to/
> Examine_and_edit_CSS#Edit_rules
> 
> Note added to Fx60 rel notes:
> https://developer.mozilla.org/en-US/Firefox/Releases/60#Developer_tools
> 
> Let me know if this looks OK. Thanks!

Looks good to me