940507 - Changing a color with the color picker tooltip will not respect the color unit setting

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Tim Nguyen :ntim]

Let's say you have HEX colors set for the color unit setting.
Well, the color picker tooltip will return rgba(0,0,0,1) instead of #000

Comment 1

[Paul Rouget [:paul]]

I can reproduce.

Comment 2

[Patrick Brosset <:pbro>]

Indeed, the colorpicker today returns an rgba type of color in all cases, and doesn't care about the original unit.
It'd be easy enough to fix by checking the unit and then using the css-color.js utility to convert the color picker's output.

Comment 3

[Tim Nguyen :ntim]

I might work on this, just need to know how I can set up my work environment.

Comment 4

[Brian Grinstead [:bgrins]]

(In reply to ntim007 from comment #3)
> I might work on this, just need to know how I can set up my work environment.

We have a guide for getting your environment set up here: https://wiki.mozilla.org/DevTools/Hacking.  Feel free to comment here or drop by #devtools on IRC if you have any questions.

Comment 5

[Patrick Brosset <:pbro>]

By all means do drop by #devtools, we'll help you out from there.
There's also a very helpful set of videos here: http://codefirefox.com/ which will help you set up your environment.

As for the bug, here are a few initial thoughts:

- We have a color utility class here: toolkit/devtools/css-color.js and, although I haven't used it yet, it seems pretty straightforward. Instantiate a new CssColor object, passing it whatever recognizable color value, and you can then read whatever other unit from the object (there's a bunch of getters on the class)

- You will have to detect the color unit originally set in the css rule. For this, the first idea that comes to mind would be to refactor css-color a little bit and extract the css color unit detection part (it is today within _getRGBATuple apparently).

- Once you have those things ready, look into browser/devtools/styleinspector/rule-view.js, there's some code that attach each color swatch to the color picker tooltip. And while doing so, it defines a onPreview and onCommit set of functions which are called when the color is changed in the tooltip. This is where the color conversion should go.

Comment 6

[Bruno Heridet [:Delapouite]]

Attachment: Added authoredUnit computed prop to css-color helper

Hello

Here's my attempt for the first part of the job, following the hints given by Patrick in comment 5.

The attached patch adds a new computed property authoredUnit on the CssColor object.

Examples : 


let color = new colorUtils.CssColor("#F00");
// color.authoredUnit === "hex"
let color = new colorUtils.CssColor("rgb(42, 42, 42)");
// color.authoredUnit === "rgb"



authoredUnit may hold any of the units previously defined in the const (except "authored"):

CssColor.COLOR_UNITS = new Set([
  "authored",
  "hex",
  "name",
  "rgb",
  "hsl"
]);

The patch also includes the corresponding mochitests.

Comment 7

[Tim Nguyen :ntim]

(In reply to Bruno Heridet [:Delapouite] from comment #6)
> Created attachment 8355178 [details] [diff] [review]
> Added authoredUnit computed prop to css-color helper
> 
> Hello
> 
> Here's my attempt for the first part of the job, following the hints given
> by Patrick in comment 5.
> 
> The attached patch adds a new computed property authoredUnit on the CssColor
> object.
> 
> Examples : 
> 
> 
> let color = new colorUtils.CssColor("#F00");
> // color.authoredUnit === "hex"
> let color = new colorUtils.CssColor("rgb(42, 42, 42)");
> // color.authoredUnit === "rgb"
> 
> 
> 
> authoredUnit may hold any of the units previously defined in the const
> (except "authored"):
> 
> CssColor.COLOR_UNITS = new Set([
>   "authored",
>   "hex",
>   "name",
>   "rgb",
>   "hsl"
> ]);
> 
> The patch also includes the corresponding mochitests.

Thanks for the patch :) . Make sure to turn rgba/hsla(*,*,*,0) into transparent.

Comment 8

[Bruno Heridet [:Delapouite]]

Hi Tim.

I'm not quite sure I understand correctly what you are requesting about transparency. So let me detail a use case to see if it fits your vision.

A user writes the following lines in his CSS file :
background-color: rgba(12, 34, 56, 0);

Which is indeed transparent.

Then in the devtools style inspector, the user clicks on the property to pop the color wheel and tweaks the opacity to 50.

