Closed Bug 940507 Opened 8 years ago Closed 7 years ago

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

Categories

(DevTools :: Inspector, defect)

28 Branch
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 34

People

(Reporter: ntim, Assigned: miker, Mentored)

References

(Blocks 1 open bug)

Details

(Whiteboard: [fixed by bug 1040701])

Attachments

(2 files)

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
Blocks: 889638
I can reproduce.
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
Whiteboard: [good-first-bug][lang=js][mentor=pbrosset]
I might work on this, just need to know how I can set up my work environment.
Flags: needinfo?(pbrosset)
(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.
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.
Flags: needinfo?(pbrosset)
Depends on: 953404
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.
(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.
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.
(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.
Blocks: 953404
No longer depends on: 953404
(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.
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.
(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 pbrosset@mozilla.com) for your patch.
Whiteboard: [good-first-bug][lang=js][mentor=pbrosset] → [good first bug][lang=js][mentor=pbrosset]
Assignee: nobody → delapouite
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.
Attachment #8355220 - Flags: review?(pbrosset)
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?
Flags: needinfo?(ntim007)
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 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()|).
Attachment #8355220 - Flags: review?(pbrosset)
Filed follow-up bug 965181 to make the color-picker respect the authored unit.
Status: NEW → ASSIGNED
Flags: needinfo?(ntim007)
Mentor: pbrosset
Whiteboard: [good first bug][lang=js][mentor=pbrosset] → [good first bug][lang=js]
This seems recently fixed, do you have an idea of what might have fixed it ?
Flags: needinfo?(pbrosset)
(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.
Flags: needinfo?(pbrosset) → needinfo?(mratcliffe)
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.
Flags: needinfo?(mratcliffe)
Assignee: delapouite → mratcliffe
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Depends on: 1040701
Resolution: --- → FIXED
Whiteboard: [good first bug][lang=js] → [fixed by bug 1040701]
Target Milestone: --- → Firefox 34
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.