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

VERIFIED FIXED in Firefox 42

Status

defect
VERIFIED FIXED
5 years ago
Last year

People

(Reporter: aljullu, Assigned: aljullu, Mentored)

Tracking

(Blocks 1 bug)

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

Firefox Tracking Flags

(firefox42 verified)

Details

Attachments

(2 attachments, 3 obsolete attachments)

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
Assignee: nobody → aljullu
Blocks: 916766
Version: unspecified → Trunk
Posted image colors.png (obsolete) —
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?
Attachment #8470466 - Flags: feedback?
Attachment #8470466 - Flags: feedback? → feedback?(bgrinstead)
I think 5f7387 looks better, how does the corresponding color (667380) look in the light theme?
Attachment #8470466 - Flags: feedback?(bgrinstead) → feedback+
Posted patch bug1049012.patch (obsolete) — Splinter Review
The patch with the colors applied.
Attachment #8472404 - Flags: review?(bgrinstead)
Posted image bug1049012.png (obsolete) —
And the screenshot.
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)?
Attachment #8472404 - Flags: review?(bgrinstead)
Ok. Anyway, I have to have for Bug 947242 to land to start with this one, right?
(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.
Depends on: 947242
(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.
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?
Flags: needinfo?(bgrinstead)
(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.
Flags: needinfo?(bgrinstead)
(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.
OK, Bug 1179078 is fixed so the border-image issue shouldn't be a problem anymore
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.
Attachment #8472404 - Attachment is obsolete: true
Attachment #8472406 - Attachment is obsolete: true
Attachment #8632352 - Flags: review?(bgrinstead)
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.
Attachment #8470466 - Attachment is obsolete: true
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
Attachment #8632352 - Flags: review?(bgrinstead) → review+
https://hg.mozilla.org/mozilla-central/rev/40d9e0664941
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 42
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
QA Whiteboard: [bugday-20151008]
Status: RESOLVED → VERIFIED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.