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

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 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 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 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!

(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.

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

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 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 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 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.

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

Pushed by gabriel.luong@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/19b81340410f Implement CSS variable autocompletion. r=jdescottes

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

(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

(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!

(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