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

RESOLVED FIXED in Firefox 61

Status

P3
normal
RESOLVED FIXED
8 months ago
6 months ago

People

(Reporter: gl, Assigned: sarahchilds19)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

unspecified
Firefox 61
dev-doc-complete

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

8.07 KB, patch
jdescottes
: review+
Details | Diff | Splinter Review
Comment hidden (empty)
(Reporter)

Updated

8 months ago
Priority: -- → P3
(Assignee)

Comment 1

8 months ago
Created attachment 8967424 [details] [diff] [review]
1451211.patch

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)
(Reporter)

Comment 2

8 months ago
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+
(Assignee)

Comment 4

8 months ago
Created attachment 8968705 [details] [diff] [review]
1451211.patch

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+
(Assignee)

Comment 6

8 months ago
Created attachment 8969000 [details] [diff] [review]
1451211.patch

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+
(Assignee)

Comment 8

8 months ago
Created attachment 8969029 [details] [diff] [review]
1451211.patch

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+

Comment 10

8 months ago
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

Comment 11

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b8e88898b0f4
Status: ASSIGNED → RESOLVED
Last Resolved: 8 months ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Keywords: dev-doc-needed

Updated

6 months ago
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)
Keywords: dev-doc-needed → dev-doc-complete
(Reporter)

Comment 13

6 months ago
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.
(Reporter)

Comment 15

6 months ago
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.