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

RESOLVED FIXED in Firefox 32

Status

()

Firefox
Developer Tools: Inspector
P3
enhancement
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: ntim, Assigned: alexharris6)

Tracking

Trunk
Firefox 32
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [bugday-20140108][mentor=bgrins][good first bug][lang=css][lang=js])

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

4 years ago
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.

Updated

4 years ago
Severity: normal → enhancement
Component: Untriaged → Developer Tools: Inspector
Whiteboard: [testday-20140108]

Updated

4 years ago
Whiteboard: [testday-20140108] → [bugday-20140108]
(Reporter)

Updated

4 years ago
Flags: needinfo?(pbrosset)
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.
Flags: needinfo?(pbrosset)
Created attachment 8360503 [details]
Screen Shot 2014-01-15 at 6.48.55 PM.png
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.
(Reporter)

Comment 4

4 years ago
Created attachment 8360525 [details]
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.
Attachment #8360525 - Flags: ui-review?(pbrosset)
(Reporter)

Comment 5

4 years ago
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.
(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 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).
Attachment #8360525 - Flags: ui-review?(pbrosset) → ui-review+
Flags: needinfo?(dhenein)
(Reporter)

Comment 8

4 years ago
(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.
so maybe to avoid inserting a new element, we can use a pseudo-element :before or :after
(Reporter)

Comment 10

4 years ago
(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.
> 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.
Flags: needinfo?(dhenein)
(Reporter)

Updated

4 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
Duplicate of this bug: 1003424
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?
OS: Windows 8.1 → All
Priority: -- → P3
Hardware: x86_64 → All
Whiteboard: [bugday-20140108] → [bugday-20140108][mentor=bgrins][good first bug][lang=css][lang=js]
Version: 29 Branch → Trunk
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.
(Reporter)

Comment 17

4 years ago
(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 :)
(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
(Assignee)

Comment 19

4 years ago
Hello, may I work on this bug? It will be my first.
(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.
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
Assignee: nobody → alexharris6
Status: NEW → ASSIGNED
(Assignee)

Comment 22

4 years ago
Created attachment 8422911 [details] [diff] [review]
Patch for bug 956044

First attempt submitting a patch, please let me know if I am doing it incorrectly or skipping any steps.
Attachment #8422911 - Flags: review+
Attachment #8422911 - Flags: feedback+
(Assignee)

Comment 23

4 years ago
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.
(Reporter)

Comment 24

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

Please request review first.
Attachment #8422911 - Flags: review?(bgrinstead)
Attachment #8422911 - Flags: review+
Attachment #8422911 - Flags: feedback+
(Reporter)

Comment 25

4 years ago
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 :)
(Reporter)

Comment 26

4 years ago
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.
(Assignee)

Comment 27

4 years ago
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.
(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.
Blocks: 1010959
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.
Attachment #8422911 - Flags: review?(bgrinstead)
(Reporter)

Comment 30

4 years ago
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 :)
(Assignee)

Comment 31

4 years ago
Created attachment 8423230 [details] [diff] [review]
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.
Attachment #8422911 - Attachment is obsolete: true
Attachment #8423230 - Flags: review?(bgrinstead)
Attachment #8360525 - Attachment is obsolete: true
(Reporter)

Comment 32

4 years ago
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 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).
Attachment #8423230 - Flags: review?(bgrinstead)
(Reporter)

Comment 34

4 years ago
(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.
(Assignee)

Comment 35

4 years ago
Created attachment 8423418 [details] [diff] [review]
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.
Attachment #8423230 - Attachment is obsolete: true
Attachment #8423418 - Flags: review?(bgrinstead)
Comment on attachment 8423418 [details] [diff] [review]
Another patch, fixing inline-block issues

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

Looks great!
Attachment #8423418 - Flags: review?(bgrinstead) → review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
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
Keywords: checkin-needed
(Reporter)

Updated

4 years ago
Flags: needinfo?(bgrinstead)
Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=5e147eed5cf8
Flags: needinfo?(bgrinstead)
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/1157720aabb3
Keywords: checkin-needed
Whiteboard: [bugday-20140108][mentor=bgrins][good first bug][lang=css][lang=js] → [bugday-20140108][mentor=bgrins][good first bug][lang=css][lang=js][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/1157720aabb3
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [bugday-20140108][mentor=bgrins][good first bug][lang=css][lang=js][fixed-in-fx-team] → [bugday-20140108][mentor=bgrins][good first bug][lang=css][lang=js]
Target Milestone: --- → Firefox 32
(Reporter)

Updated

4 years ago
Depends on: 1011847
(Reporter)

Updated

4 years ago
Depends on: 1012811
You need to log in before you can comment on or make changes to this bug.