The result is now: 
background-color: rgba(12, 34, 56, 0.5);

(and not the hsla version because authoredUnit is "rgb")

"transparent" is already available as a special value :

const SPECIAL_VALUES = new Set([
  "currentcolor",
  "initial",
  "inherit",
  "transparent",
  "unset"
]);

and accessible through the boolean .transparent computed property.

Comment 9

[Patrick Brosset <:pbro>]

(In reply to Bruno Heridet [:Delapouite] from comment #6)
> Created attachment 8355178 [details] [diff] [review]
> Added authoredUnit computed prop to css-color helper
> 
> Hello
> 
> Here's my attempt for the first part of the job, following the hints given
> by Patrick in comment 5.
> 
> The attached patch adds a new computed property authoredUnit on the CssColor
> object.
I like this first change.

Comment 10

[Tim Nguyen :ntim]

(In reply to Bruno Heridet [:Delapouite] from comment #8)
> Hi Tim.
> 
> I'm not quite sure I understand correctly what you are requesting about
> transparency. So let me detail a use case to see if it fits your vision.
> 
> A user writes the following lines in his CSS file :
> background-color: rgba(12, 34, 56, 0);
> 
> Which is indeed transparent.
> 
> Then in the devtools style inspector, the user clicks on the property to pop
> the color wheel and tweaks the opacity to 50.
> 
> The result is now: 
> background-color: rgba(12, 34, 56, 0.5);
> 
> (and not the hsla version because authoredUnit is "rgb")
> 
> "transparent" is already available as a special value :
> 
> const SPECIAL_VALUES = new Set([
>   "currentcolor",
>   "initial",
>   "inherit",
>   "transparent",
>   "unset"
> ]);
> 
> and accessible through the boolean .transparent computed property.

No, if the person sets a value like rgba/hsla(*,*,*,0) using the color picker, it will automatically turn it into transparent, but only if the person has used the color picker.

Comment 11

[Bruno Heridet [:Delapouite]]

Attachment: colorPicker.patch

Alright, thanks for your explanation Tim.

So I guess, I've a bit previously over-engineered the issue after all after reading again your initial need in Comment 1.

The right place to fix it was in by adding those few lines into the Tooltip onChanged callback :


--- a/browser/devtools/shared/widgets/Tooltip.js
+++ b/browser/devtools/shared/widgets/Tooltip.js
@@ -829,7 +829,10 @@ SwatchColorPickerTooltip.prototype = Heritage.extend(SwatchBasedEditorTooltip.pr
   _onSpectrumColorChange: function(event, rgba, cssColor) {
     if (this.activeSwatch) {
       this.activeSwatch.style.backgroundColor = cssColor;
-      this.activeSwatch.nextSibling.textContent = cssColor;
+      // toString() returns the color unit defined in the option panel
+      let colorObj = new colorUtils.CssColor(cssColor);
+      let colorStr = colorObj.transparent ? "transparent" : colorObj.toString();
+      this.activeSwatch.nextSibling.textContent = colorStr;
       this.preview(cssColor);
     }
   },


Here, nextSibling corresponds to the text/input element refreshed by the colorPicker in the style editor.

I've attached this fix in the joined patch.

Comment 12

[Tim Nguyen :ntim]

(In reply to Bruno Heridet [:Delapouite] from comment #11)
> Created attachment 8355220 [details] [diff] [review]
> colorPicker.patch
> 
> Alright, thanks for your explanation Tim.
> 
> So I guess, I've a bit previously over-engineered the issue after all after
> reading again your initial need in Comment 1.
> 
> The right place to fix it was in by adding those few lines into the Tooltip
> onChanged callback :
> 
> 
> --- a/browser/devtools/shared/widgets/Tooltip.js
> +++ b/browser/devtools/shared/widgets/Tooltip.js
> @@ -829,7 +829,10 @@ SwatchColorPickerTooltip.prototype =
> Heritage.extend(SwatchBasedEditorTooltip.pr
>    _onSpectrumColorChange: function(event, rgba, cssColor) {
>      if (this.activeSwatch) {
>        this.activeSwatch.style.backgroundColor = cssColor;
> -      this.activeSwatch.nextSibling.textContent = cssColor;
> +      // toString() returns the color unit defined in the option panel
> +      let colorObj = new colorUtils.CssColor(cssColor);
> +      let colorStr = colorObj.transparent ? "transparent" :
> colorObj.toString();
> +      this.activeSwatch.nextSibling.textContent = colorStr;
>        this.preview(cssColor);
>      }
>    },
> 
> 
> Here, nextSibling corresponds to the text/input element refreshed by the
> colorPicker in the style editor.
> 
> I've attached this fix in the joined patch.

You might want to ask a review (to the mentor [e-mail]) for your patch.

Comment 13

[Manish Goregaokar [:manishearth]]

Just to let you know, if you think that your patch has reached a usable stage, edit the details and set the review flag to |? :patrick|. If it gets review+, it can go on to be checked in.

Comment 14

[Patrick Brosset <:pbro>]

Thanks for the patch.

I've played with it and it seems to fix the issue noted in the bug.
When you edit a color with the color-picker, the resulting color will be in hex if you've set your default color unit to hex (except when alpha isn't 1 in which case it will be rgba, but that's a special case).

I have to admit I was a bit confused at first because I didn't remember the bug being about respecting the the color unit *setting* but instead thought, for some reason, that it was about respecting whatever unit the color being edited was in.
Say you would edit color #454545, the result after edition would still be a hex value.
But say you would edit color rgb(100,100,100), the result would still be rgb after edition.

I'm going to review the code now anyway, cause the patch seems to work fine and makes sense in context of this bug, but I think I'll file a bug for what I just said. I think it makes sense.
Tim, what do you think about this?

Comment 15

[Bruno Heridet [:Delapouite]]

Yes Patrick I was also confused and tend to agree with your observation.

#1 In the initial request, Tim described the need to respect the default color format setting.
#2 Then, the conversation diverged for strange reasons introducing another behavior which is respecting the authored value instead.

As you said my patch fixes situation #1, the current bug.
Option #2 is also an interesting and valid approach, but then the purpose of the default color format setting lost its interest.

Comment 16

[Patrick Brosset <:pbro>]

Comment on attachment 8355220 [details] [diff] [review]
colorPicker.patch

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

Looks good!
Only one comment about |get authoredUnit|

::: browser/devtools/shared/test/browser_css_color.js
@@ +50,5 @@
>  
>      testToString(color, name, hex, hsl, rgb);
>      testColorMatch(name, hex, hsl, rgb, color.rgba);
> +    if (unit) {
> +      testColorUnit(color, authored, unit);

This is linked to my other comment about the new function you added to css-color.
This test function should be removed if the new css-color function is removed too.

@@ +151,4 @@
>  
>  function getTestData() {
>    return [
> +    // Name

Useless comment

::: toolkit/devtools/css-color.js
@@ +125,5 @@
>        return this.authored;
>      }
>    },
>  
> +  get authoredUnit() {

This is a good addition to the API, but is not used in your patch (apart from the test, but I don't think it should stay there either).
We should refrain from adding things to our public APIs if they're not needed for now.
Maybe we can keep the code and attach it to the new bug I want to file about respecting the authored unit (see comment 14).

::: browser/devtools/shared/widgets/Tooltip.js
@@ +829,4 @@
>    _onSpectrumColorChange: function(event, rgba, cssColor) {
>      if (this.activeSwatch) {
>        this.activeSwatch.style.backgroundColor = cssColor;
> +      // toString() returns the color unit defined in the option panel

Please move that comment down to the line below (next to the |colorObj.toString()|).

Comment 17

[Patrick Brosset <:pbro>]

Filed follow-up bug 965181 to make the color-picker respect the authored unit.

Comment 18

[Tim Nguyen :ntim]

This seems recently fixed, do you have an idea of what might have fixed it ?

Comment 19

[Patrick Brosset <:pbro>]

(In reply to Tim Nguyen [:ntim] from comment #18)
> This seems recently fixed, do you have an idea of what might have fixed it ?
Mike Ratcliffe worked on a bug related to this recently. I can't find the bug number anymore though. Needinfo'ing Mike as he will probably remember.

Comment 20

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

I fixed this as part of bug 1040701, which fixed the marking of changed properties. It turned out that the whole storage system (where we remember changed properties, original values etc.) was completely broken so I had to fix that and then fix all new features that had been added since the store was broken... this was one of those features.