I've added the colour swatch next to the css values in the menu, only if they are a colour. It currently validates the string using the color library, and from there it will create two spans within the autocomplete item to display the swatch and the label description. This was styled after the rulesview colour swatch as well.

Comment on attachment 8967424 [details] [diff] [review] 1451211.patch Review of attachment 8967424 [details] [diff] [review]: ----------------------------------------------------------------- Hey jdescottes, think you can feedback this?

Comment on attachment 8967424 [details] [diff] [review] 1451211.patch Review of attachment 8967424 [details] [diff] [review]: ----------------------------------------------------------------- This looks good! Some comments but nothing major. Do you want to try and add a test here? Otherwise we can log a follow up I think. ::: devtools/client/shared/autocomplete-popup.js @@ +10,4 @@ > const {HTMLTooltip} = require("devtools/client/shared/widgets/tooltip/HTMLTooltip"); > const EventEmitter = require("devtools/shared/event-emitter"); > const {PrefObserver} = require("devtools/client/shared/prefs"); > +let {colorUtils} = require("devtools/shared/css/color"); Can we use `const` for consistency with the other imports? @@ +472,5 @@ > + colorSwatch.style.cssText = "background-color: " + item.postLabel; > + postDesc.appendChild(colorSwatch); > + let labelDesc = this._document.createElementNS(HTML_NS, "span"); > + labelDesc.textContent = item.postLabel; > + postDesc.appendChild(labelDesc); We can avoid using two spans here: postDesc.textContent = item.postLabel; if (isValidColor) { let colorSwatch = // create color swatch ... postDesc.insertBefore(colorSwatch, postDesc.childNodes[0]); } (and no need for the else block this way!) @@ +618,5 @@ > }, > > /** > + * Determines if the specified colour object is a valid colour, and if > + * is not a "special value" other than transparent nit: "if is" -> "if it is" ? ::: devtools/client/themes/common.css @@ +108,4 @@ > text-align: end; > } > > +.devtools-autocomplete-listbox .autocomplete-item > .autocomplete-postlabel > .autocomplete-swatch { Unless really necessary, can we use a less specific selector. Maybe just ".devtools-autocomplete-listbox .autocomplete-swatch" @@ +120,5 @@ > + display: inline-block; > + position: relative; > +} > + > +.devtools-autocomplete-listbox .autocomplete-item > .autocomplete-postlabel > .autocomplete-colorswatch::before { same comment @@ +134,5 @@ > + left: 0; > + right: 0; > + bottom: 0; > + z-index: -1; > +} not an issue: This is the fourth spot where we duplicate this CSS code, we should log a follow up Bug to mutualize this.

I have addressed Julian's concerns and made those edits. I have also added testing case for the feature. These testing cases are: - Displays for valid colour values - Doesn't display for invalid colours or non-colour values - Works with all legal CSS colour values Please note that it does not display a swatch for the "transparent" value. I intend to look into it to see why my implementation doesn't allow for it. Additionally, I wasn't sure if I should add the colorSwatch tests into the postLabel section or keep it separate. They have been added to it as I believed they are related, and the swatch tests uses similar variables so this removed some duplicate variable declarations. They can be removed if you see it fit though.

Comment on attachment 8968705 [details] [diff] [review] 1451211.patch Review of attachment 8968705 [details] [diff] [review]: ----------------------------------------------------------------- Nice work Sarah! Just some minor comments. I'm switching to r+. If you upload a patch with a commit message, we can land this for Firefox 61 :) ::: devtools/client/shared/test/browser_inplace-editor_autocomplete_css_variable.js @@ +26,3 @@ > // ] > const testData = [ > + ["v", "v", -1, 0, null, false], nit: in other tests where we have arrays like that, we define true consts to make the data more readable. Here that could look like const COLORSWATCH = true; const testData = [ ["v", "v", -1, 0, null, !COLORSWATCH], // ... ["VK_DOWN", "var(--ghi)", 2, 9, "#00FF00", COLORSWATCH], ] See an example in tree with devtools/client/inspector/rules/test/browser_rules_completion-new-property_02.js ::: devtools/client/shared/test/helper_inplace_editor.js @@ +118,5 @@ > + // Determines if there is a color swatch attached to the label > + // and if the color swatch contains a valid color > + let length = selectedElement.getElementsByClassName( > + "autocomplete-swatch autocomplete-colorswatch").length; > + let valid = editor.popup._isValidColor(postLabel); I don't think we should test this implementation internal as part of an integration test. This would be appropriate in a unit test for devtools/shared/css/color.js Knowing that "editor.popup._isValidColor(postLabel)" does not check if the autocomplete popup is also calling this same code to build its popup. Testing that there is a color swatch is enough. (Additionally you could check that the background color of the color swatch matches the post label.)

I've added the suggested changes by Julian. To accommodate the removal of checking for a valid colour type it now checks if the background colour of the swatch matches the postLabel's value. When looking at the spans, all the bg colour values are transferred to an RGB/RGBA format unless it's a textual name (BlueViolet). This caused issues when checking if the bg colour matched the post label text, so to address this I changed both from their previous colour type to an RGBA format and compared their values. Since colour values have different representations that represent the same colour I believed it was okay to change to a specific format to test equality. A commit message has also been added, if the format is incorrect please let me know.

Comment on attachment 8969000 [details] [diff] [review] 1451211.patch Review of attachment 8969000 [details] [diff] [review]: ----------------------------------------------------------------- Thanks Sarah! A few nits + also the commit message should be r=jdescottes :) ::: devtools/client/shared/autocomplete-popup.js @@ +468,5 @@ > + if (this._isValidColor(item.postLabel)) { > + let colorSwatch = this._document.createElementNS(HTML_NS, "span"); > + colorSwatch.className = "autocomplete-swatch autocomplete-colorswatch"; > + colorSwatch.style.cssText = "background-color: " + item.postLabel; > + console.log(colorSwatch.style.backgroundColor); this console.log should be removed :) ::: devtools/client/themes/common.css @@ +120,5 @@ > + display: inline-block; > + position: relative; > +} > + > +..devtools-autocomplete-listbox .autocomplete-colorswatch::before { must have missed it last time: only one dot here

Adjusted the last nits and changed the commit message. Also remembered to run it through ESLint and fixed any issues there.

Comment on attachment 8969029 [details] [diff] [review] 1451211.patch Review of attachment 8969029 [details] [diff] [review]: ----------------------------------------------------------------- great, thanks for amending the patch.

Pushed by jdescottes@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/b8e88898b0f4 Show a colour swatch next to colour values in the CSS variable autocomplete postlabel. r=jdescottes

I have documented this: https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/How_to/Examine_and_edit_CSS#CSS_variable_autocompletion And added a note to the Fx61 rel notes: https://developer.mozilla.org/en-US/Firefox/Releases/61#Developer_tools Does this look OK? Please let me know if you think anything else is needed for the docs. Thanks!

Thi(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #12) > I have documented this: > https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/How_to/ > Examine_and_edit_CSS#CSS_variable_autocompletion > > And added a note to the Fx61 rel notes: > https://developer.mozilla.org/en-US/Firefox/Releases/61#Developer_tools > > Does this look OK? Please let me know if you think anything else is needed > for the docs. Thanks! We should also try and capture Bug 1431949. We are actually showing a tooltip along the autocomplete values which shows the value of these CSS Variables. In this bug, we added a color swatch next to the color values in the tooltip next to these CSS Variable names.

(In reply to Gabriel [:gl] (ΦωΦ) from comment #13) > Thi(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #12) > > I have documented this: > > https://developer.mozilla.org/en-US/docs/Tools/Page_Inspector/How_to/ > > Examine_and_edit_CSS#CSS_variable_autocompletion > > > > And added a note to the Fx61 rel notes: > > https://developer.mozilla.org/en-US/Firefox/Releases/61#Developer_tools > > > > Does this look OK? Please let me know if you think anything else is needed > > for the docs. Thanks! > > We should also try and capture Bug 1431949. We are actually showing a > tooltip along the autocomplete values which shows the value of these CSS > Variables. In this bug, we added a color swatch next to the color values in > the tooltip next to these CSS Variable names. OK, cool — I've referenced that bug in both places now too.

One correction is that it shows the variable values for all your CSS variables in this autocomplete tooltip. It is not limited to just color variables, but we do show the color swatch in the color variable values.