947242 - DevTools themes - switch to new theme colors

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Brian Grinstead [:bgrins]]

The new theme colors are listed here: https://developer.mozilla.org/en-US/docs/Tools/DevToolsColors.  We have been updating to use them in piecemeal as we touch different aspects of the theme, but for the highlight colors we should be able to move over all at once.

Comment 1

[Brian Grinstead [:bgrins]]

From Bug 941799 Comment 15, a comment about colors in the variable view and text editor.  We should make sure that string and booleans are sufficiently different with the new colors, or move them to use different highlights.

> booleans and strings looks very similar, at least with the light theme (orange-y red vs. orange). They
> *do* match what's in the editor, so I'm happy with that, but it might make sense to file a followup 
> about distinguishing them more.

Comment 2

[Brian Grinstead [:bgrins]]

Attachment: theme-css-variables-part1.patch

First patch: extract existing colors into CSS variables

Comment 3

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

(In reply to Brian Grinstead [:bgrins] from comment #2)
> Created attachment 8346652 [details] [diff] [review]
> theme-css-variables-part1.patch
> 
> First patch: extract existing colors into CSS variables

<3

Comment 4

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

Does this mean we can move those rules back to their own CSS files and user variables everywhere?

Comment 5

[Girish Sharma [:Optimizer]]

(In reply to Brian Grinstead [:bgrins] from comment #2)
> Created attachment 8346652 [details] [diff] [review]
> theme-css-variables-part1.patch
> 
> First patch: extract existing colors into CSS variables

\o/ _o_ \o/

Comment 6

[Brian Grinstead [:bgrins]]

(In reply to Victor Porof [:vp] from comment #4)
> Does this mean we can move those rules back to their own CSS files and user
> variables everywhere?

Yes, exactly.  For example, from widgets.css we can set color: var(theme-color-1).

Comment 7

[Matthew N. [:MattN]]

Comment on attachment 8346652 [details] [diff] [review]
theme-css-variables-part1.patch

(In reply to Victor Porof [:vp] from comment #4)
> Does this mean we can move those rules back to their own CSS files and user
> variables everywhere?

You should check with :heycam to make sure that CSS variables are likely ride the trains before relying on them. Some months ago I was advised by dbaron not to use them for a while after landing on m-c in case they get preffed off. You can see[1] that the feature is currently only enabled in non-release builds at the moment so this patch would break in Beta and Release.

[1] https://mxr.mozilla.org/mozilla-central/source/modules/libpref/src/init/all.js?rev=6ceff347849d#1987

Comment 8

[Brian Grinstead [:bgrins]]

(In reply to Matthew N. [:MattN] from comment #7)
> Comment on attachment 8346652 [details] [diff] [review]
> theme-css-variables-part1.patch
> 
> (In reply to Victor Porof [:vp] from comment #4)
> > Does this mean we can move those rules back to their own CSS files and user
> > variables everywhere?
> 
> You should check with :heycam to make sure that CSS variables are likely
> ride the trains before relying on them. Some months ago I was advised by
> dbaron not to use them for a while after landing on m-c in case they get
> preffed off. You can see[1] that the feature is currently only enabled in
> non-release builds at the moment so this patch would break in Beta and
> Release.
> 
> [1]
> https://mxr.mozilla.org/mozilla-central/source/modules/libpref/src/init/all.
> js?rev=6ceff347849d#1987

:heycam, what is the best place to watch for updates on when we can safely use the CSS variables feature in release builds?

Comment 9

[Cameron McCormack (:heycam)]

I've just filed bug 957833 for unpreffing the feature.  I'll also send out an "intent to ship" mail to dev-platform when we think it's ready: https://wiki.mozilla.org/WebAPI/ExposureGuidelines#Intent_to_Ship

Comment 10

[sys.sgx]

Updates wanted on this.

Comment 11

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

Still blocked on bug 957833.

Comment 12

[Brian Grinstead [:bgrins]]

Attachment: theme-variables-p1.patch

rebased

Comment 13

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

Do we still need the .theme-fg-colorx classes now?

Comment 14

[Brian Grinstead [:bgrins]]

(In reply to Victor Porof [:vporof][:vp] from comment #13)
> Do we still need the .theme-fg-colorx classes now?

It is still the easiest way to get the colors applied from markup / js for things you don't want to make a class for, like http://dxr.mozilla.org/mozilla-central/source/browser/devtools/styleinspector/computed-view.js#972 or http://dxr.mozilla.org/mozilla-central/source/browser/devtools/markupview/markup-view.xhtml#33.

It's not used a ton (http://dxr.mozilla.org/mozilla-central/search?q=theme-fg&redirect=true), but I think it can still be helpful

Comment 15

[Brian Grinstead [:bgrins]]

Attachment: theme-colors-other.patch

What do you think about removing comments next to lines that have been switched over to the theme colors?  The comments make it easy for reading and seeing what the color will generally be,  but the var name should hopefully be descriptive enough

Comment 16

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

That's why the comments were there in the first place afaik, so that it'll make our lives easier when switching to variables. They can now be removed.

Comment 17

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

Comment on attachment 8426233 [details] [diff] [review]
theme-variables-p1.patch

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

::: browser/themes/shared/devtools/dark-theme.css
@@ +19,5 @@
> +  --theme-color-3: #a673bf; /* pink / lavender */
> +  --theme-color-4: #6270b2; /* purple / violet */
> +  --theme-color-5: #a18650; /* yellow */
> +  --theme-color-6: #b26b47; /* orange */
> +  --theme-color-7: #bf5656; /* red */

I think the name of these variables should match the ones at https://developer.mozilla.org/en-US/docs/Tools/DevToolsColors

Comment 18

[Brian Grinstead [:bgrins]]

Attachment: colors-part1.patch

Note that this patch changes fg-color3 from pink to bluegrey in the dark theme.  This affects the markupview and ruleview quite a bit, but it looks OK to me.  We could always leave this as theme-highlight-pink for now and fix that inconsistency later

Here is my plan:

Part 1 (this patch): Switches over to new variable names just in light and dark theme, establishing variable names and using the current (outdated colors).
Part 2: Switch over to these variables names in the rest of the CSS where it makes sense
Part 3: Switch the variables to the new colors - check UI and see if we need to make any adjustments.

Comment 19

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

Comment on attachment 8426275 [details] [diff] [review]
colors-part1.patch

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

Let me know what you think about my comments below.

::: browser/themes/shared/devtools/light-theme.css
@@ +5,5 @@
>  
>  /* According to:
>   * https://bugzilla.mozilla.org/show_bug.cgi?id=715472#c17
>   */
> +

Nit: either have or don't have a newline here. This differs across the two theme files.

@@ +7,5 @@
>   * https://bugzilla.mozilla.org/show_bug.cgi?id=715472#c17
>   */
> +
> +:root {
> +  --theme-body-color: #18191a;

I'd really, really like the variable names to be the same as on the wiki.
So this should be --theme-foreground-text, or the wiki updated.

@@ +8,5 @@
>   */
> +
> +:root {
> +  --theme-body-color: #18191a;
> +  --theme-body-background: #fcfcfc;

--theme-content-background, or the wiki updated.

@@ +9,5 @@
> +
> +:root {
> +  --theme-body-color: #18191a;
> +  --theme-body-background: #fcfcfc;
> +  --theme-contrast-background: #a18650;

This isn't on the wiki.

@@ +11,5 @@
> +  --theme-body-color: #18191a;
> +  --theme-body-background: #fcfcfc;
> +  --theme-contrast-background: #a18650;
> +
> +  --theme-link: hsl(208,56%,40%); /* blue */

Neither this.

@@ +12,5 @@
> +  --theme-body-background: #fcfcfc;
> +  --theme-contrast-background: #a18650;
> +
> +  --theme-link: hsl(208,56%,40%); /* blue */
> +  --theme-link-visited: hsl(208,56%,40%); /* blue */

Nor this. Why don't we just stick to the existing palette?

@@ +13,5 @@
> +  --theme-contrast-background: #a18650;
> +
> +  --theme-link: hsl(208,56%,40%); /* blue */
> +  --theme-link-visited: hsl(208,56%,40%); /* blue */
> +  --theme-comment: hsl(90,2%,46%); /* grey */

Ditto.

@@ +15,5 @@
> +  --theme-link: hsl(208,56%,40%); /* blue */
> +  --theme-link-visited: hsl(208,56%,40%); /* blue */
> +  --theme-comment: hsl(90,2%,46%); /* grey */
> +  --theme-selection-background: #4c9ed9;
> +  --theme-selection-color: #f5f7fa;

I like these two names better than what's on the wiki. We should probably update the wiki in this case.

@@ +45,4 @@
>  }
>  
>  .theme-bg-darker {
>    background: #EFEFEF;

Is this still necessary? Why isn't this a variable?

@@ +55,4 @@
>  }
>  
>  .theme-bg-contrast,
>  .variable-or-property:not([overridden])[changed] { /* contrast bg color to attract attention on a container */

We need a bug filed to move all these rules back into their original files.

@@ +60,5 @@
>  }
>  
>  .theme-link,
>  .cm-s-mozilla .cm-link,
>  .CodeMirror-Tern-type { /* blue */

...and I believe it's best we put these (.cm-foo and .CodeMirror-Foo) in a separate css file for codemirror. Weren't we using mozilla.css or something?

@@ +385,5 @@
>  }
>  
>  .devtools-side-splitter {
> +  -moz-border-end: 1px solid var(--theme-splitter-color);
> +  border-color: var(--theme-splitter-color); /* Needed for responsive container at low width. */

Probably not in the scope of this bug, but this border-color makes no sense to me. Why isn't -moz-border-end sufficient?

Comment 20

[Brian Grinstead [:bgrins]]

(In reply to Victor Porof [:vporof][:vp] from comment #19)
> Comment on attachment 8426275 [details] [diff] [review]
> colors-part1.patch
> 
> Review of attachment 8426275 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Let me know what you think about my comments below.
> 


Regarding the comments about names not being in the wiki - the wiki is not really in sync with our themes right now.  The colors we are using don't match the ones in the wiki yet, and the distinction between Editor/Content and Chrome is pretty weak.  In general, I'd prefer to update the wiki based on the names and conventions that we are already using, but I'm fine with taking whatever makes sense for each case.

> @@ +60,5 @@
> >  }
> >  
> >  .theme-link,
> >  .cm-s-mozilla .cm-link,
> >  .CodeMirror-Tern-type { /* blue */
> 
> ...and I believe it's best we put these (.cm-foo and .CodeMirror-Foo) in a
> separate css file for codemirror. Weren't we using mozilla.css or something?

I'm not necessarily opposed to this, though editor theming is one area where direct color mapping doesn't really make as much sense.  For a couple of reasons: 1) we purposely want certain tokens to match with certain foreground colors for fitting in with the rest of the UI.  An example of this is when you do "Edit as HTML" in the inspector.  In this case, we want the markup view colors to match exactly with the editor colors.  By moving this into another file, it would be easier to accidentally bump these out of sync.  2) An editor theme could decide that certain colors aren't the right ones for certain token types.  Luckily the light and dark themes have become very consistent over time.  Regardless, I'd like to not do that in this bug.

Comment 21

[Brian Grinstead [:bgrins]]

Ugh, also colors like "Foreground (Text) - Grey: #b6babf" for the grey theme are in the "Chrome" column but are widely used in content.  I need to update the wiki to remove text colors from Chrome and Content and put into their own section instead

Comment 22

[Brian Grinstead [:bgrins]]

Comment on attachment 8426275 [details] [diff] [review]
colors-part1.patch

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

::: browser/themes/shared/devtools/light-theme.css
@@ +45,4 @@
>  }
>  
>  .theme-bg-darker {
>    background: #EFEFEF;

It's used only in a couple of places in the inspector right now.  I'd prefer to wait to refactor this until after the first wave of changes.  We can make it a variable - but right now this isn't used anywhere except for the theme files.

@@ +55,4 @@
>  }
>  
>  .theme-bg-contrast,
>  .variable-or-property:not([overridden])[changed] { /* contrast bg color to attract attention on a container */

Bug 1014205

@@ +385,5 @@
>  }
>  
>  .devtools-side-splitter {
> +  -moz-border-end: 1px solid var(--theme-splitter-color);
> +  border-color: var(--theme-splitter-color); /* Needed for responsive container at low width. */

IIRC because it switches to a top border when in the devtools-responsive-container

Comment 23

[Brian Grinstead [:bgrins]]

One problem right now is that the dark theme uses  "Content (Text) - Grey: #8fa1b2" for the body text, while the light theme uses "Foreground (Text) - Dark: #18191a".

Also, the alternate content text colors are titled light/grey/dark grey for dark theme and dark/grey/dark grey for the light theme - in this case 'light' and 'dark' are the equal colors. So these need to be renamed to something like:

content-text-color1, content-text-color2, content-text-color3
or
content-text-highcontrast, content-text-lowcontrast, content-text-darkgrey

Comment 24

[Brian Grinstead [:bgrins]]

The current text color usage is so confusing.  I'm seriously wondering if we could consolidate all of the text colors (chrome/content) except for selection into a list from highest contrast to lowest contrast:

--theme-textcolor1, --theme-textcolor2, --theme-textcolor3, --theme-textcolor4, --theme-textcolor5

Comment 25

[Brian Grinstead [:bgrins]]

OK, I've organized the wiki some more.  In addition to this, I've removed "Content (Text) - Dark: #292e33" from the light theme, to bring the number of active colors in line with the dark theme.  It is only use in one place: in debugger.inc.css - .theme-light .trace-item - I will switch this to a different color in patch part 2

Comment 26

[Tim Nguyen :ntim]

What color are we gonna use for bug 1015627 ?

Comment 27

[Brian Grinstead [:bgrins]]

Attachment: new-theme-colors-1.patch

OK, I've done my best to figure out how we are using various text colors and reorganize the variable appropriately.  I've also switched to all the new colors from the wiki  as opposed to doing it in a part 3 as I said in comment 18.  This will actually change the look of the tools to match the new theme colors.

The proposal for the variable naming in this patch with regards to text colors is like so (wiki name on left, variable name in patch on right):

Body Text                         -> --theme-body-color
Foreground (Text)                 -> --theme-body-color-alt
Content (Text) - High Contrast    -> --theme-content-color1
Content (Text) - Grey             -> --theme-content-color2
Content (Text) - Dark Grey        -> --theme-content-color3

Where content-color1,2,3 is a color in descending order of contrast (so content-color3 is the closest to the theme-body-background, while content-color1 is the highest contrast).

Comment 28

[Brian Grinstead [:bgrins]]

Attachment: new-theme-colors-2.patch

The big switch of hardcoded values to the variables

Comment 29

[Brian Grinstead [:bgrins]]

Comment on attachment 8468847 [details] [diff] [review]
new-theme-colors-2.patch

Apologies in advance

Comment 30

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

Comment on attachment 8468846 [details] [diff] [review]
new-theme-colors-1.patch

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

I'm a bit concerned about the fact that we still have a lot custom colors that aren't from the palette. I suggest either dealing with that in a followup, or fixing it here. Now that we have css variables, there's absolutely no reason to still have stray custom colors in our css files (that are not just transparent grays, used to "lighten" or "darken" something up).

Let me know what you think.

::: browser/themes/shared/devtools/dark-theme.css
@@ +86,5 @@
>  .cm-s-mozilla .cm-hr,
>  .cm-s-mozilla .cm-comment,
>  .variable-or-property .token-undefined,
> +.variable-or-property .token-null,
> +.CodeMirror-Tern-completion-unknown:before { /* grey */

Don't think we still need the "grey" comment.

@@ +92,4 @@
>  }
>  
>  .theme-gutter {
>    background-color: #0f171f;

No color variables for the gutter?

@@ +97,5 @@
>    border-color: #303b47;
>  }
>  
>  .theme-separator { /* grey */
>    border-color: #303b47;

We should have a variable for this. Maybe one that matches the splitters? Or not..

@@ +110,4 @@
>  }
>  
>  .CodeMirror-Tern-completion-number:before {
>    background-color: #5c9966;

Lots of custom colors. Either update the wiki, or only use the colors from the palette.

@@ +124,4 @@
>  }
>  
>  .CodeMirror-Tern-completion-object:before {
>    background-color: #3689b2;

Ditto for all colors used in this file (and all files, but I would be happy with a followup if that seems like a lot of busywork and outside the scope of this bug).

@@ +154,5 @@
>  }
>  
>  .theme-fg-color5,
> +.cm-s-mozilla .cm-keyword {
> +  color: var(--theme-highlight-lightorange);

No more .cm-bracket here?

@@ +198,1 @@
>    border-color: hsla(210,8%,5%,.6);

This looks like a very specific color too.

@@ +230,5 @@
>    border-left: solid 1px #fff;
>  }
>  
>  .cm-s-mozilla.CodeMirror-focused .CodeMirror-selected { /* selected text (focused) */
>    background: rgb(185, 215, 253);

Why can't we use a selection color here?

@@ +269,5 @@
>  }
>  
>  /* Highlight for evaluating current statement. */
>  div.CodeMirror span.eval-text {
>    background-color: #556;

This color is not from the palette.

@@ +405,5 @@
>  
>  .CodeMirror-hints,
>  .CodeMirror-Tern-tooltip {
>    box-shadow: 0 0 4px rgba(255, 255, 255, .3);
>    background-color: #0f171f;

Ditto.

::: browser/themes/shared/devtools/light-theme.css
@@ +9,5 @@
>   */
> +:root {
> +  --theme-body-background: #fcfcfc;
> +  --theme-sidebar-background: #f7f7f7;
> +  --theme-contrast-background: #E6B064;

Try to keep these hex values' formatting consistent. Either all uppercase, or all lowercase.

@@ +96,5 @@
>    border-color: hsl(0,0%,65%);
>  }
>  
>  .theme-separator { /* grey */
>    border-color: #cddae5;

Same comments in this file as for dark-theme.css

Comment 31

[Brian Grinstead [:bgrins]]

(In reply to Victor Porof [:vporof][:vp] from comment #30)
> Comment on attachment 8468846 [details] [diff] [review]
> new-theme-colors-1.patch
> 
> Review of attachment 8468846 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm a bit concerned about the fact that we still have a lot custom colors
> that aren't from the palette. I suggest either dealing with that in a
> followup, or fixing it here. Now that we have css variables, there's
> absolutely no reason to still have stray custom colors in our css files
> (that are not just transparent grays, used to "lighten" or "darken"
> something up). 
> @@ +110,4 @@
> >  }
> >  
> >  .CodeMirror-Tern-completion-number:before {
> >    background-color: #5c9966;
> 
> Lots of custom colors. Either update the wiki, or only use the colors from
> the palette.
> 
> @@ +124,4 @@
> >  }
> >  
> >  .CodeMirror-Tern-completion-object:before {
> >    background-color: #3689b2;
> 
> Ditto for all colors used in this file (and all files, but I would be happy
> with a followup if that seems like a lot of busywork and outside the scope
> of this bug).

Yes, I'd be happy to do it, but would like to tackle anything that didn't already match (or closely match) an existing color as a follow up.  I'm sure most of these will map directly into an existing one, but it is a process of finding where each style is used in both themes and trying out different colors (which is quite time consuming).  If we do a follow up I think it will be easier to go through the changes before/after.

> @@ +154,5 @@
> >  }
> >  
> >  .theme-fg-color5,
> > +.cm-s-mozilla .cm-keyword {
> > +  color: var(--theme-highlight-lightorange);
> 
> No more .cm-bracket here?

No, this caused brackets in HTML text in the debugger to stand out very oddly, much stronger than any other color.

Comment 32

[Brian Grinstead [:bgrins]]

Attachment: new-theme-colors-1.patch

As discussed, there are also a couple of theme colors I'd actually like to tweak, but I think it would be best to follow up once we land this to best keep the wiki in sync with reality.

Comment 33

[Brian Grinstead [:bgrins]]

Attachment: new-theme-colors-2.patch

Same, but includes matching bracket colors for markup view as what is seen in HTML highlighting in CM in the debugger

Comment 34

[Brian Grinstead [:bgrins]]

Attachment: new-theme-colors-2.patch

Rebased, and fixed some test failures in the Computed View in which the variables were showing up in the list of properties with browser styles checked (like in https://tbpl.mozilla.org/?tree=Try&rev=10da8ae170d5).  New try push: https://tbpl.mozilla.org/?tree=Try&rev=57b3aeecdf33

Comment 35

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

Comment on attachment 8470881 [details] [diff] [review]
new-theme-colors-2.patch

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

I mean, most of the changes here are mechanical so there's really not that much to review :0

Comment 36

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

General comments for both patches: some of the new colors look a bit weird to me, and it took me a while to adjust my eyes to them. Let's please get so UI review to make sure things still look good.

Can you also please go through all the devtools css files, and make a list of where we still have custom colors, either in rgba or hsl, or hex format. Grays and transparent blacks/whites don't matter. File followup bugs for all of them and maybe make them depend on a [meta]?

Comment 37

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

Oh, and another thing! Now that we have color variables, we should move certain rules (like codemirror, variables view etc.) back to their respective css files, and keep theme-dark.css and theme-light.css as small and light as possible!

Comment 38

[Brian Grinstead [:bgrins]]

> Oh, and another thing! Now that we have color variables, we should move
> certain rules (like codemirror, variables view etc.) back to their
> respective css files, and keep theme-dark.css and theme-light.css as small
> and light as possible!

Have opened Bug 1014205 for that
 
> Can you also please go through all the devtools css files, and make a list
> of where we still have custom colors, either in rgba or hsl, or hex format.
> Grays and transparent blacks/whites don't matter. File followup bugs for all
> of them and maybe make them depend on a [meta]?

Can do - filed 1055311

Comment 39

[Brian Grinstead [:bgrins]]

> General comments for both patches: some of the new colors look a bit weird
> to me, and it took me a while to adjust my eyes to them. Let's please get so
> UI review to make sure things still look good.

Darrin, will you have time to do a quick ui review of this build?  I've switched to the new palette colors at https://developer.mozilla.org/en-US/docs/Tools/DevToolsColors.  Would be great if you could check out the light/dark themes and make sure everything looks good.  Especially the dark theme, which has switched from old colors to new.

The binary is available here: https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-57b3aeecdf33/try-macosx64-debug/

Comment 40

[Brian Grinstead [:bgrins]]

Attachment: new-theme-colors-2.patch

Rebased

Comment 41

[Tim Nguyen :ntim]

The patches cause a few regressions :
- bug 797273 also affects the dark theme now
- The "This page has no sources", "No stack frames to display" and "No event listeners to display" labels (in debugger) are now black on dark theme
- This puts us even further than Stephen's mockups : http://people.mozilla.org/~shorlander/mockups/devTools/ux-refresh-2013/

Comment 42

[Brian Grinstead [:bgrins]]

Attachment: light-bluegrey.png

A screenshot of an idea for changing the blue-grey color for the light theme to #0072AB (which would also requite updating https://developer.mozilla.org/en-US/docs/Tools/DevToolsColors)

Comment 43

[Brian Grinstead [:bgrins]]

(In reply to Tim Nguyen [:ntim] from comment #41)
> The patches cause a few regressions :
> - bug 797273 also affects the dark theme now
> - The "This page has no sources", "No stack frames to display" and "No event
> listeners to display" labels (in debugger) are now black on dark theme
> - This puts us even further than Stephen's mockups :
> http://people.mozilla.org/~shorlander/mockups/devTools/ux-refresh-2013/

I'll try to update to match the colors used for the dark theme in markup view from the mockups.  It is getting tricky to make all of these changes without changing other parts of the tools

Comment 44

[Tim Nguyen :ntim]

(In reply to Brian Grinstead [:bgrins] from comment #43)
> (In reply to Tim Nguyen [:ntim] from comment #41)
> > The patches cause a few regressions :
> > - bug 797273 also affects the dark theme now
> > - The "This page has no sources", "No stack frames to display" and "No event
> > listeners to display" labels (in debugger) are now black on dark theme
> > - This puts us even further than Stephen's mockups :
> > http://people.mozilla.org/~shorlander/mockups/devTools/ux-refresh-2013/
> 
> I'll try to update to match the colors used for the dark theme in markup
> view from the mockups.  It is getting tricky to make all of these changes
> without changing other parts of the tools
Does using colors from : http://people.mozilla.org/~shorlander/mockups/devTools/ux-refresh-2013/[e-mail] (or the corresponding ones in the wiki) break other tools ?

Comment 45

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

My 2c as a reviewer: I honestly don't care which exact colors we use as long as they're the same across all tools and the wiki is up to date. That's the only thing I'm looking for in code.

From a design POV, the colors in comment 44 look fantastic with a beautiful contrast.

If the CSS of some tools needs to be changed, then we should definitely do that! Let's go ahead and make things consistent, I'm assuming this was the intention for this bug in the first place -- using color variables *everywhere*.

Comment 46

[Tim Nguyen :ntim]

(In reply to Victor Porof [:vporof][:vp] from comment #45)
> My 2c as a reviewer: I honestly don't care which exact colors we use as long
> as they're the same across all tools and the wiki is up to date. That's the
> only thing I'm looking for in code.
> 
> From a design POV, the colors in comment 44 look fantastic with a beautiful
> contrast.
> 
> If the CSS of some tools needs to be changed, then we should definitely do
> that! Let's go ahead and make things consistent, I'm assuming this was the
> intention for this bug in the first place -- using color variables
> *everywhere*.

In the first patch, we have the needed color variables to match the mockup (--theme-highlight-purple, --theme-highlight-lightorange and --theme-highlight-blue)

Comment 47

[Brian Grinstead [:bgrins]]

Going to try and switch the colors to match the comps and re-request ux review

Comment 48

[Brian Grinstead [:bgrins]]

Attachment: new-theme-colors-2.patch

Rebased

Comment 49

[Brian Grinstead [:bgrins]]

Attachment: new-theme-colors.patch

WIP - rebased on top of the new variables from Bug 1102369

Comment 50

[Jordan Santell [:jsantell] [@jsantell]]

What's the status of this? Working on performance tools to use theme colors and noticed it didn't quite look right, but adding the latest patch fits much better I think?

Comment 51

[Brian Grinstead [:bgrins]]

Stephen, here is a gallery of screenshots of the tools with the current patch applied:

https://www.dropbox.com/sh/1lwabf6x6dfpyyv/AABWp_unLXaYGLqQRrVfXj0Aa?dl=0

And here is what it looks like when I try to make the light theme match http://people.mozilla.org/~shorlander/mockups/devTools/ux-refresh-2013/[e-mail].  IMO the other tools end up looking funny with this change, but there aren't mockups of the other tools to try and match.

https://www.dropbox.com/sh/jd9wpaii00julyb/AAAflrEJdfTLkASSYRJGMFj9a?dl=0

Do you think we should make adjustments before landing the new colors?  We should definitely try to proceed somehow with this, so that we are actually using the colors specified here: https://developer.mozilla.org/en-US/docs/Tools/DevToolsColors.

Here is a try build that will have binaries with the patch applied if you want to check it out: https://treeherder.mozilla.org/#/jobs?repo=try&revision=787f5e2fb3ad

Comment 52

[Jordan Santell [:jsantell] [@jsantell]]

Also, maybe we can have a non-"highlight" version of these colors -- we don't necessarily want these really bright colors in our code editor, for example. Maybe we can add something like a desaturate function in css-color -- I've been using `setAlpha` there for awhile now for canvas renderings. This wouldn't solve the issue in CSS (we don't have any preprocessors there, do we?), but maybe something to explore

Comment 53

[Brian Grinstead [:bgrins]]

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #52)
> Also, maybe we can have a non-"highlight" version of these colors -- we
> don't necessarily want these really bright colors in our code editor, for
> example.

I think that's why I like the 'current patch' version from Comment 51 more.  Even though the colors in the inspector are less bright, it makes the code highlighting colors much better.

I should note that it's important that the code highlight colors for HTML elements match up with the colors used in the inspector, such that when you right click -> Edit as HTML you don't end up with totally different styles in the editor compared to the markup view.

Comment 54

[Brian Grinstead [:bgrins]]

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #52)
> Maybe we can add something like a desaturate function in css-color
> -- I've been using `setAlpha` there for awhile now for canvas renderings.
> This wouldn't solve the issue in CSS (we don't have any preprocessors there,
> do we?), but maybe something to explore

There is a preprocessor, but it doesn't have any specific color modification functionality AFAIK.  Even if we were able to make something work with that, it would only work for light-theme.css/dark-theme.css/toolbarbuttons.inc.css.  What would really help with this is the color modification functionality in CSS 4: http://dev.w3.org/csswg/css-color-4/#rgba-adjusters.

Comment 55

[Cameron McCormack (:heycam)]

Implementing the color() functions is bug 1128204.

Comment 56

[Jordan Santell [:jsantell] [@jsantell]]

Using the current colors in the performance tool results in some pretty bland visuals: http://i.imgur.com/iBlOjNq.gif

Using the patch from attachment 8528463 [details] [diff] [review], we get some better looking visuals (still tweaking what colors to use, but this saturation definitely works better) http://i.imgur.com/jhRzKoo.gif

The use cases for text and graphs are pretty different, the new patch makes the inspector pretty bright..

Comment 57

[Jordan Santell [:jsantell] [@jsantell]]

As a temporary solution, we can reference these values with a new name for the time being. `--theme-new-hightlight-green` or something, just so the performance graphs can use them

Comment 58

[Stephen Horlander [:shorlander]]

(In reply to Brian Grinstead [:bgrins] from comment #51)
> And here is what it looks like when I try to make the light theme match
> http://people.mozilla.org/~shorlander/mockups/devTools/ux-refresh-2013/
> [e-mail].  IMO the other tools end up looking
> funny with this change, but there aren't mockups of the other tools to try
> and match.

Can you please clarify "funny"?

They mostly look good to me. Rationale for brightening the colors is to a) increase contrast overall and b) make colors more visually distinct from each other.

Some of them do seem a little bright in that try-build, we could tweak them some to smooth them out.

It is probably also true that these colors won't work in all contexts. Which means we could either spec out a corresponding bright and dark variant of the baseline palette, or do as suggested and dial them up/down programmatically.

Comment 59

[Brian Grinstead [:bgrins]]

(In reply to Stephen Horlander [:shorlander] from comment #58)
> (In reply to Brian Grinstead [:bgrins] from comment #51)
> > And here is what it looks like when I try to make the light theme match
> > http://people.mozilla.org/~shorlander/mockups/devTools/ux-refresh-2013/
> > [e-mail].  IMO the other tools end up looking
> > funny with this change, but there aren't mockups of the other tools to try
> > and match.
> 
> Can you please clarify "funny"?
> 

I should clarify - I don't think Attachment 8528463 [details] [diff] (aka the builds in the try push) makes it look funny, but rather changes I made on top of that locally to try and match it up to the original mockups.  And by funny, I mean specifically that the code editor syntax highlighting looks a little jarring and hard to read: https://www.dropbox.com/sh/jd9wpaii00julyb/AAAflrEJdfTLkASSYRJGMFj9a?dl=0#lh:null-Screenshot 2015-04-01 11.57.53.png.

For comparison, I've sent a try push with that change (the "funny" one) on top of the current patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6e274d2259f1.

> They mostly look good to me. Rationale for brightening the colors is to a)
> increase contrast overall and b) make colors more visually distinct from
> each other.
> 
> Some of them do seem a little bright in that try-build, we could tweak them
> some to smooth them out.

I'm happy with the changes in that try build that you used and am ready to land them right now if you are OK with that.  We would have the whole 40 cycle to tweak them as necessary.

Comment 60

[Stephen Horlander [:shorlander]]

(In reply to Brian Grinstead [:bgrins] from comment #59)
> I'm happy with the changes in that try build that you used and am ready to
> land them right now if you are OK with that.  We would have the whole 40
> cycle to tweak them as necessary.

Works for me!

Comment 61

[Brian Grinstead [:bgrins]]

(In reply to Stephen Horlander [:shorlander] from comment #58)
> It is probably also true that these colors won't work in all contexts. Which
> means we could either spec out a corresponding bright and dark variant of
> the baseline palette, or do as suggested and dial them up/down
> programmatically.

That does sound useful.  For now it will have to be manual tuning since I'm guessing 1128204 is a ways off.

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #57)
> As a temporary solution, we can reference these values with a new name for
> the time being. `--theme-new-hightlight-green` or something, just so the
> performance graphs can use them

I'd prefer if we made it a variation of the new colors (i.e. as a follow up to this bug) instead of treating the old palette as 'dark' and the new palette as 'bright'.

Comment 62

[Brian Grinstead [:bgrins]]

Attachment: new-theme-colors.patch

Same patch, just updated the commit message

Comment 63

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8587423 [details] [diff] [review]
new-theme-colors.patch

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

Great! I think some of the text with this color are pretty bright, but yeah we'll have time to tweak that, maybe with text versions of these colors

Comment 64

[Brian Grinstead [:bgrins]]

Attachment: new-theme-colors.patch

Had to update hardcoded colors in assertions for browser/devtools/shared/test/browser_theme.js.

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5f55bca4208e

Comment 65

[Brian Grinstead [:bgrins]]

remote:   https://hg.mozilla.org/integration/fx-team/rev/05ed7dd4126b

Comment 66

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/05ed7dd4126b

Comment 67

[Liz Henry (:lizzard) (relman/hg->git project)]

Release Note Request (optional, but appreciated)
[Why is this notable]: Nice to note for Dev Edition 40
[Suggested wording]: New theme colors for DevTools
[Links (documentation, blog post, etc)]: https://developer.mozilla.org/en-US/docs/Tools/DevToolsColors

Comment 68

[Lawrence Mandel [:lmandel] (use needinfo)]

I don't think the colour tweaks that we've made are significant enough to call out in the notes. Liz, I hope you don't mind that I dropped this change for the Dev Edition 40 release notes.