1224819 - Modifying (via color picker popup) the color value (that's defined via javascript) of an element, displays it in 'rules view' in RGB format even if you have set a different 'Default color unit' in 'Toolbox Options'

Status: RESOLVED WORKSFORME

(DevTools :: Inspector: Rules, defect, P2)

Description

[Kostas]

To reproduce: (using nightly 45 x64 2015-11-142 with win10x64) -it also occurs in Firefox 42 stable-

- open Devtools|Toolbox Options|'Default color unit' and set it (from 'As authored') to e.g. 'Hex' (or it could be: HSL(A))
- open https://bugzilla.mozilla.org/
- open Web Console and run this command:
> document.querySelector('#welcome').style.color = "red";
- rightclick on the "Welcome to bugzilla" text (which is now red) and press 'Inspect Element'
- notice in Inspector|Rules view|'.element' selector, that it's displayed in hex (#f00), i.e. as expected.
- now click the (red) color sample next to the 'color' property, so that the color picker value popup appears, and change the color to a different one.
Notice that the changed color is displayed in hex format for as long as the color picker popup is shown, BUT if you close the popup via clicking anywhere on the page, then it's converted in RGB format instead.



PS. I've noticed that while examining modified colors of elements via executing a userscript of mine.

Comment 1

[Patrick Brosset <:pbro>]

Tom might know what's happening here (btw, could this be a mentored good first bug?)

Comment 2

[Tom Tromey :tromey]

Offhand I am not sure.  I would have to debug to see exactly where this is happening.
I agree this could be a good mentored first bug.

Comment 3

[Saira haris Ali]

Hi , I would like to work on this bug . But this is my first project and I can not find the files to remove this bug. So if you can please provide some more information on this?

Comment 4

[Saira haris Ali]

Ok I find this link how to open browser tool box . 

https://developer.mozilla.org/en-US/docs/Tools/Browser_Toolbox

Comment 5

[Tom Tromey :tromey]

(In reply to Saira haris Ali from comment #3)
> Hi , I would like to work on this bug . But this is my first project and I
> can not find the files to remove this bug. So if you can please provide some
> more information on this?

I'd be happy to try :)

The color picker code is in devtools/client/shared/widgets/Tooltip.js.
The rule view itself is devtools/client/styleinspector/rule-view.js.
The color swatch is hooked up here:
https://dxr.mozilla.org/mozilla-central/source/devtools/client/styleinspector/rule-view.js#3478

I would probably start debugging by:
* Get read to reproduce the bug; in particular make sure that the inspector
  is open (this ensures that rule-view.js has been loaded)
* Open the browser toolbox (you'll have to agree to debug your firefox)
* In the browser toolbox, open rule-view.js and set a breakpoint on _onSwatchCommit:
https://dxr.mozilla.org/mozilla-central/source/devtools/client/styleinspector/rule-view.js#3793
* Reproduce the bug and step through to see what goes wrong.
  Maybe the bug has already happened before this point, in which case you'll have
  to debug the color-picking tooltip.

I hope this helps!  Feel free to ask more questions, etc.

Comment 6

[Saira haris Ali]

Thank you . I have followed your steps and want to continue working on it . Can you assign me this?

Comment 7

[abay.jashibekov]

Hello, can I work on this bug?

Comment 8

[Patrick Brosset <:pbro>]

(In reply to abay.jashibekov from comment #7)
> Hello, can I work on this bug?
Yes, there has been no activity for the past 2,5 months. I'll assign it to you.
There are instructions in comment 5. Please first make sure you go over the contribution guide to install the right things and get your environment set up: https://wiki.mozilla.org/DevTools/Hacking

Comment 9

[abay.jashibekov]

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #8)
> (In reply to abay.jashibekov from comment #7)
> > Hello, can I work on this bug?
> Yes, there has been no activity for the past 2,5 months. I'll assign it to
> you.
> There are instructions in comment 5. Please first make sure you go over the
> contribution guide to install the right things and get your environment set
> up: https://wiki.mozilla.org/DevTools/Hacking

Thanks! Will work on that!

Comment 10

[abay.jashibekov]

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #8)
> (In reply to abay.jashibekov from comment #7)
> > Hello, can I work on this bug?
> Yes, there has been no activity for the past 2,5 months. I'll assign it to
> you.
> There are instructions in comment 5. Please first make sure you go over the
> contribution guide to install the right things and get your environment set
> up: https://wiki.mozilla.org/DevTools/Hacking

Hello, Patrick! Wanted to ask whether the file 'rules-view.js' is still present in the current package? I cannot find it at the moment

Comment 11

[Patrick Brosset <:pbro>]

(In reply to abay.jashibekov from comment #10)
> Hello, Patrick! Wanted to ask whether the file 'rules-view.js' is still
> present in the current package? I cannot find it at the moment
Whoops, sorry. Indeed, there was sort of a refactor a few weeks ago. The whole styleinspector directory was moved, the files renamed and split. But the code is the same.

In comment 5, Tom was talking about a color swatch being hooked in rule-view.js. This is now done in 
\devtools\client\inspector\rules\views\text-property-editor.js
Search for "this.ruleView.tooltips.colorPicker.addSwatch"

Tom then mentioned debugging onSwatchCommit. This code is also now in text-property-editor.js. Search for "_onSwatchCommit: function() {" to find it.

Comment 12

[abay.jashibekov]

(In reply to Patrick Brosset [:pbrosset] [:pbro] from comment #11)
> (In reply to abay.jashibekov from comment #10)
> > Hello, Patrick! Wanted to ask whether the file 'rules-view.js' is still
> > present in the current package? I cannot find it at the moment
> Whoops, sorry. Indeed, there was sort of a refactor a few weeks ago. The
> whole styleinspector directory was moved, the files renamed and split. But
> the code is the same.
> 
> In comment 5, Tom was talking about a color swatch being hooked in
> rule-view.js. This is now done in 
> \devtools\client\inspector\rules\views\text-property-editor.js
> Search for "this.ruleView.tooltips.colorPicker.addSwatch"
> 
> Tom then mentioned debugging onSwatchCommit. This code is also now in
> text-property-editor.js. Search for "_onSwatchCommit: function() {" to find
> it.

Hi, Patrick! Have looked at the code, seems like the unwanted change is happening somewhere in setPropertyValue of rule.js. However, I encounter an issue with debugging (which limits my debugging capabilities a lot): when I open up a color picker, this triggers a breakpoint and my mouse becomes of no use, as I cannot use it to click at all. Is there a way to go about it?

Comment 13

[Patrick Brosset <:pbro>]

(In reply to abay.jashibekov from comment #12)
> Hi, Patrick! Have looked at the code, seems like the unwanted change is
> happening somewhere in setPropertyValue of rule.js. However, I encounter an
> issue with debugging (which limits my debugging capabilities a lot): when I
> open up a color picker, this triggers a breakpoint and my mouse becomes of
> no use, as I cannot use it to click at all. Is there a way to go about it?
Yes there is. In the Browser Toolbox, go the settings panel (click on the cog icon near the top right corner), then in this panel, check the option "Disable popup autohide" (in the "Available Toolbox Buttons" section).
Doing this will keep all popups always visible on screen. So you can click to open a color picker, and it will stay there.

Comment 14

[abay.jashibekov]

Attachment: Bug Fix

Comment 15

[abay.jashibekov]

Submitting the potential bug fix. Since we save property name and value in this.committed (usually for restoring), but we update them in _onValueDone, it would be sensible to obtain the property attributes from this.committed to display those properties. Have tested workability with all of the default color types. Would be glad to receive feedback, thanks!

Comment 16

[abay.jashibekov]

Attachment: 0001-Bug-1224819-Fixed.patch

Please ignore the previous patch. 
This one should be working better. I've noticed that parserOptions has defaultColorType, which if set to true, deals with all the undesired conversions between different formats. I would be happy to get some feedback! Thanks!

Comment 17

[Patrick Brosset <:pbro>]

Comment on attachment 8723911 [details] [diff] [review]
0001-Bug-1224819-Fixed.patch

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

Redirecting this bug to Mike, as I have a bunch of other reviews to do today, and Mike has been working with color formats in the rule-view more than me.

Comment 18

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

Comment on attachment 8723911 [details] [diff] [review]
0001-Bug-1224819-Fixed.patch

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

If the property has been changed:
- We choose not to use the default color type because we want the property in the exact way the user typed it.
- We put a green line next to the property to show that it has been edited.

Because we get propDirty from a store object (a cache of changed properties and typed values) we can use the user entered value and green line on the rule even when it is used on multiple nodes.

Your change will not break the display of green lines but will always force the display of the default color type... even when it is typed in. This is not obvious unless you choose a default color unit other than "As Authored."

So with your change if you choose hex as the default color type and type white the color will instantly change to #fff when the value should remain as the user had typed it.

Basically, typed values should always be "as typed" no matter which default color unit is chosen.

Comment 19

[Patrick Brosset <:pbro>]

Triaging (filter on CLIMBING SHOES).

Comment 20

[Patrick Brosset <:pbro>]

[good first bug] whiteboard -> keyword mass change

Comment 21

[Louis Christopher]

This bug seems to be fixed in the latest version of Firefox(tested on Firefox 55 and up).

Comment 22

[Suhail Singh Bains]

Hi,
This would be first ever bug to dive into.
Is this bug in Firefox Dev Edition?

Comment 23

[Razvan Caliman [:rcaliman]]

Attachment: hex color preserved after color picker edit

This bug seems to be fixed in Firefox Nightly 63.0a1

Comment 24

[Paarmita Bhargava]

Hey,
If this bug is still open, I would like to work on it.

Comment 25

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

As Rasvan mentioned in comment 23, this bug appears to have been fixed some time ago.