956044 - DevTools Color swatch should have a checkered background to see alpha transparency.

Status: RESOLVED FIXED

(DevTools :: Inspector, enhancement, P3)

Description

[Tim Nguyen :ntim]

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0 (Beta/Release)
Build ID: 20131231030203

Steps to reproduce:

The title explains all.

Comment 1

[Patrick Brosset <:pbro>]

Yes, that's a good idea. I think we mentioned it at some stage, but that didn't get implemented.
That should be quick enough to add.

Comment 2

[Patrick Brosset <:pbro>]

Attachment: Screen Shot 2014-01-15 at 6.48.55 PM.png

Comment 3

[Patrick Brosset <:pbro>]

attachment 8360503 [details] shows that current situation, that is a semi transparent color will produce a semi transparent color swatch on whatever background it happens to be. So in the dark theme, that'll be on the dark background of the rule-view, which might change your perception of what the color actually is.
Using a checkered background would be better.

Comment 4

[Tim Nguyen :ntim]

Attachment: Checkered background

Do you think this graphic is good enough ? I could optimize it for speed if you want (no quality loss). I could also make a smaller grid.
If this is good enough, I might make a patch.

Comment 5

[Tim Nguyen :ntim]

Crap, I forgot that background-image shows on top of background-color.
There are 3 possible solutions :
- New element
- Using solid linear-gradient to display the color

Both require JS changes.

Comment 6

[Patrick Brosset <:pbro>]

