1049012 - DevTools: replace #5a6169 color with a color from the palette

Status: VERIFIED FIXED

(DevTools :: General, defect)

Description

[Albert Juhé]

Coming from bug 951714 comment 53. We said we should change the color #5a6169 with a color from the palette that can be found here: https://developer.mozilla.org/en-US/docs/Tools/DevToolsColors

Comment 1

[Albert Juhé]

Attachment: colors.png

Here there is an image comparing the current color (bottom, #5a6169) with the two suggested: Foreground Text Grey (#b6babf) and Content Text Dark Grey (#5f7387). Which one should we use?

Comment 2

[Brian Grinstead [:bgrins]]

I think 5f7387 looks better, how does the corresponding color (667380) look in the light theme?

Comment 3

[Albert Juhé]

Attachment: bug1049012.patch

The patch with the colors applied.

Comment 4

[Albert Juhé]

Attachment: bug1049012.png

And the screenshot.

Comment 5

[Brian Grinstead [:bgrins]]

Comment on attachment 8472404 [details] [diff] [review]
bug1049012.patch

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

::: browser/themes/shared/devtools/toolbars.inc.css
@@ +9,1 @@
>  %define solidSeparatorDark linear-gradient(#2d5b7d, #2d5b7d)

This color for solidSeparatorDark should also match something from the palette, preferably the corresponding color as solidSeparatorLight (so #5f7387).  Does that look OK, or should we use a different color for these (possibly the splitter colors)?

Comment 6

[Albert Juhé]

Ok. Anyway, I have to have for Bug 947242 to land to start with this one, right?

Comment 7

[Brian Grinstead [:bgrins]]

(In reply to Albert Juhé from comment #6)
> Ok. Anyway, I have to have for Bug 947242 to land to start with this one,
> right?

Yeah, let's wait for that.  In fact, once that lands we could probably get rid of the preprocessor directives altogether and store this value in a variable, or just use it directly in the necessary rules.

Comment 8

[Tim Nguyen :ntim]

(In reply to Albert Juhé from comment #4)
> Created attachment 8472406 [details]
> bug1049012.png
> 
> And the screenshot.

I don't really like how it looks, especially the light theme. I think you could use the breadcrumb buttons border color here.

Comment 9

[Albert Juhé]

Sorry for the (very) long delay. I have been trying to work on this again but I couldn't get the variables to work inside the linear-gradient function.

This works:

> border-image: linear-gradient(transparent 15%, red 15%, red 85%, transparent 85%) 1 1;

But this doesn't:

> border-image: linear-gradient(transparent 15%, var(--theme-splitter-color) 15%, var(--theme-splitter-color) 85%, transparent 85%) 1 1;

When I inspect that element with the browser toolbox it shows:

> border-image: none 1 1 1 1;

Is there a bug that makes --var() not to work inside CSS functions or am I missing something?

Comment 10

[Brian Grinstead [:bgrins]]

(In reply to Albert Juhé from comment #9)
> Sorry for the (very) long delay. I have been trying to work on this again
> but I couldn't get the variables to work inside the linear-gradient function.
> 
> This works:
> 
> > border-image: linear-gradient(transparent 15%, red 15%, red 85%, transparent 85%) 1 1;
> 
> But this doesn't:
> 
> > border-image: linear-gradient(transparent 15%, var(--theme-splitter-color) 15%, var(--theme-splitter-color) 85%, transparent 85%) 1 1;
> 
> When I inspect that element with the browser toolbox it shows:
> 
> > border-image: none 1 1 1 1;
> 
> Is there a bug that makes --var() not to work inside CSS functions or am I
> missing something?

Yeah, that looks like a bug.  Confirmed on http://jsfiddle.net/L7v9guun/1/.  I'm guessing maybe Bug 1099448, I'll ask over there.

Comment 11

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #10)
> (In reply to Albert Juhé from comment #9)
> > Is there a bug that makes --var() not to work inside CSS functions or am I
> > missing something?
> 
> Yeah, that looks like a bug.  Confirmed on http://jsfiddle.net/L7v9guun/1/. 
> I'm guessing maybe Bug 1099448, I'll ask over there.

But don't let it hold you up too much.  If that's the last thing and we can't update it at the moment, then let's proceed with what you have and then file a follow up bug that depends on the platform fix to change that last one.

Comment 12

[Brian Grinstead [:bgrins]]

OK, Bug 1179078 is fixed so the border-image issue shouldn't be a problem anymore

Comment 13

[Albert Juhé]

Attachment: bug1049012a.patch

This patch replaces the hard-coded colours with one from the palette. I couldn't find any that was similar to the original ones in dark and light themes at the same time. In the end I chose --theme-splitter-color because I think it's the one that looks better, but let me know if you think another one is better or if I should create a new variable.

Comment 14

[Albert Juhé]

Attachment: devtools-infobar-tabs-border-color.png

The biggest change is in the dark theme, instead of having a light separator now it's dark (in the screenshot, top=new, bottom=old).

As I said in the previous message. Please, let me know if you think I should change that.

Comment 15

[Brian Grinstead [:bgrins]]

Comment on attachment 8632352 [details] [diff] [review]
bug1049012a.patch

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

Looks like there was a lot of duplication in the original code, this looks a lot simpler and gets rid of preprocessing, which is great!  Also the splitter color is fine - these are sort of splitters anyway

Comment 16

[Brian Grinstead [:bgrins]]

Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=95d02ca1d08a

Comment 17

[Brian Grinstead [:bgrins]]

remote:   https://hg.mozilla.org/integration/fx-team/rev/40d9e0664941

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/40d9e0664941

Comment 19

[Saheda Reza [:Antora]]

I have reproduced this bug on Firefox Version 34.0a1

It's fixed and verified on latest Firefox Beta
Build ID 	20151005144425
User Agent 	Mozilla/5.0 (Windows NT 6.3; rv:42.0) Gecko/20100101 Firefox/42.0

Tested OS- Windows8.1 32bit