Closed
Bug 1422635
Opened 7 years ago
Closed 7 years ago
Show variable auto-completion in inspector
Categories
(DevTools :: Inspector: Rules, enhancement, P3)
DevTools
Inspector: Rules
Tracking
(firefox60 verified)
VERIFIED
FIXED
Firefox 60
Tracking | Status | |
---|---|---|
firefox60 | --- | verified |
People
(Reporter: rajdeep.nanua, Assigned: rajdeep.nanua)
References
(Blocks 2 open bugs)
Details
(Keywords: dev-doc-complete)
Attachments
(1 file, 2 obsolete files)
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•7 years ago
|
Component: Untriaged → Developer Tools: Inspector
Assignee | ||
Updated•7 years ago
|
Component: Developer Tools: Inspector → Developer Tools: CSS Rules Inspector
Updated•7 years ago
|
Assignee: nobody → rajdeep.nanua
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Priority: -- → P3
Comment hidden (mozreview-request) |
Updated•7 years ago
|
Attachment #8934047 -
Flags: review?(gl) → review?(pbrosset)
Comment 2•7 years ago
|
||
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•7 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)
Updated•7 years ago
|
Severity: normal → enhancement
Keywords: dev-doc-needed
Comment hidden (mozreview-request) |
Assignee | ||
Comment 5•7 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•7 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+
Comment 7•7 years ago
|
||
(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)
Comment 8•7 years ago
|
||
Let's first see if Rajdeep can take care of this.
Flags: needinfo?(jdescottes) → needinfo?(rajdeep.nanua)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 13•7 years ago
|
||
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•7 years ago
|
||
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+
Updated•7 years ago
|
Attachment #8942629 -
Attachment is obsolete: true
Attachment #8942629 -
Flags: review?(jdescottes)
Updated•7 years ago
|
Attachment #8942630 -
Attachment is obsolete: true
Attachment #8942630 -
Flags: review?(gl)
Comment hidden (mozreview-request) |
Comment 16•7 years ago
|
||
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•7 years ago
|
||
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.
Comment 18•7 years ago
|
||
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•7 years ago
|
||
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•7 years ago
|
||
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0569381b5e5f
Implement CSS variable autocompletion. r=jdescottes
Comment 21•7 years ago
|
||
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•7 years ago
|
||
Pushed by gabriel.luong@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/19b81340410f
Implement CSS variable autocompletion. r=jdescottes
Updated•7 years ago
|
Flags: needinfo?(rajdeep.nanua)
Comment 23•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Comment 24•7 years ago
|
||
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
Comment 25•7 years ago
|
||
(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
Comment 26•7 years ago
|
||
(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•7 years ago
|
||
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)
Keywords: dev-doc-needed → dev-doc-complete
Assignee | ||
Comment 28•7 years ago
|
||
(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)
Updated•7 years ago
|
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•