775135 - Add color type dropdown to rule and computed views

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

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

We should add a color dropdown to the rule and computed views. The choices should be:
- nickname
- hex
- shortHex
- rgb
- rgba
- hsl
- hsla

If changing a color will lose data e.g. alpha channels then it shouldn't be changed.

Comment 1

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

Attachment: Html file from previous bug that includes some useful methods

Comment 2

[sys.sgx]

adding: a box having suggestions for similar color hues of the element.

Comment 4

[Ravisankar Sivasubramaniam]

Hi David,

Would you please assign this defect to me?

Would you also let me know if the following pages need to be updated?  

https://wiki.mozilla.org/DevTools/Features/StyleInspector
https://wiki.mozilla.org/DevTools/Features/Highlighter

What is the driver behind this requirement?

If the user selects one among the dropdown values, which elements' color have to be changed?  Where should the dropdown be positioned?  Is there an internal state variable that has to be updated with the selected value or will this internal state variable be a new one.

Comment 5

[Ravisankar Sivasubramaniam]

David:  Am I correct in assuming that the dropdown contains only the unit of color and not the value of the color itself?

Comment 6

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

(In reply to Ravisankar Sivasubramaniam from comment #4)
> Hi David,
> 
> Would you please assign this defect to me?
> 

Sure, done.

> Would you also let me know if the following pages need to be updated?  
> 
> https://wiki.mozilla.org/DevTools/Features/StyleInspector
> https://wiki.mozilla.org/DevTools/Features/Highlighter
> 

Not until the patch has been landed.

> What is the driver behind this requirement?
> 

The default view is rgb but users often prefer to use their favorite color types.

> If the user selects one among the dropdown values, which elements' color
> have to be changed?

All color values in the selected style tool should be changed if changing their value will not lose anything e.g. rgba with an alpha value should not be converted to rgb.

> Where should the dropdown be positioned?

I would say the top right of each style view.

> Is there an
> internal state variable that has to be updated with the selected value or
> will this internal state variable be a new one.

You would need to create a new variable.

(In reply to Ravisankar Sivasubramaniam from comment #5)
> David:  Am I correct in assuming that the dropdown contains only the unit of
> color and not the value of the color itself?

Yes, it will contain the unit of color as there could be many color values involved.

Comment 7

[Jan Willem Oostendorp]

Hi,

I don't mean to be rude but could somebody tell me how far this issue is?
I've reported the 'duplicate bug' (https://bugzilla.mozilla.org/show_bug.cgi?id=777036).
And I would like to know how far this issue is.
If I could view my style colors in HEX format -and set it as a default display format- I would probably quit Firebug.

This is the most important issue which holds me back from using the build in tools.
(And probably more developers?)

Comment 8

[Ravisankar Sivasubramaniam]

Hi Jan,

This is my first firefox defect and I think my mentor is out on Thanksgiving vacation.  I don't want to hold your work. I see that you are willing to fix this defect yourself (per bug 777036).  If so please feel free to assign this defect to yourself and fix the defect.

Comment 9

[Jan Willem Oostendorp]

Hi Ravisankar,

Although I would like to I can't.
I'm a php/html developer and have no experience in C / CC+ and such :(
So I wish you good luck with it.

Comment 10

[Ravisankar Sivasubramaniam]

Hi Michael,

I am extremely sorry to have wasted everyone's time here. I am not able to get to work on this defect due to profession work pressure.  would you please reassign this defect to someone else?

Comment 11

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

Of course, we all know that it can be difficult to find time to contribute during busy times.

Comment 12

[Jorge Luis Mendez [:jlmendezbonini]]

Hi Michael,

I'm willing to take this bug. Who is mentoring it? The whiteboard seems to indicate that dcamp is but you've been clarifying the information surrounding this report. Also, if you are the mentor, may I contact you via IRC?

Comment 13

[Dave Camp (:dcamp)]

Mike is probably better than I :)

Comment 15

[Sachin Hosmani [:sachin]]

I'm willing to take up this bug too.
This will be my first bug, if I am assigned.

Comment 16

[Jorge Luis Mendez [:jlmendezbonini]]

Sachin,

Go ahead and take it because I'm working on another one at the moment.

Good luck!

Comment 17

[Rob Campbell [:rc] (:robcee)]

is this really a good first bug? seems like it's a tad involved.

Comment 18

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

Good question, at the moment I am helping Sachin get his build up and running but then we can discuss what needs to be done.

We will need three things:
1. A color type dropdown.
2. A color object attached to each CSS property. It could have a method to output all color types (for searching) and a toString method that outputs the currently selected color.
3. A change to the property string that would allow it to be dynamically generated, replacing color with the selected color type.

Step 2 is mostly done (see attachment) but still needs tweaking. The color methods should return the rgba color string if attempting to convert to rgb would break the opacity.

Using this method we can simply change the type and refresh the computed view, The rule view would be more difficult though although we could spin that out into another bug.

As a first bug I don't know, it depends if Sachin thinks he will be able to do it.

Comment 19

[Sachin Hosmani [:sachin]]

Attachment: The required color unit dropdown applied to computed view

This covers the adding of a color unit drop down to computed CSS view.
It seems to work fine for me.

Comment 20

[Jan Willem Oostendorp]

Maybe I'm too early, but can you show screenshot(s)?

Comment 21

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

(In reply to Jan Willem Oostendorp from comment #20)
> Maybe I'm too early, but can you show screenshot(s)?

We have not decided how it should look at the moment. We also need to ensure that we don't switch the color if we would lose e.g. transparency. I will review the code properly when I get time.

Comment 22

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

Comment on attachment 719904 [details] [diff] [review]
The required color unit dropdown applied to computed view

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

A few small changes are needed but it works nicely.

Can you address these issues whilst you are working on the rule view?

Great job by the way.

::: browser/devtools/styleinspector/CssLogic.jsm
@@ +126,5 @@
>    _matchedRules: null,
>    _matchedSelectors: null,
>  
> +  //default color unit for style is nickname
> +  colorUnit: CssColor.COLORUNIT.nickname,

We should default to hex colors, it is what other developer tools do.

@@ +1433,5 @@
>    {
>      if (!this._value && this._cssLogic._computedStyle) {
>        try {
>          this._value = this._cssLogic._computedStyle.getPropertyValue(this.property);
> +        if(this._value.length > 0 && this._value[0] !== '"') {

if (this._value.length > 0 && !/^".*"$/.test(this._value)) {

::: browser/devtools/styleinspector/csshtmltree.xul
@@ +13,5 @@
>    %inspectorDTD;
>    <!ELEMENT loop ANY>
>    <!ATTLIST li foreach CDATA #IMPLIED>
>    <!ATTLIST div foreach CDATA #IMPLIED>
> +  <!ATTLIST localeop foreach CDATA #IMPLIED>

I suspect that this was changed by mistake?

::: browser/locales/en-US/chrome/browser/devtools/styleinspector.dtd
@@ +34,5 @@
>  <!ENTITY bestMatch             "Best Match">
>  <!ENTITY matched               "Matched">
>  <!ENTITY parentMatch           "Parent Match">
>  
> +<!-- LOCALIZATION NOTE (computedViewTitle, ruleViewTitle): These are the 

Remove the trailing space.

@@ +40,4 @@
>  <!ENTITY computedViewTitle     "Computed">
>  <!ENTITY ruleViewTitle         "Rules">
> +
> +<!-- LOCALIZATION NOTE (colorUnit): The label associated with the color unit 

Remove the trailing space.

@@ +43,5 @@
> +<!-- LOCALIZATION NOTE (colorUnit): The label associated with the color unit 
> +  -  dropdown. -->
> +<!ENTITY colorUnit             "Color Unit:">
> +
> +<!-- LOCALIZATION NOTE (colorNickName, colorHex, colorShortHex, colorRgb, colorRgba, 

Remove the trailing space.

Comment 23

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

Please also make the following changes so that the style looks better:
csshtmltree.xul
---------------
+      <!--
+      A dropdown to choose color unit
+      -->
+      <xul:hbox flex="1">
+        <xul:button type="menu" save="${colorUnit}" class="color-dropdown"
+                    oncommand="${colorUnitChanged}">
+          <xul:menupopup>
+            <xul:menuitem label="&colorNickName;"/>
+            <xul:menuitem label="&colorHex;"/>
+            <xul:menuitem label="&colorShortHex;"/>
+            <xul:menuitem label="&colorRgb;"/>
+            <xul:menuitem label="&colorRgba;"/>
+            <xul:menuitem label="&colorHsl;"/>
+            <xul:menuitem label="&colorHsla;"/>
+          </xul:menupopup>
+        </xul:button>
+      </xul:hbox>

browser/themes/linux/devtools/csshtmltree.css,
browser/themes/osx/devtools/csshtmltree.css &
browser/themes/windows/devtools/csshtmltree.css
-----------------------------------------------
 .legendKey {
   margin: 0 5px;
 }
 
+.color-dropdown {
+  min-width: 30px;
+  width: 30px;
+  list-style-image: url("chrome://browser/skin/devtools/option-icon.png");
+  -moz-image-region: rect(0px 16px 16px 0px);
+}
+
+.color-dropdown:hover {
+  -moz-image-region: rect(0px 32px 16px 16px);
+}
+
+.color-dropdown > hbox > dropmarker {
+  display: none;
+}

Comment 24

[Sachin Hosmani [:sachin]]

Attachment: Dropdowns applied to both Computed and Rule Views

The dropdowns were made button dropdowns as Mike suggested.

As I tested, 

Computed View :
Works as before but if the color has an alpha component, it shows 
rgba for: rgb, hex, shortHex; and 
hsla for: hsl.

Rule View :
It changes the color value(s) similarly. 
The change is shown right after editing a color's value, and also after creating a new Css property.

However, when I switched to Computed View and came back to Rule View, it showed rgb values, regardless of what the chosen unit was.

The dropdown must be put in a better place.

And should we make the chosen color units, preferences?

Comment 25

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

Attachment: Rule View with color dropdown

Comment 26

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

Attachment: Computed View with color dropdown

We need UX help here. The top of the rule view is now ugly and the top of the computed view is too crowded. Of course the cog dropdown should not have a label.

I think that for both views just the icon would work and for the computed view the icon should be before the search box.

Or you may have a flash of genius and come up with something completely different ... what do you guys think?

Comment 27

[Jan Willem Oostendorp]

The screens leave me with a few questions:
1. Does this drop down only appear on the computed tab?
2. If so, does it only affect the computed tab?
3. Is it safed so it won't change between pages?

#2 & #3 would make it nearly useless

@Michael Ratcliffe I don't have any better idea's about where to show this?

In time there might be enough settings to move it into a lose tab. But to create that just for one setting might be a bit to much...

Comment 28

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

(In reply to Jan Willem Oostendorp from comment #27)
> The screens leave me with a few questions:
> 1. Does this drop down only appear on the computed tab?

No, the attachments in comment 25 and comment 26 show it in the rule and computed views. The dropdown could even be added to the style editor but I am not sure that we want to do this.

> 2. If so, does it only affect the computed tab?

The dropdown in the rule view affects the rule view and the dropdown in the computed view affects the computed view.

> 3. Is it saved so it won't change between pages?
>

We will save the value.

> #2 & #3 would make it nearly useless
> 

It would certainly make it less useful.

> @Michael Ratcliffe I don't have any better idea's about where to show this?
> 

The two people I copied into the bug (comment 26) are both very talented designers and they are thinking about how we are best doing this.

> In time there might be enough settings to move it into a lose tab. But to
> create that just for one setting might be a bit to much...

We are planning on adding more settings soon. Sachin has been doing a great job but we have some changes to the UI coming and we need to wait for the designers to get this up and running ... this is in the hands of the designers.

Comment 29

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

When bug 848745 lands we can use the color name to triplet (and back) part of the API to improve our color look-ups.

Comment 30

[Paul Rouget [:paul]]

Comment on attachment 724677 [details]
Computed View with color dropdown

This is too packed. Also - please look at the new computed view design in bug 836233.

Comment 31

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

We discussed this in California ... we need to move this into the context menu.

Comment 32

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

New color conversion examples:
let domUtils = Cc["@mozilla.org/inspector/dom-utils;1"].getService(Ci.inIDOMUtils);

DOMUtils.rgbToColorName(255, 0, 0); // returns "red"
DOMUtils.colorNameToRGB("red"); // returns {b=0, g=0, r=255}

function isColorName(name) {
  try {
    DOMUtils.colorNameToRGB(name); // Throws on illegal value.
    return true
  } catch(e) {
    return false;
  }
}

domUtils should already be available in most places that you will need it via a lazy getter.

Comment 33

[Paul Rouget [:paul]]

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #31)
> We discussed this in California ... we need to move this into the context
> menu.

Oh yes we did. That was a good idea :)

Comment 34

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

When Paul gets back from vacation he will fix bug 855499 and then we will be able to use the context menu.

Comment 35

[Sachin Hosmani [:sachin]]

Alright

Comment 36

[Riddhi]

Hi,

Is this bug fixed?

Comment 37

[Sachin Hosmani [:sachin]]

(In reply to Riddhi from comment #36)
> Hi,
> 
> Is this bug fixed?

No, it's not. It depends on Bug 855499. We're waiting for that to get fixed.

Comment 38

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

(In reply to Riddhi from comment #36)
> Hi,
> 
> Is this bug fixed?

No, "Select color type" needs to go into an HTML5 context menu because we don't want to clutter the interface.

At the moment our context menus have been disabled. We are going to have a generic system that can be used by all of our tools. This will include a basic HTML5 context menu with copy & paste but each tool will be able to add entries to it that will be visible when the tool is selected.

When Paul Rouget returns from vacation he is planning on implementing it. This bug is on hold until then.

Comment 39

[Jan Willem Oostendorp]

I'm curious what version Of Firefox might we expect this bug to be implemented?
Or am I getting ahead of things.

I saw that in FF22 you will be able top put the toolbox on the side (source:mzl.la/Yo3I7T). In combination with this bug I can (probably) say goodbye to Firebug.

Comment 40

[Jan Willem Oostendorp]

I've been playing around with the nightly (25.0a1)
And in the dev menu I see an options tab. Shouldn't this option go there?
From a UX standpoint it makes sense to have all options in one place. I don't expect users will change it often.
From a technical standpoint, this tab already exists and is functioning. Plus the current dependency (#855499) hasn't even been started on.

Comment 41

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

We are planning on adding this to the context menu (which we have temporarily removed). Adding it to the options tab may be one option but reactivating the context menus is not a big job and makes more sense.

Comment 42

[Jan Willem Oostendorp]

If it makes more sense is debatable. (Why not add it on both?)

Fact is this bug has been open for a year and the context menu's hasn't been worked on yet. So big deal or not it isn't ready, the settings tab is.

Comment 43

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

(In reply to Jan Willem Oostendorp from comment #42)
> If it makes more sense is debatable. (Why not add it on both?)
> 
> Fact is this bug has been open for a year and the context menu's hasn't been
> worked on yet. So big deal or not it isn't ready, the settings tab is.

The reason that it would make more sense in my opinion is because then all CSS related options could be available from the context menu in CSS tools. If we used the options panel and I wanted to display colors as HEX in the computed view but RGBA in the rule view and HSLA in the style editor? That would mean having three separate options.

Saying that, temporarily adding this to the options panel would also makes sense... you are more than welcome to work on this and bug 855499 if you would like to improve the experience for everybody.

Comment 44

[Jan Willem Oostendorp]

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #43)
> you are more than welcome to work on this and bug 855499 if you
> would like to improve the experience for everybody.

Although I would love to help -rather then stand on the sidelines- I only know php/html/css/js so I doubt how much I can help :(

Comment 45

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

Bug 911748 fixed this.

Comment 46

[Kevin]

Hi, this is marked as resolved but  I can't see any way of selecting Hex values. Can anyone tell me how please? Its driving me nuts. Who on earth uses RGB to manage colors in CSS?
thanks

Comment 47

[Jan Willem Oostendorp]

(In reply to Kevin from comment #46)
> Hi, this is marked as resolved but  I can't see any way of selecting Hex
> values. Can anyone tell me how please? Its driving me nuts. Who on earth
> uses RGB to manage colors in CSS?
> thanks

This will only apply from firefox 27+, currently in nightly build

Comment 48

[Panos Astithas (he/him) [:past] (please ni?)]

Correction: Firefox 27 is currently in Aurora. If you want to try one of those builds, go to aurora.mozilla.org.

Comment 49

[jon.tech.uk]

Bug in the new cold. Steps to replicate (Firefox 27b2):
- Select an element in the inspector
- Add an inline element ie color:#ffffff
- Untick the box next to the new line
- The rule is now RGB format
The bug is only for new inline elemnts (top box in rules)

Comment 50

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

(In reply to jon.tech.uk from comment #49)
> Bug in the new cold. Steps to replicate (Firefox 27b2):
> - Select an element in the inspector
> - Add an inline element ie color:#ffffff
> - Untick the box next to the new line
> - The rule is now RGB format
> The bug is only for new inline elemnts (top box in rules)

This means that you are still in rgb mode. Change the default in the options panel and it should work for you.

Comment 51

[konecny.mike]

Using Aurora 28.0a2.
Is there any way to make the color picker (after clicking the coloured dot) NOT change the format to rgba?
I realise this is because the picker also supports the alpha channel, but if alpha==1 (which is most of the times), it should default to showing the color in the format the user has requested (hex in my case).
Thank you!

Comment 52

[Boris Prpic]

Same question.

Is that a bug in 28? I think it worked before.

Comment 53

[soccerjonp]

This is happening for me in FF 28. Is it a regression? It switches to RGBA whenever the color picker is used even though I've set HEX as my default.