1431949 - Show variable values in the CSS variable autocomplete popup

Status: RESOLVED FIXED

(DevTools :: Inspector, enhancement, P3)

Comment 1

[Sarah]

Attachment: 1431949.patch

Comment 2

[Sarah]

Attachment: 1431949.patch

Comment 3

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

Comment on attachment 8944286 [details] [diff] [review]
1431949.patch

Review of attachment 8944286 [details] [diff] [review]:
-----------------------------------------------------------------

Gonna wait for a new patch.

Comment 4

[Sarah]

Attachment: 1431949.patch

Comment 5

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

Comment on attachment 8951490 [details] [diff] [review]
1431949.patch

Hey Julian, do you think you can mentor and review this? 

The point of this is to add the variable value in the autocomplete for CSS variables. The variable value in the autocomplete popup should be styled differently think tooltip/title. 

Imagine in the autocomplete popup, you see the following:

--blue-55     #0074e8
--grey-20     #ededf0
etc..

Comment 6

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

Comment on attachment 8951490 [details] [diff] [review]
1431949.patch

Review of attachment 8951490 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/shared/inplace-editor.js
@@ +1518,5 @@
>    },
>  
> +  _getCSSVariableValue : function(varName) {
> +    let tmp = this.cssVariables.get(varName);
> +    // Would return the real value of the variable in the case that a variable was assigned to

Let's not worry about getting the desired variable (eg, --red: var(--blue) and --blue: red where red is the desired value) and just return the first value (eg, var(--blue)) that the CSS variable points to. Let's remove the commented out bits.

Comment 7

[Sarah]

(In reply to Gabriel [:gl] (ΦωΦ) from comment #6)
> Comment on attachment 8951490 [details] [diff] [review]
> 1431949.patch
> 
> Review of attachment 8951490 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: devtools/client/shared/inplace-editor.js
> @@ +1518,5 @@
> >    },
> >  
> > +  _getCSSVariableValue : function(varName) {
> > +    let tmp = this.cssVariables.get(varName);
> > +    // Would return the real value of the variable in the case that a variable was assigned to
> 
> Let's not worry about getting the desired variable (eg, --red: var(--blue)
> and --blue: red where red is the desired value) and just return the first
> value (eg, var(--blue)) that the CSS variable points to. Let's remove the
> commented out bits.

Okay, is that the only part you had an issue with? I can fix that up and upload the new patch if that's the case

Comment 8

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

(In reply to Sarah from comment #7)
> (In reply to Gabriel [:gl] (ΦωΦ) from comment #6)
> > Comment on attachment 8951490 [details] [diff] [review]
> > 1431949.patch
> > 
> > Review of attachment 8951490 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: devtools/client/shared/inplace-editor.js
> > @@ +1518,5 @@
> > >    },
> > >  
> > > +  _getCSSVariableValue : function(varName) {
> > > +    let tmp = this.cssVariables.get(varName);
> > > +    // Would return the real value of the variable in the case that a variable was assigned to
> > 
> > Let's not worry about getting the desired variable (eg, --red: var(--blue)
> > and --blue: red where red is the desired value) and just return the first
> > value (eg, var(--blue)) that the CSS variable points to. Let's remove the
> > commented out bits.
> 
> Okay, is that the only part you had an issue with? I can fix that up and
> upload the new patch if that's the case

I am still waiting for feedback from jdescottes since he knows more about the inplace-editor and autocomplete popup than I do.

Comment 9

[Julian Descottes [:jdescottes]]

Comment on attachment 8951490 [details] [diff] [review]
1431949.patch

Review of attachment 8951490 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry about the delay here. Looks good and works pretty well. 
I have a few suggestions for the implementation. 

I also would like to see this tested at least by modifying browser_inplace-editor_autocomplete_css_variable.js.
We can modify the testCompletion function from helper_inplace_editor.js to also assert the text content of the selected popup item.
I don't know if you are familiar with running our tests, don't hesitate to ping us on slack/IRC if you need help.

::: devtools/client/shared/autocomplete-popup.js
@@ +86,5 @@
>  
>    this.selectedIndex = -1;
> +
> +  // Variable allowing for the extension of the label w/o altering item value
> +  this.extendLabelLength = 0;

We can get rid of this new property and instead modify the logic used to calculate "_maxLabelLength".
In `get _maxLabelLength()`, we can check if the item has a postLabel and in this case we can calculate the "max" post label length as we loop on the items.

@@ +448,4 @@
>        label.textContent = item.label.slice(item.preLabel.length);
>      }
>      listItem.appendChild(label);
> +    if(item.postLabel != "") {

Let's just check if item.postLabel is truthy here in case someone passes null/undefined `if(item.postLabel)`

@@ +450,5 @@
>      listItem.appendChild(label);
> +    if(item.postLabel != "") {
> +      let postDesc = this._document.createElementNS(HTML_NS, "span");
> +      postDesc.textContent = item.postLabel;
> +      postDesc.className = "cssvariable-value";

Maybe keep the classname generic? "autocomplete-post-label" to be consistent with the rest of the patch

::: devtools/client/shared/inplace-editor.js
@@ +1352,5 @@
>          if (varMatch && varMatch.length == 2) {
>            startCheckQuery = varMatch[1];
>            list = this._getCSSVariableNames();
> +          for(var i = 0; i < list.length; i++) {
> +            postLabelValues.push(this._getCSSVariableValue(list[i]));

Can we remove the for loop and use map here instead? 
`postLabelValues = list.map(variableName => this._getCSSVariableValue(variableName));`

@@ +1421,5 @@
>            count++;
>            finalList.push({
>              preLabel: startCheckQuery,
> +            label: list[i],
> +            postLabel : (isCSSVarList ? postLabelValues[i] : "")

Could we try to skip the isCSSVarList variable? Checking if postLabelValues[i] is truthy should be enough.
`postLabel : (postLabelValues[i] ? postLabelValues[i] : "")`

::: devtools/client/themes/common.css
@@ +110,5 @@
>  }
>  
>  .devtools-autocomplete-listbox .autocomplete-item > .autocomplete-count {
>    text-align: end;
> +  width:100%;

I don't think the count feature is used anymore, and will file a bug to investigate that. Was this change needed for any reason?

Comment 10

[Sarah]

Attachment: 1431949.patch

I have implemented the changes that Julian suggested. The "extendLabelLength" variable has also been removed and replaced by altering the maxLabelLength method. 

I wasn't able to add the testing yet as I'm still unfamiliar with it.

Comment 11

[Julian Descottes [:jdescottes]]

Comment on attachment 8957409 [details] [diff] [review]
1431949.patch

Review of attachment 8957409 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for updating the patch Sarah, that looks good to me and should be ready for review (let's wait for gl's feedback though) 
I think we can contribute the test here since this is your first patch :)

::: devtools/client/shared/autocomplete-popup.js
@@ +259,4 @@
>        if (count) {
>          label += count + "";
>        }
> +      if(postLabel) label += postLabel;

This should be written as 

  if (postLabel) {
    label += postLabel;
  }

We use eslint in order to enforce syntax rules. You can set it up by following the instructions at http://docs.firefox-dev.tools/contributing/eslint.html

(there are some other failures against the eslint rules in the patch, so let us know if you have trouble setting up eslint in order to detect them)

::: devtools/client/shared/inplace-editor.js
@@ +1515,5 @@
>  
>    /**
> +  * Returns the variable's value for the given CSS variable name.
> +  *
> +  * @return {String} variable Value

We should make this jsdoc consistent with rest of the file. For instance:

  * @return {String} varName
  *         The name of the variable

::: devtools/client/themes/common.css
@@ +105,5 @@
> +}
> +
> +.devtools-autocomplete-listbox .autocomplete-item > .autocomplete-postlabel {
> +  font-style : italic;
> +  float : right;

nit: For consistency with the rest of the file, could we remove the space between the property name and ":" for these two properties?

Comment 12

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

Comment on attachment 8957409 [details] [diff] [review]
1431949.patch

Same feedback. Mainly, looking for the unit tests and cleaning up the nits.

Comment 13

[Sarah]

Attachment: 1431949.patch

I've fixed the ESL issues that arose from my code, and I've added the testing cases. I'm not sure if they were done to standards so if possible please let me know how I can improve them. From what I've understood they didn't seem to encounter any errors during execution of testing.

Comment 14

[Julian Descottes [:jdescottes]]

Comment on attachment 8962116 [details] [diff] [review]
1431949.patch

Review of attachment 8962116 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for working on the test! 
Some comments to improve the current approach but we are not far from a patch that can land.

Feel free to send the next version for review (r?) rather than feedback.

Note that the patch is based on a slightly old changeset now and will need to be rebased before landing.
We had some pretty big changes in the DevTools codebase (eg about migrating from function* to async function)
so you will have some conflicts. We can also take care of the rebase if you want.

::: devtools/client/shared/test/browser_inplace-editor_autocomplete_css_variable.js
@@ +53,4 @@
>    ];
>  };
>  
> +const mockGetCSSVariableValues = function (index) {

This is optional, but we don't need that many mocks in this test. We could stop using both mockGetCSSVariableNames and mockGetCSSVariableValues if instead we create a map of variables as follows: 

  const CSS_VARIABLES = [
    ["--abc", "blue"],
    ["--def", "red"],
    // etc... 
  ];

And then pass it when creating the inplace editor.
  createInplaceEditorAndClick({
    start: runAutocompletionTest,
    contentType: InplaceEditor.CONTENT_TYPES.CSS_VALUE,
    property: {
      name: "color"
    },
    cssVariables: new Map(CSS_VARIABLES),
    done: resolve,
    popup: popup
  }, doc);

::: devtools/client/shared/test/helper_inplace_editor.js
@@ +70,4 @@
>   * @param {Array} testData
>   *        - {String} key, the key to send
>   *        - {String} completion, the expected value of the auto-completion
> + *        - {String} tooltip, the expected value of the tooltip

Could we call this postLabel, to be consistent with the rest of the implementation?
Maybe we should also mention that this should be the post-label for the selected item in the autocomplete popup.

Later we could refactor this to assert the post-labels for the whole popup on one test, rather than checking the selected item's post label.

@@ +75,5 @@
>   *        - {Number} total, the total number of suggestions in the popup
>   * @param {InplaceEditor} editor
>   *        The InplaceEditor instance being tested
>   */
> +function* testCompletion([key, completion, tooltip, index, total], editor) {

This helper method is used in other tests:
- devtools/client/shared/test/browser_inplace-editor_autocomplete_offset.js
- devtools/client/shared/test/browser_inplace-editor_autocomplete_01.js
- devtools/client/shared/test/browser_inplace-editor_autocomplete_02.js

Modifying the signature here by adding a new expected argument in the middle of the array means you would have to update all the other tests. The method probably needs refactor, but for the time being you can simply add your new argument as the last item of the array. That should be easier than modifying all existing tests.

@@ +105,4 @@
>    info("Checking the state");
>    if (completion !== null) {
>      is(editor.input.value, completion, "Correct value is autocompleted");
> +    if (tooltip !== null) {

let's move this outside of the `if (completion !== null)`

@@ +105,5 @@
>    info("Checking the state");
>    if (completion !== null) {
>      is(editor.input.value, completion, "Correct value is autocompleted");
> +    if (tooltip !== null) {
> +      is(editor._getCSSVariableValue(index), tooltip, "Correct tooltip is displayed");

The goal here is to check that the autocomplete popup rendered the expected post label for the currently selected item. So we need to assert the markup of the popup. For instance:

>  if (postLabel) {
>    let selectedItem = editor.popup.getItems()[index];
>    let selectedElement = editor.popup.elements.get(selectedItem);
>    ok(selectedElement.textContent.includes(postLabel),
>      "Selected popup element contains the expected post-label: " > postLabel);
>  }

Comment 15

[Sarah]

Attachment: 1431949.patch

I fixed up the tests to reflect your suggestions. I do encounter a warning after only the first "key" is entered.

[JavaScript Warning: "Key event not available on some keyboard layouts: key=“r” modifiers=“accel,alt” id=“key_toggleReaderMode”" {file: "chrome://mochikit/content/tests/SimpleTest/EventUtils.js" line: 946}] 

I didn't want to change anything if it were something being worked on at the moment, and it didn't change the results of the test.

Comment 16

[Julian Descottes [:jdescottes]]

Comment on attachment 8964090 [details] [diff] [review]
1431949.patch

Review of attachment 8964090 [details] [diff] [review]:
-----------------------------------------------------------------

We should update the commit message with r=jdescottes

Looks good to me, thanks for addressing all the comments Sarah! Just one nit to fix and this should be good to go.
I pushed your changeset to try, our continuous integration server. It will run all the devtools tests against your changeset.
The results will be posted at https://treeherder.mozilla.org/#/jobs?repo=try&revision=239b1880c54028c6d4d43f49521b209c76eeca39

If they don't highlight any unexpected failure, we will land your patch to integration branch and then it should ship with Firefox 61.

::: devtools/client/shared/test/helper_inplace_editor.js
@@ +72,4 @@
>   *        - {String} completion, the expected value of the auto-completion
>   *        - {Number} index, the index of the selected suggestion in the popup
>   *        - {Number} total, the total number of suggestions in the popup
> + *        - {String} postLabel, the expected value of the tooltip

nit: the expected value of the tooltip -> the expected post label for the selected suggestion

Comment 17

[Julian Descottes [:jdescottes]]

Comment on attachment 8964090 [details] [diff] [review]
1431949.patch

Review of attachment 8964090 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/shared/test/helper_inplace_editor.js
@@ +106,4 @@
>    if (completion !== null) {
>      is(editor.input.value, completion, "Correct value is autocompleted");
>    }
> +  if (postLabel !== null) {

The tests on try highlighted an issue. Since "postLabel" can be missing, it will just be undefined. 
Let's simply use `if (postLabel) { ... }` here.

Here is a new try push with the suggested fix: https://treeherder.mozilla.org/#/jobs?repo=try&revision=55160d10c27479dfa329f9c17358dfbb767ffd2d

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f6eb728f242
Show variable values in the CSS variable autocomplete popup. r=jdescottes

Comment 19

[Julian Descottes [:jdescottes]]

Looks like Gabriel amended the patch and addressed my comments. 

Thanks for working on this bug Sarah and for taking the time to add a test as well! Your patch will be released with Firefox 61. If you want to find other mentored bugs, have a look at http://bugs.firefox-dev.tools/?tool=all&mentored

Comment 20

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/3f6eb728f242

Comment 21

[Chris Mills [:cmills]]

Documented; see https://bugzilla.mozilla.org/show_bug.cgi?id=1451211#c14