Show variable auto-completion in inspector

VERIFIED FIXED in Firefox 60

Status

enhancement
P3
normal
VERIFIED FIXED
2 years ago
Last year

People

(Reporter: rajdeep.nanua, Assigned: rajdeep.nanua)

Tracking

(Blocks 2 bugs, {dev-doc-complete})

unspecified
Firefox 60
Dependency tree / graph

Firefox Tracking Flags

(firefox60 verified)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Assignee

Description

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

Updated

2 years ago
Component: Untriaged → Developer Tools: Inspector
Assignee

Updated

2 years ago
Component: Developer Tools: Inspector → Developer Tools: CSS Rules Inspector
Assignee: nobody → rajdeep.nanua
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Priority: -- → P3
Comment hidden (mozreview-request)
Attachment #8934047 - Flags: review?(gl) → review?(pbrosset)
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.
Attachment #8934047 - Flags: review?(pbrosset) → review?(jdescottes)

Comment 3

2 years ago
mozreview-review
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()`)
Attachment #8934047 - Flags: review?(jdescottes)
Severity: normal → enhancement
Keywords: dev-doc-needed
Comment hidden (mozreview-request)
Assignee

Comment 5

2 years ago
mozreview-review-reply
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

2 years ago
mozreview-review
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!
Attachment #8934047 - Flags: review?(jdescottes) → review+
(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.
Flags: needinfo?(jdescottes)
Let's first see if Rajdeep can take care of this.
Flags: needinfo?(jdescottes) → needinfo?(rajdeep.nanua)
Duplicate of this bug: 1258494
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
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.
Flags: needinfo?(rajdeep.nanua)

Comment 14

Last year
mozreview-review
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.
Attachment #8934047 - Flags: review+
Attachment #8942629 - Attachment is obsolete: true
Attachment #8942629 - Flags: review?(jdescottes)
Attachment #8942630 - Attachment is obsolete: true
Attachment #8942630 - Flags: review?(gl)
Comment hidden (mozreview-request)
See Also: → 1432452

Comment 16

Last year
mozreview-review
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.
Attachment #8934047 - Flags: review?(jdescottes) → review+

Comment 17

Last year
mozreview-review-reply
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.
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.
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

Last year
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0569381b5e5f
Implement CSS variable autocompletion. r=jdescottes
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
Flags: needinfo?(rajdeep.nanua)

Comment 22

Last year
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/19b81340410f
Implement CSS variable autocompletion. r=jdescottes
Flags: needinfo?(rajdeep.nanua)

Comment 23

Last year
bugherder
https://hg.mozilla.org/mozilla-central/rev/19b81340410f
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
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
Status: RESOLVED → VERIFIED
(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
Blocks: 1223058
(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!
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!
Flags: needinfo?(rajdeep.nanua)
Assignee

Comment 28

Last year
(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
Flags: needinfo?(rajdeep.nanua)
Depends on: 1444772
Blocks: 1444772
No longer depends on: 1444772

Updated

Last year
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.