(In reply to Tim Nguyen [:ntim] from comment #5)
> ****, I forgot that background-image shows on top of background-color.
> There are 3 possible solutions :
> - New element
> - Using solid linear-gradient to display the color
> 
> Both require JS changes.

Avoiding to include a new element would be better indeed.
Also, we could avoid using an image for the grid and instead use a css gradient.
We have a CSS class lying around in common.css (and afaik it's not used):
.devtools-tooltip-tiles {
  background-color: #eee;
  background-image: linear-gradient(45deg, #ccc 25%, transparent 25%, transparent 75%, #ccc 75%, #ccc),
    linear-gradient(45deg, #ccc 25%, transparent 25%, transparent 75%, #ccc 75%, #ccc);
  background-size: 20px 20px;
  background-position: 0 0, 10px 10px;
}

Comment 7

[Patrick Brosset <:pbro>]

Comment on attachment 8360525 [details]
Checkered background

In terms of grid, I think this is exactly what we need, but we might want to make it smaller though, the color swatches are really small.

Darrin: this is probably not of the highest importance right now, but do you think we would need one graphic per theme (one darker grid for the dark theme, and one lighter for the light one).

Comment 8

[Tim Nguyen :ntim]

(In reply to Patrick Brosset [:pbrosset] from comment #6)
> (In reply to Tim Nguyen [:ntim] from comment #5)
> > ****, I forgot that background-image shows on top of background-color.
> > There are 3 possible solutions :
> > - New element
> > - Using solid linear-gradient to display the color
> > 
> > Both require JS changes.
> 
> Avoiding to include a new element would be better indeed.
> Also, we could avoid using an image for the grid and instead use a css
> gradient.
> We have a CSS class lying around in common.css (and afaik it's not used):
> .devtools-tooltip-tiles {
>   background-color: #eee;
>   background-image: linear-gradient(45deg, #ccc 25%, transparent 25%,
> transparent 75%, #ccc 75%, #ccc),
>     linear-gradient(45deg, #ccc 25%, transparent 25%, transparent 75%, #ccc
> 75%, #ccc);
>   background-size: 20px 20px;
>   background-position: 0 0, 10px 10px;
> }

Nice :)
The only issue is that the background-image will appear on the top of the color swatch current color. Which means it won't allow you to see the color at all.

Comment 9

[Patrick Brosset <:pbro>]

so maybe to avoid inserting a new element, we can use a pseudo-element :before or :after

Comment 10

[Tim Nguyen :ntim]

(In reply to Patrick Brosset [:pbrosset] from comment #9)
> so maybe to avoid inserting a new element, we can use a pseudo-element
> :before or :after

yep, but I think you'll need to apply the background color to the pseudo element instead of the element itself when using the color picker. Not sure if this is easy with JS.

Comment 11

[Darrin Henein [:darrin]]

> Darrin: this is probably not of the highest importance right now, but do you
> think we would need one graphic per theme (one darker grid for the dark
> theme, and one lighter for the light one).

I think we are OK with the one graphic for now. Its pretty universal and after looking at other applications with dark/light themes (e.g. Adobe suite), practice is to use the grey/white background regardless.

Comment 13

[Victor Porof [:vporof][:vp]]

Or, you could use this: http://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/devtools/canvasdebugger.inc.css?from=canvasdebugger.inc.css#9

Comment 14

[Brian Grinstead [:bgrins]]

Or you could use this: http://dxr.mozilla.org/mozilla-central/source/browser/devtools/shared/widgets/spectrum.css#44

Comment 15

[Patrick Brosset <:pbro>]

Wow, so many things we can use :)
Looks like we only need someone to work on the bug then, and perhaps this should be filed as a good-first-bug too, what do you think?

Comment 16

[Brian Grinstead [:bgrins]]

Had discussed this the other day and pointed out that if we just add a container element for the swatch it is pretty easy to get this effect: http://jsfiddle.net/bgrins/7aWU8/.  Depending on how hard this will be will with the output parser, we could probably make it work with :before as mentioned in Comment 9.  These kinds of tricks can be difficult when using in XUL doc though.

Comment 17

[Tim Nguyen :ntim]

(In reply to Brian Grinstead [:bgrins] from comment #16)
> Had discussed this the other day and pointed out that if we just add a
> container element for the swatch it is pretty easy to get this effect:
> http://jsfiddle.net/bgrins/7aWU8/.  Depending on how hard this will be will
> with the output parser, we could probably make it work with :before as
> mentioned in Comment 9.  These kinds of tricks can be difficult when using
> in XUL doc though.

The problem with ::before is well, the background-color needs to be on the pseudo element not the swatch itself. This is quite tricky to do in JS, so let's avoid the ::before/::after path. (FYI z-index:-1 on ::before doesn't fix the problem).

Wrapping with a container seems like the best solution to me :)

Comment 18

[Patrick Brosset <:pbro>]

(In reply to Brian Grinstead [:bgrins] from comment #16)
> Had discussed this the other day and pointed out that if we just add a
> container element for the swatch it is pretty easy to get this effect:
> http://jsfiddle.net/bgrins/7aWU8/.
Adding an extra container around the color-swatch is super easy in the output-parser. This is where the span is built:
http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/output-parser.js#332

Comment 19

[alexharris6]

Hello, may I work on this bug? It will be my first.

Comment 20

[Patrick Brosset <:pbro>]

(In reply to alexharris6 from comment #19)
> Hello, may I work on this bug? It will be my first.
Sure, if it's your first, you'll need to set up the environment first.
You'll need to go through https://wiki.mozilla.org/DevTools/GetInvolved and https://wiki.mozilla.org/DevTools/Hacking.
bgrins is the mentor on this bug, he'll give details about the code.
If you have questions, feel free to drop by our #devtools IRC channel and ask there.

Comment 21

[Brian Grinstead [:bgrins]]

Alex, once you have the build environment running, see Comment 16 and Comment 18 for a few more details.   The main changes will be to toolkit/devtools/output-parser.js and to some CSS files.

For changes to the output-parser, we will need to have a new child element inside of the colorSwatch element.  Then the new child element will have the color applied.

For changes to the CSS, then there are 2 classes that will need to change to act more like the checker-background element in http://jsfiddle.net/bgrins/7aWU8/.  The places to modify the CSS are:

.ruleview-colorswatch -> in browser/devtools/themes/shared/ruleview.css
.computedview-colorswatch -> in 3 places so the changes need to be copied to each (sorry) browser/themes/[linux|windows|osx]/devtools/computedview.css

Comment 22

[alexharris6]

Attachment: Patch for bug 956044

First attempt submitting a patch, please let me know if I am doing it incorrectly or skipping any steps.

Comment 23

[alexharris6]

To explain the discrepancies between my patch and the potential solutions discussed above: I poked around a little bit and found that it was possible to just use a ::before pseudoelement to provide the background, it just needed to have z-index: -1, and position: absolute. I discussed in IRC with bgrins, and it was decided that this approach was preferred to adding a child node via output-parser.js.

I added to the css to all 4 files the 4 files I am aware of that deal with the colorswatch.

Bgrins and I also discussed the potential for combining .ruleview-colorswatch and .computedview-colorswatch, and putting it all in shared. It was decided that this task should be handled separately from this bugfix. I am not sure what the proper protocol is to follow up with that, since it doesn't seem like it should be logged as a bug.

Comment 24

[Tim Nguyen :ntim]

Comment on attachment 8422911 [details] [diff] [review]
Patch for bug 956044

Please request review first.

Comment 25

[Tim Nguyen :ntim]

Comment on attachment 8422911 [details] [diff] [review]
Patch for bug 956044

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

::: browser/themes/shared/devtools/ruleview.css
@@ +123,5 @@
> +.ruleview-colorswatch::before {
> +  content: '';
> +  background-color: #eee;
> +  background-image: linear-gradient(45deg, #ccc 25%, transparent 25%, transparent 75%, #ccc 75%, #ccc),
> +  linear-gradient(45deg, #ccc 25%, transparent 25%, transparent 75%, #ccc 75%, #ccc);

Plese align the 2 gradients (in indentation).
Same for the other gradients you added :)

Comment 26

[Tim Nguyen :ntim]

Also, you are missing a patch title : Bug 956044 - Add a checkered background to the inspector color swatches. r=bgrins

You can do that by :
hg qnew name-of-file.patch -m "Title here". You can also do it manually.

Comment 27

[alexharris6]

Thanks Tim. Can you clarify how to align the gradients? Should they not just be on one long line? Is there a specific CSS style guide I should be referencing?

Once I get that resolved, I will add a patch title and re-attach.

Comment 28

[Brian Grinstead [:bgrins]]

(In reply to alexharris6 from comment #27)
> Thanks Tim. Can you clarify how to align the gradients? Should they not just
> be on one long line? Is there a specific CSS style guide I should be
> referencing?
> 
> Once I get that resolved, I will add a patch title and re-attach.

Nice, just quickly checking with the patch applied and it looks great.

For aligning the gradients - since there are two gradients separated by a comma just line up the second one directly below the first.

background-image: linear-gradient(...),
                  linear-gradient(...);

For editing the commit message, if you are using mercurial queues you can just use hg qref -e.

Comment 29

[Brian Grinstead [:bgrins]]

Comment on attachment 8422911 [details] [diff] [review]
Patch for bug 956044

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

Can you also make a change in light-theme.css and dark-theme.css while you are working with this?  Remove the markupview-colorswatch class from those files (it isn't used anymore)

::: browser/themes/shared/devtools/ruleview.css
@@ +126,5 @@
> +  background-image: linear-gradient(45deg, #ccc 25%, transparent 25%, transparent 75%, #ccc 75%, #ccc),
> +  linear-gradient(45deg, #ccc 25%, transparent 25%, transparent 75%, #ccc 75%, #ccc);
> +  background-size: 12px 12px;
> +  background-position: 0 0, 6px 6px;
> +  display: inline-block;

As far as I know, this inline-block doesn't make a difference and it can be removed.

Comment 30

[Tim Nguyen :ntim]

Comment on attachment 8422911 [details] [diff] [review]
Patch for bug 956044

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

::: browser/themes/shared/devtools/ruleview.css
@@ +137,1 @@
>  .ruleview-overridden {

You might want to add a new line before for consistency :)

Comment 31

[alexharris6]

Attachment: Fixed patch, additional changes and title added

This new patch includes a patch title, some css formatting fixes, removes unnecessary inline-blocks, and removes unused "markupview-colorswatch" class from light and dark themes.

Comment 32

[Tim Nguyen :ntim]

Comment on attachment 8423230 [details] [diff] [review]
Fixed patch, additional changes and title added

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

Almost there :)

::: browser/themes/osx/devtools/computedview.css
@@ -167,5 @@
>    float: right;
>  }
>  
>  .computedview-colorswatch {
> -  display: inline-block;

You're removing display: inline-block; from the wrong place. Brian told you to remove it from the ::before pseudo element.

Comment 33

[Brian Grinstead [:bgrins]]

Comment on attachment 8423230 [details] [diff] [review]
Fixed patch, additional changes and title added

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

::: browser/themes/osx/devtools/computedview.css
@@ -167,5 @@
>    float: right;
>  }
>  
>  .computedview-colorswatch {
> -  display: inline-block;

I did mean on the before element.  TBH I'm not sure if this needs to be there for computedview-colorswatch or if it could be removed.  If it doesn't cause any difference in the layout, I'd keep this removed (since it makes it more consistent with ruleview-swatch, and hopefully we will be able to merge the two into a shared file at some point).

Comment 34

[Tim Nguyen :ntim]

(In reply to Brian Grinstead [:bgrins] from comment #33)
> Comment on attachment 8423230 [details] [diff] [review]
> Fixed patch, additional changes and title added
> 
> Review of attachment 8423230 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/themes/osx/devtools/computedview.css
> @@ -167,5 @@
> >    float: right;
> >  }
> >  
> >  .computedview-colorswatch {
> > -  display: inline-block;
> 
> I did mean on the before element.  TBH I'm not sure if this needs to be
> there for computedview-colorswatch or if it could be removed.  If it doesn't
> cause any difference in the layout, I'd keep this removed (since it makes it
> more consistent with ruleview-swatch, and hopefully we will be able to merge
> the two into a shared file at some point).

FYI, the default <span> display value is inline. So it's better to keep the inline-block here.

Comment 35

[alexharris6]

Attachment: Another patch, fixing inline-block issues

Ok, another attempt at this patch. Thanks for bearing with me :)

This version removes inline-block from all of the relevant ::before psuedoelements. I tried removing inline-block from .computedview-colorswatch, but it seems it is necessary for it to render correctly. Also, .ruleview-colorswatch was also being set to inline-block in styleinspector/ruleview.css, so I removed that and set it in the other instance of the class. Now .computedview-colorswatch and .ruleview-colorswatch are identical, and can be easily combined into a single class when the time comes.

Comment 36

[Brian Grinstead [:bgrins]]

Comment on attachment 8423418 [details] [diff] [review]
Another patch, fixing inline-block issues

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

Looks great!

Comment 37

[Carsten Book [:Tomcat]]

Hi,
could you provide a Try link. Suggestions for what to run if you haven't
yet can be found here:
https://wiki.mozilla.org/Sheriffing/How:To:Recommended_Try_Practices

Comment 38

[Brian Grinstead [:bgrins]]

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=5e147eed5cf8

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/1157720aabb3

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1157720aabb3