Closed Bug 1451211 Opened 6 years ago Closed 6 years ago

Show a color swatch next to color values in the CSS variable autocomplete postlabel

Categories

(DevTools :: Inspector, enhancement, P3)

enhancement

Tracking

(firefox61 fixed)

RESOLVED FIXED
Firefox 61
Tracking Status
firefox61 --- fixed

People

(Reporter: gl, Assigned: sarahchilds19)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 3 obsolete files)

      No description provided.
Priority: -- → P3
Attached patch 1451211.patch (obsolete) — Splinter Review
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.
Attachment #8967424 - Flags: feedback?(gl)
Comment on attachment 8967424 [details] [diff] [review]
1451211.patch

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

Hey jdescottes, think you can feedback this?
Attachment #8967424 - Flags: feedback?(gl) → feedback?(jdescottes)
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.
Attachment #8967424 - Flags: feedback?(jdescottes) → feedback+
Attached patch 1451211.patch (obsolete) — Splinter Review
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.
Attachment #8967424 - Attachment is obsolete: true
Attachment #8968705 - Flags: feedback?(jdescottes)
Attachment #8968705 - Flags: feedback?(gl)
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.)
Attachment #8968705 - Flags: feedback?(jdescottes) → review+
Attached patch 1451211.patch (obsolete) — Splinter Review
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.
Attachment #8968705 - Attachment is obsolete: true
Attachment #8968705 - Flags: feedback?(gl)
Attachment #8969000 - Flags: feedback?(jdescottes)
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
Attachment #8969000 - Flags: feedback?(jdescottes) → review+
Attached patch 1451211.patchSplinter Review
Adjusted the last nits and changed the commit message. Also remembered to run it through ESLint and fixed any issues there.
Attachment #8969000 - Attachment is obsolete: true
Attachment #8969029 - Flags: feedback?(jdescottes)
Comment on attachment 8969029 [details] [diff] [review]
1451211.patch

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

great, thanks for amending the patch.
Attachment #8969029 - Flags: feedback?(jdescottes) → review+
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
https://hg.mozilla.org/mozilla-central/rev/b8e88898b0f4
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Product: Firefox → DevTools
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!
Flags: needinfo?(sarahchilds19)
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.
Flags: needinfo?(sarahchilds19)
(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.
You need to log in before you can comment on or make changes to this bug.