957117 - DevTools Themes: Use light theme for top tabbar and toolbars

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Brian Grinstead [:bgrins]]

The top tab bar and toolbars are the last main holdouts in the light theme that are still dark.

Comment 1

[Brian Grinstead [:bgrins]]

Attachment: wip-screenshot

This is a screenshot where I am using an SVG filter style in CSS to invert the dark theme white icons into black icons for the light theme.

This is helpful especially in the tab icons, since we want to use the white icon on the selected tab but the dark icon on the others, and the icon is specified as a URL in the toolbox so this gets complicated.  Using the filter lets us easily pick which one to use and don't need to add the additional images.

Comment 2

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

Lookin good!

Comment 3

[Brian Grinstead [:bgrins]]

Attachment: debugger-light.png

Current UI after applying fix for Bug 943883 and making a few changes to widgets.css

Comment 4

[Brian Grinstead [:bgrins]]

Darrin, there is a build here that you can download and run: https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-12971a148eda/.  It is similar to the UI seen in Attachment 8357177 [details], with a few updates.  Can you click around in the light theme and give me some impressions?

A couple of specific things I'm looking for feedback on:
* I changed the breadcrumbs from what is seen in previous screenshot.  I don't think it is perfect, but with a couple of tweaks could it be 'good enough' until we land Bug 936421?  Or should we wait until the updates for breadcrumbs to the new styles are done before proceeding?
* The style editor left side and network panel listing is still dark.  At least in both of these the top toolbar directly against the tab bar is light, so it isn't as bad as it could be - but should we make sure one or both of these is done before proceeding?  I think that updating the style editor will be relatively quick, and updating the network panel take more time.
* Buttons on debugger / webconsole - I'm just using some flat colors for now for the normal / hover / active states.  I think it looks reasonable at least until we do Bug 952100 but let me know if you see any changes we should make.
* Icon coloring - I'm auto generating the color right now using an SVG filter and flipping the white to black rather than using the light theme icons.  Do they look good, or should we go ahead and replace the icons with the light theme versions?

One issue I know of already:
* The 'inspect element' button is blurry.  But it is going away after Bug 916443 so I'm just going to leave it for now.

Comment 5

[Tim Nguyen :ntim]

(In reply to Brian Grinstead [:bgrins] from comment #4)
> Darrin, there is a build here that you can download and run:
> https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/
> [e-mail]-12971a148eda/.  It is similar to the UI seen in
> Attachment 8357177 [details], with a few updates.  Can you click around in
> the light theme and give me some impressions?
> 
> A couple of specific things I'm looking for feedback on:
> * I changed the breadcrumbs from what is seen in previous screenshot.  I
> don't think it is perfect, but with a couple of tweaks could it be 'good
> enough' until we land Bug 936421?  Or should we wait until the updates for
> breadcrumbs to the new styles are done before proceeding?
> * The style editor left side and network panel listing is still dark.  At
> least in both of these the top toolbar directly against the tab bar is
> light, so it isn't as bad as it could be - but should we make sure one or
> both of these is done before proceeding?  I think that updating the style
> editor will be relatively quick, and updating the network panel take more
> time.
> * Buttons on debugger / webconsole - I'm just using some flat colors for now
> for the normal / hover / active states.  I think it looks reasonable at
> least until we do Bug 952100 but let me know if you see any changes we
> should make.
> * Icon coloring - I'm auto generating the color right now using an SVG
> filter and flipping the white to black rather than using the light theme
> icons.  Do they look good, or should we go ahead and replace the icons with
> the light theme versions?
> 
> One issue I know of already:
> * The 'inspect element' button is blurry.  But it is going away after Bug
> 916443 so I'm just going to leave it for now.

I think it's pretty good in overall, I just found 2 issues :
- Some parts are still not in light theme (netmonitor, style editor sidebar)
- Icons have an unwanted shadow, but I guess that's because you inverted the light icons.

Comment 6

[Darrin Henein [:darrin]]

> * I changed the breadcrumbs from what is seen in previous screenshot.  I
> don't think it is perfect, but with a couple of tweaks could it be 'good
> enough' until we land Bug 936421?  Or should we wait until the updates for
> breadcrumbs to the new styles are done before proceeding?

I think we should wait for the new breadcrumb style, both for the style and the slimmer toolbar.

> * The style editor left side and network panel listing is still dark.  At
> least in both of these the top toolbar directly against the tab bar is
> light, so it isn't as bad as it could be - but should we make sure one or
> both of these is done before proceeding?  I think that updating the style
> editor will be relatively quick, and updating the network panel take more
> time.

Again, I would rather wait and proceed once the theme is at least touching each panel. Let's identify what you need for the remaining panels and get on it :)

> * Buttons on debugger / webconsole - I'm just using some flat colors for now
> for the normal / hover / active states.  I think it looks reasonable at
> least until we do Bug 952100 but let me know if you see any changes we
> should make.

They look fine for now, I'd rather spend energy on the new look.

> * Icon coloring - I'm auto generating the color right now using an SVG
> filter and flipping the white to black rather than using the light theme
> icons.  Do they look good, or should we go ahead and replace the icons with
> the light theme versions?

Tab bar icons look good. The only issues I have (I believe ntim mentioned above) is the active state on some of the right-side command buttons... the blue icon has a drop shadow which looks bad. However, I know that some icons have the drop shadow and some don't, so I can always remove the shadow from the ones that have it. It's not really noticeable on the dark theme anyway.

Lastly, I think some of the borders/lines are still too dark in the light theme. See my attachment to see what I mean.

Comment 7

[Darrin Henein [:darrin]]

Attachment: Lines are too dark

Either choose a lighter color from the palette or I can specify something.

Comment 8

[Brian Grinstead [:bgrins]]

(In reply to Darrin Henein [:darrin] from comment #7)
> Created attachment 8357768 [details]
> Lines are too dark
> 
> Either choose a lighter color from the palette or I can specify something.

This arrow icon next to the line is an image with matching colors - itemArrow-ltr.png and itemArrow-rtl.png.  Can you pick a color for the lines and send over light theme versions of those arrow icons?  I'm using #aaa right now for some of the other splitters on the light theme - like in the inspector panel between markup view and CSS rules FWIW, but we could change it.

Comment 9

[Darrin Henein [:darrin]]

Attachment: itemArrow-ltr.svg @1x

Can we throw this in and see how it looks? The line is kept at #AAA and the fill is #F7F7F7 as you mentioned was being used in the gutter.

Comment 10

[Darrin Henein [:darrin]]

Attachment: itemArrow-ltr svg

LTR arrow.

Comment 11

[Darrin Henein [:darrin]]

Attachment: itemArrow-rtl svg

RTL arrow.

Comment 12

[Brian Grinstead [:bgrins]]

Attachment: light-theme.patch

Current version of light theme patch.  There is a build in progress at: https://tbpl.mozilla.org/?tree=Try&rev=9314e5e1e401

Things that have changed since last build:
* Use light theme for search results in inspector
* Update style editor to support light theme
* Use new, matching arrows for style editor and debugger sources list
* Use lighter colored separators everywhere in light theme
* Generally cleaned up a few rough edges

Still to do:
* Network panel (opened Bug 958231 for this, but will try to narrow the scope of the changes as much as possible without getting into a bigger net panel redesign)
* Breadcrumbs - realized when looking into the code that we also need to support RTL here, so it may be bigger than it once seemed
* Update command icons that have drop shadows in active state - I have a needinfo for Darrin over at Bug 957291 for this

Comment 13

[Brian Grinstead [:bgrins]]

Attachment: netpanel-light-vs-dark.png

Pulled out dark specific styles on network panel.  I think there is still a lot more we can do with styling the timeline and other parts of this UI - most of those changes should probably be done separately in Bug 909251.  But if there is anything you see that should definitely be changed for this, let me know.

Comment 14

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

A few thoughts:
* the table borders are much too dark; they add unnecessary noise
* the waterfall is very hard to understand on a light background (please post a screenshot without the sidebar expanded, and only a few requests, you'll see why I'm so very worried about this)
* the status code colored orbs are much too dark with the light background

Comment 15

[Tim Nguyen :ntim]

(In reply to Brian Grinstead [:bgrins] from comment #13)
> Created attachment 8358437 [details]
> netpanel-light-vs-dark.png
> 
> Pulled out dark specific styles on network panel.  I think there is still a
> lot more we can do with styling the timeline and other parts of this UI -
> most of those changes should probably be done separately in Bug 909251.  But
> if there is anything you see that should definitely be changed for this, let
> me know.

The 2px border definitively needs to be changed into a 1px one.

Comment 16

[Brian Grinstead [:bgrins]]

(In reply to Victor Porof [:vp] from comment #14)
> A few thoughts:
> * the table borders are much too dark; they add unnecessary noise

Updated.

> * the waterfall is very hard to understand on a light background (please
> post a screenshot without the sidebar expanded, and only a few requests,
> you'll see why I'm so very worried about this)

I was hoping to not tweak the colors too much so we didn't have to come up with new designs, updates https://developer.mozilla.org/en-US/docs/Tools/Network_Monitor, etc, especially for designs that will probably change with Bug 909251.  But we should definitely make sure it is readable - I'll post a screenshot with just a few requests.

> * the status code colored orbs are much too dark with the light background

Made them lighter.

Comment 17

[Brian Grinstead [:bgrins]]

Attachment: network-light-vs-dark-few-requests.png

Updated screenshot with a few requests and no request selected, after some UI changes.

Comment 18

[Brian Grinstead [:bgrins]]

Attachment: light-theme.patch

This is the updated patch that includes the network panel.  Have an ongoing build for binary downloads here: https://tbpl.mozilla.org/?tree=Try&rev=4f7786d22e85

Comment 19

[Brian Grinstead [:bgrins]]

Attachment: theme-light-apply-after-8367079.patch

Victor, can you review the changes for the network panel and debugger?  These changes are in netmonitor.inc.css, debugger.inc.css, widgets.inc.css for the side menu widget, and splitview.css (which has been moved into a single shared file).

Patrick, can you have a look at the other changes?  This will not be the final patch, since it still needs some UI review, but I think the code is close enough as far as the structural changes go that it is worth reviewing at this point.

** Note that this needs to be applied after https://bug936421.bugzilla.mozilla.org/attachment.cgi?id=8367079 in order to apply.

I also have a try push for binaries: https://tbpl.mozilla.org/?tree=Try&rev=f68874f39062.

Comment 20

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

Couldn't apply this on fx-team tip. Can you please rebase?

Comment 21

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

Oh, I need to apply a patch first!

Comment 22

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

Comment on attachment 8367112 [details] [diff] [review]
theme-light-apply-after-8367079.patch

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

Some general feedback first: (I'm not sure if this is caused by this patch or the one in bug 936421, but I'm posting all of it here anyway!)

0. Everything looks so different! So amazingly different. I feel like I'm falling in love again.

1. The glow sticks in the netmonitor waterfall still look awful. IMHO, a minimal effort of making them look at least a little bit better should be made before landing these patches. Removing the shadow and makings things look a little more flat would be nice, probably.

2. The borders in the netmonitor table still look much too dark to my eye. I'd make things about 4 times lighter, even. I would like us to the same relative contrast between lines and backgrounds.

3. The gradient in the netmonitor footer (for the filtering buttons) is a bit too out of line with the rest of the flat UI. Either remove it, or make it much more subtle.

4. When you start the netmonitor, the "Perform a request or reload the page..." notice has a white background, in contrast with the gray-ish background of everything else. Not sure I like that, especially since on the dark theme the backgrounds are the same.

5. When you hover tabs in the toolbox, in the light theme, the subtle dark border on the bottom of each tab disappears. This is different from what happens on the dark theme, and looks pretty weird to my eye. Let's be consistent.

6. If you blackbox a source in the debugger, in the light theme, you won't see the blackbox icon for the "Stop blackboxing this source" button anymore.

7. In the dark theme, selected items in the debugger's sources list have their blue background bleed over their bottom border.

8. In the style editor, the arrows for each css file are cut on the right by the splitter.

9. Is there another bug filed about removing the remaining padding from the breadcrumbs and other toolbars? The mockups show the breadcrumbs without any padding as far as I can tell.

10. In the light theme, if the debugger is paused and there are some watch expressions added, there's no separator between the watch expressions scope and the first other scope, in the variables view. The borders are a bit inconsistent.

Again, this looks amazing. I love it. Keep up the good work.

There's a lot of stuff in this patch. I'll have another go through it once you address the comments below.

::: browser/themes/shared/devtools/debugger.inc.css
@@ +14,5 @@
>  }
>  
>  #sources-pane .devtools-toolbar {
>    border: none; /* Remove the devtools-toolbar's black bottom border. */
>    -moz-border-end: 1px solid #222426; /* Match the sources list's dark margin. */

Put the color for this in a different rule, under .theme-dark

@@ +17,5 @@
>    border: none; /* Remove the devtools-toolbar's black bottom border. */
>    -moz-border-end: 1px solid #222426; /* Match the sources list's dark margin. */
>  }
>  
> +.theme-light #sources-pane > tabs,

Why aren't you changing just the .devtools-toolbar color here, so that it's symmetrical to the rule above? If there are any borders that need different colors, they should probably be for different rules in different files. Like toolbars.inc.css, where the tabs colors are defined :)

@@ +19,5 @@
>  }
>  
> +.theme-light #sources-pane > tabs,
> +.theme-light #sources-pane .devtools-toolbar {
> +  -moz-border-end-color: #aaa;

Are the other separators #aaa in the light theme?

@@ +528,5 @@
>    -moz-image-region: rect(0px,32px,16px,16px);
>  }
>  
> +.theme-light #resume[checked] > image {
> +  filter: none !important;

Might want to add a comment here about how the [checked] image is blue and we don't want to invert that.

::: browser/themes/shared/devtools/netmonitor.inc.css
@@ +7,4 @@
>    background: url(background-noise-toolbar.png), #343c45; /* Dark toolbars */
>  }
>  
> +.theme-dark #requests-menu-empty-notice {

Please accompany all .theme-dark rules with .theme-white rules, or have none of them, where it makes sense. I think it makes sense here (see my preliminary observations.

@@ +21,3 @@
>  %filter substitution
>  %define tableBorderLight rgba(0,0,0,0.2)
>  %define tableBorderDark rgba(128,128,128,0.15)

Should split these for dark vs. light themes, like in widgets.inc.css for the SideMenuWidget. Ideally, we'd have something like this:

%define table_itemDarkStartBorder rgba(0,0,0,0.2)
%define table_itemDarkEndBorder rgba(128,128,128,0.15)
%define table_itemLightStartBorder rgba(128,128,128,0.25)
%define table_itemLightEndBorder transparent

See below how these should be used.

@@ +51,5 @@
>  
> +.theme-light .requests-menu-header:not(:last-child),
> +.theme-light .requests-menu-subitem:not(:last-child) {
> +  -moz-border-end: 1px solid #aaa;
> +}

All of this should now simply be:

.theme-dark .requests-menu-header:not(:last-child),
.theme-dark .requests-menu-subitem:not(:last-child) {
  -moz-border-end: 1px solid @table_itemDarkStartBorder@;
  box-shadow: 1px 0 0 @table_itemDarkEndBorder@;
}

.theme-light .requests-menu-header:not(:last-child),
.theme-light .requests-menu-subitem:not(:last-child) {
  -moz-border-end: 1px solid @table_itemLightStartBorder@;
  box-shadow: 1px 0 0 @table_itLightrkEndBorder@;
}

@@ +141,2 @@
>  
>  box.requests-menu-status:not([code]) {

Add a .theme-dark here and for every other rule.

@@ +301,5 @@
>    border: 1px solid #fff;
>  }
>  
> +.theme-light .requests-menu-timings-cap {
> +  border: 1px solid #8fa1b2;

Document this color: /* Grey content text */
Use the mirrored color for the dark theme in a new rule.

@@ +340,5 @@
>  }
>  
> +.theme-light .requests-menu-timings-box {
> +  border-top: 1px solid #8fa1b2;
> +  border-bottom: 1px solid #8fa1b2;

Ditto!

Btw, these aren't visible in the light theme at all.

@@ +372,5 @@
>                0 0 4px 0 rgba(255,255,255,1.0) inset;
>  }
>  
> +.requests-menu-timings-box.send,
> +.requests-menu-timings-cap.send {

Forgot a .theme-light here?
Add .theme-dark for the other rules.

@@ +417,5 @@
>    background: rgba(255,255,255,0.05);
>  }
>  
> +.theme-light .side-menu-widget-item:not(.selected)[odd] {
> +  background: rgba(128, 128, 128, 0.05);

Nit: this file uses rgba()s with no commas between color components. Let's try to be consistent.

@@ +422,4 @@
>  }
>  
>  .theme-light .side-menu-widget-item {
> +  border-bottom: 1px solid #aaa;

This isn't needed anymore.

@@ +565,1 @@
>    color: #f5f7fa; /* Light foreground text */

Add corresponding .theme-light rules with the mirror color.

::: browser/themes/shared/devtools/profiler.inc.css
@@ -17,5 @@
>  }
>  
> -.profiler-sidebar + .devtools-side-splitter {
> -  border-color: transparent;
> -}

Why did you remove this? Now for each profile the item arrow is cut on the right, just like in the style editor.

@@ +44,5 @@
>    display: block;
>  }
>  
> +.theme-dark .selected .profiler-sidebar-item > hbox {
> +  color: #b6babf;

Add a corresponding .theme-light rule with the mirror color.

Comment 23

[Brian Grinstead [:bgrins]]

Comment on attachment 8367112 [details] [diff] [review]
theme-light-apply-after-8367079.patch

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

OK, I'm going to upload a new patch with these changes applied.  I've made notes inline to keep track of what changed, and here are responses to the general feedback:


> 1. The glow sticks in the netmonitor waterfall still look awful. IMHO, a
> minimal effort of making them look at least a little bit better should be
> made before landing these patches. Removing the shadow and makings things
> look a little more flat would be nice, probably.

I cut back on the shadow blurs significantly for both dark and light theme (from 8px/4px to 2px/1px).  Will post a screenshot. Let me know if that is better, or if you think we need to make further changes still.

> 2. The borders in the netmonitor table still look much too dark to my eye.
> I'd make things about 4 times lighter, even. I would like us to the same
> relative contrast between lines and backgrounds.

Made this lighter: from #aaa to #ebeced

> 3. The gradient in the netmonitor footer (for the filtering buttons) is a
> bit too out of line with the rest of the flat UI. Either remove it, or make
> it much more subtle.

Done

> 4. When you start the netmonitor, the "Perform a request or reload the
> page..." notice has a white background, in contrast with the gray-ish
> background of everything else. Not sure I like that, especially since on the
> dark theme the backgrounds are the same.

Done

> 5. When you hover tabs in the toolbox, in the light theme, the subtle dark
> border on the bottom of each tab disappears. This is different from what
> happens on the dark theme, and looks pretty weird to my eye. Let's be
> consistent.

Done

> 6. If you blackbox a source in the debugger, in the light theme, you won't
> see the blackbox icon for the "Stop blackboxing this source" button anymore.

Done

> 7. In the dark theme, selected items in the debugger's sources list have
> their blue background bleed over their bottom border.

Done

> 8. In the style editor, the arrows for each css file are cut on the right by
> the splitter.

Yes, I believe this is an existing bug.  I can look into it anyway, hope it won't be too big of a deal

> 9. Is there another bug filed about removing the remaining padding from the
> breadcrumbs and other toolbars? The mockups show the breadcrumbs without any
> padding as far as I can tell.
>

Yes, after Bug 952100 lands we will be able to remove the remaining space on the toolbar.

> 10. In the light theme, if the debugger is paused and there are some watch
> expressions added, there's no separator between the watch expressions scope
> and the first other scope, in the variables view. The borders are a bit
> inconsistent.

Done - made the splitter transparent and added top borders onto the section headers.

::: browser/themes/shared/devtools/debugger.inc.css
@@ +14,5 @@
>  }
>  
>  #sources-pane .devtools-toolbar {
>    border: none; /* Remove the devtools-toolbar's black bottom border. */
>    -moz-border-end: 1px solid #222426; /* Match the sources list's dark margin. */

Done

@@ +17,5 @@
>    border: none; /* Remove the devtools-toolbar's black bottom border. */
>    -moz-border-end: 1px solid #222426; /* Match the sources list's dark margin. */
>  }
>  
> +.theme-light #sources-pane > tabs,

The comments and code were out of date.  We need to match the splitter color on the end of the sidebar, which is #aaa or black, for light and dark.  I've updated 
this, and made it more symmetrical.

@@ +528,5 @@
>    -moz-image-region: rect(0px,32px,16px,16px);
>  }
>  
> +.theme-light #resume[checked] > image {
> +  filter: none !important;

Done

::: browser/themes/shared/devtools/netmonitor.inc.css
@@ +7,4 @@
>    background: url(background-noise-toolbar.png), #343c45; /* Dark toolbars */
>  }
>  
> +.theme-dark #requests-menu-empty-notice {

Removing the rule

@@ +51,5 @@
>  
> +.theme-light .requests-menu-header:not(:last-child),
> +.theme-light .requests-menu-subitem:not(:last-child) {
> +  -moz-border-end: 1px solid #aaa;
> +}

Updated (but kept ltr and rtl for both light and dark because of box shadow offset)

@@ +141,2 @@
>  
>  box.requests-menu-status:not([code]) {

Done

@@ +301,5 @@
>    border: 1px solid #fff;
>  }
>  
> +.theme-light .requests-menu-timings-cap {
> +  border: 1px solid #8fa1b2;

Done

@@ +340,5 @@
>  }
>  
> +.theme-light .requests-menu-timings-box {
> +  border-top: 1px solid #8fa1b2;
> +  border-bottom: 1px solid #8fa1b2;

Done.  Also switched timings borders to light content text / dark content text for dark and light theme

@@ +372,5 @@
>                0 0 4px 0 rgba(255,255,255,1.0) inset;
>  }
>  
> +.requests-menu-timings-box.send,
> +.requests-menu-timings-cap.send {

Yes, this one should have .theme-light.  I didn't add theme-dark to the others so they could share the same colors (blocked / dns / connect) so we wouldn't need to update the documentation for waterfall colors.  I will add dark theme in for the ones that do override on light.

@@ +565,1 @@
>    color: #f5f7fa; /* Light foreground text */

Done

::: browser/themes/shared/devtools/profiler.inc.css
@@ -17,5 @@
>  }
>  
> -.profiler-sidebar + .devtools-side-splitter {
> -  border-color: transparent;
> -}

It was removed so that we could see the border along the toolbar's side, but I will handle this as we did in debugger.  I've also now added a helper class called devtools-invisible-splitter that hides the splitter colors, since this is used in a couple of places at least

@@ +44,5 @@
>    display: block;
>  }
>  
> +.theme-dark .selected .profiler-sidebar-item > hbox {
> +  color: #b6babf;

Done

Comment 24

[Brian Grinstead [:bgrins]]

Attachment: network-updated.png

Less prominent horizontal lines, and much smaller box shadow than before

Comment 25

[Brian Grinstead [:bgrins]]

Attachment: light-theme2.patch

Alright, here is the patch with feedback addressed as noted in Comment 23

Comment 26

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

Comment on attachment 8367397 [details] [diff] [review]
light-theme2.patch

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

* The welcome text when the netmonitor is opened is now blue. I don't think that's a good idea.
* The item arrows in the style editor's styles list are still cut on the right.
* Should maybe make bug 952100 depend on this one?
* Not directly related to this bug but, for some reason, in RTL mode, the sidebar in the netmonitor makes the waterfall act and resize weirdly. I'll try to find a regression window :/

I'll play with this some more.

::: browser/themes/shared/devtools/debugger.inc.css
@@ -254,5 @@
>    color: inherit !important;
>  }
>  
>  /* Watch expressions view */
> -

Nit: no need to remove this line.

::: browser/themes/shared/devtools/netmonitor.inc.css
@@ +15,5 @@
>  %filter substitution
> +%define table_itemDarkStartBorder rgba(0,0,0,0.2)
> +%define table_itemDarkEndBorder rgba(128,128,128,0.15)
> +%define table_itemLightStartBorder #ebeced
> +%define table_itemLightEndBorder rgba(0,0,0,0.1)

I think you should give

%define table_itemLightStartBorder rgba(128,128,128,0.25)
%define table_itemLightEndBorder transparent

a shot here. I think it looks much better, and avoids having vertical lines in the middle of the selected items.

@@ +40,5 @@
>  }
>  
> +.theme-dark .requests-menu-header:not(:last-child):-moz-locale-dir(rtl),
> +.theme-dark .requests-menu-subitem:not(:last-child):-moz-locale-dir(rtl) {
> +  box-shadow: -1px 0 0 @table_itemDarkEndBorder@;

Forgot a -moz-border-end here.

@@ +51,5 @@
> +}
> +
> +.theme-light .requests-menu-header:not(:last-child):-moz-locale-dir(rtl),
> +.theme-light .requests-menu-subitem:not(:last-child):-moz-locale-dir(rtl) {
> +  box-shadow: -1px 0 0 @table_itemLightEndBorder@;

Forgot a -moz-border-end here.

@@ +361,5 @@
>  .requests-menu-timings-box.dns,
>  .requests-menu-timings-cap.dns {
>    background-color: rgba(255,128,255,0.6);
> +  box-shadow: 0 0 2px 0 rgba(128,128,255,1.0),
> +              0 0 1px 0 rgba(255,255,255,1.0) inset;

No different colors across themes for .blocked and .dns? Just making sure you didn't forget about it.

@@ +419,5 @@
>  .side-menu-widget-item-contents {
>    padding: 0px;
>  }
>  
>  .side-menu-widget-item:not(.selected)[odd] {

Add a .theme-dark before this rule.

@@ +429,4 @@
>  }
>  
>  .theme-light .side-menu-widget-item {
> +  border-bottom: 1px solid #ebeced;

You didn't remove this. There's no need for it.

@@ +442,1 @@
>    background: #343c45; /* Dark toolbars */

No light theme equivalent here? In that case, maybe there's no need for the explicit dark theme background either?

::: browser/themes/shared/devtools/widgets.inc.css
@@ +230,5 @@
>  
> +
> +.theme-light .breadcrumbs-widget-item[checked] {
> +  background: -moz-element(#breadcrumb-separator-before) no-repeat 0 0;
> +  background-color: #4c9ed9;

What are the dark theme equivalents of all these colors? I think it's a good idea to add symmetrical .theme-dark rules here. Also, can you please name the colors, so when switching to CSS variables our lives will be easier?

@@ +416,5 @@
>    box-shadow: inset 0 -1px 0 @smw_itemLightTopBorder@;
>  }
>  
> +.theme-dark .side-menu-widget-item.selected {
> +  background: #1d4f73;

Nit: please name the colors.

@@ +460,4 @@
>  }
>  
>  .theme-light .side-menu-widget-item.selected > .side-menu-widget-item-arrow:-moz-locale-dir(rtl) {
> +  background-image: url(itemArrow-rtl.svg), linear-gradient(to right, #aaa, #aaa);

Nit: could use a variable for all these #aaa's

@@ +560,5 @@
>    color: #585959; /* Grey foreground text */
>  }
>  
> +.theme-dark .variables-view-scope > .title {
> +  color: #fff;

Shouldn't we use a color from the pallette for this?

@@ +561,5 @@
>  }
>  
> +.theme-dark .variables-view-scope > .title {
> +  color: #fff;
> +}

Light theme equivalent?

Comment 27

[Brian Grinstead [:bgrins]]

Attachment: light-theme3.patch

Thank you for the great comments!  I've left comments inline below: 

(In reply to Victor Porof [:vp] from comment #26)
> Comment on attachment 8367397 [details] [diff] [review]
> light-theme2.patch
> 
> Review of attachment 8367397 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> * The welcome text when the netmonitor is opened is now blue. I don't think
> that's a good idea.

Not sure exactly which text you are talking about here.  The empty "Perform a request or reload the page to see detailed information about network activity" message?  I started looking into this, and realized that for the dark theme, the side-menu-widget had a custom background color set, instead of relying on theme-sidebar.

In order to make this work, I've removed the custom color on side-menu-widget.  Then I decided to try updating the background for the main dark theme body and sidebar to match the new colors (#14171a and #181d20 instead of #131c26 for both).  Will definitely want to have Darrin weigh in on whether we roll with the new colors or stick with the old (I could flip back to the old with no harm).

> * The item arrows in the style editor's styles list are still cut on the
> right.

Added devtools-invisible-splitter class, and updated styles to prevent the arrow being cut off.  Definitely an improvement over the current nightlies!

> * Should maybe make bug 952100 depend on this one?

Done

> 
> ::: browser/themes/shared/devtools/debugger.inc.css
> @@ -254,5 @@
> >    color: inherit !important;
> >  }
> >  
> >  /* Watch expressions view */
> > -
> 
> Nit: no need to remove this line.
> 

Done 

> ::: browser/themes/shared/devtools/netmonitor.inc.css
> @@ +15,5 @@
> >  %filter substitution
> > +%define table_itemDarkStartBorder rgba(0,0,0,0.2)
> > +%define table_itemDarkEndBorder rgba(128,128,128,0.15)
> > +%define table_itemLightStartBorder #ebeced
> > +%define table_itemLightEndBorder rgba(0,0,0,0.1)
> 
> I think you should give
> 
> %define table_itemLightStartBorder rgba(128,128,128,0.25)
> %define table_itemLightEndBorder transparent
> 
> a shot here. I think it looks much better, and avoids having vertical lines
> in the middle of the selected items.
> 

Updated this.  Also positioned the waterfall background image over 1px so it doesn't look like a double border at the start of the waterfall grid on the light theme.

> @@ +40,5 @@
> >  }
> >  
> > +.theme-dark .requests-menu-header:not(:last-child):-moz-locale-dir(rtl),
> > +.theme-dark .requests-menu-subitem:not(:last-child):-moz-locale-dir(rtl) {
> > +  box-shadow: -1px 0 0 @table_itemDarkEndBorder@;
> 
> Forgot a -moz-border-end here.
> 
> @@ +51,5 @@
> > +}
> > +
> > +.theme-light .requests-menu-header:not(:last-child):-moz-locale-dir(rtl),
> > +.theme-light .requests-menu-subitem:not(:last-child):-moz-locale-dir(rtl) {
> > +  box-shadow: -1px 0 0 @table_itemLightEndBorder@;
> 
> Forgot a -moz-border-end here.
> 

I actually left them off since they were already defined by the previous rules (to prevent duplicating it).

> @@ +361,5 @@
> >  .requests-menu-timings-box.dns,
> >  .requests-menu-timings-cap.dns {
> >    background-color: rgba(255,128,255,0.6);
> > +  box-shadow: 0 0 2px 0 rgba(128,128,255,1.0),
> > +              0 0 1px 0 rgba(255,255,255,1.0) inset;
> 
> No different colors across themes for .blocked and .dns? Just making sure
> you didn't forget about it.
> 

Yeah, that is intentional - the colors are visible in both themes. I added a comment for this.

> @@ +419,5 @@
> >  .side-menu-widget-item-contents {
> >    padding: 0px;
> >  }
> >  
> >  .side-menu-widget-item:not(.selected)[odd] {
> 
> Add a .theme-dark before this rule.
> 
> @@ +429,4 @@
> >  }
> >  
> >  .theme-light .side-menu-widget-item {
> > +  border-bottom: 1px solid #ebeced;
> 
> You didn't remove this. There's no need for it.

Done and done.

> @@ +442,1 @@
> >    background: #343c45; /* Dark toolbars */
> 
> No light theme equivalent here? In that case, maybe there's no need for the
> explicit dark theme background either?
> 

You are correct, that was redundant.  Removed.

> ::: browser/themes/shared/devtools/widgets.inc.css
> @@ +230,5 @@
> >  
> > +
> > +.theme-light .breadcrumbs-widget-item[checked] {
> > +  background: -moz-element(#breadcrumb-separator-before) no-repeat 0 0;
> > +  background-color: #4c9ed9;
> 
> What are the dark theme equivalents of all these colors? I think it's a good
> idea to add symmetrical .theme-dark rules here. Also, can you please name
> the colors, so when switching to CSS variables our lives will be easier?
> 
> @@ +416,5 @@
> >    box-shadow: inset 0 -1px 0 @smw_itemLightTopBorder@;
> >  }
> >  
> > +.theme-dark .side-menu-widget-item.selected {
> > +  background: #1d4f73;
> 
> Nit: please name the colors.
> 

Added comments with color names.  Can make further refactors here (grouping the theme specific colors together) after bug 936421 lands.

> @@ +460,4 @@
> >  }
> >  
> >  .theme-light .side-menu-widget-item.selected > .side-menu-widget-item-arrow:-moz-locale-dir(rtl) {
> > +  background-image: url(itemArrow-rtl.svg), linear-gradient(to right, #aaa, #aaa);
> 
> Nit: could use a variable for all these #aaa's

Created smw_marginLight and replaced smw_margin with smw_marginDark.

> 
> @@ +560,5 @@
> >    color: #585959; /* Grey foreground text */
> >  }
> >  
> > +.theme-dark .variables-view-scope > .title {
> > +  color: #fff;
> 
> Shouldn't we use a color from the pallette for this?
> 
> @@ +561,5 @@
> >  }
> >  
> > +.theme-dark .variables-view-scope > .title {
> > +  color: #fff;
> > +}
> 
> Light theme equivalent?

Remove the dark theme color here - it wasn't needed.

Comment 28

[Brian Grinstead [:bgrins]]

Attachment: big-ui-comparison.png

Here is a screenshot showing an opened debugger / split console with:

1) Dark theme (with current body / sidebar colors)
2) Dark theme (with new body / sidebar colors)
3) Light theme

Comment 29

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

(In reply to Brian Grinstead [:bgrins] from comment #27)
> > Review of attachment 8367397 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > * The welcome text when the netmonitor is opened is now blue. I don't think
> > that's a good idea.
> 

> Not sure exactly which text you are talking about here.  The empty "Perform
> a request or reload the page to see detailed information about network
> activity" message?  I started looking into this, and realized that for the
> dark theme, the side-menu-widget had a custom background color set, instead
> of relying on theme-sidebar.
> 
> In order to make this work, I've removed the custom color on
> side-menu-widget.  Then I decided to try updating the background for the
> main dark theme body and sidebar to match the new colors (#14171a and
> #181d20 instead of #131c26 for both).  Will definitely want to have Darrin
> weigh in on whether we roll with the new colors or stick with the old (I
> could flip back to the old with no harm).
> 

Yes, I'm referring to the "perform a request etc."

I really don't like this change. Everything is much too different than what all the previous colors used to be. My only gripe was with the text *color*, not the background or anything else. I'd be much happier if you reverted this change. The only thing you had to do was use the correct text color for the light and dark themes.

Comment 30

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

Comment on attachment 8367524 [details] [diff] [review]
light-theme3.patch

This should be good to go, modulo only my previous comment which needs addressing; I'm very happy with everything else.

Comment 31

[Brian Grinstead [:bgrins]]

Attachment: light-theme4.patch

Victor, this is the same patch except that I've just reverted the background color changes for theme-body and theme-sidebar in the dark theme.  We will wait for Bug 947242 to make this change.

Darrin, I know you've gotten a chance to look over the a previous version, but I think this is ready for a UI review.  Here is the most recent build with the new patches applied: https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-6b4486ce1170/try-macosx64-debug/.  **Note that this binary is using some different background colors on the dark theme.. There is an ongoing build in progress here that uses the original background colors, but they are the same otherwise: https://tbpl.mozilla.org/?tree=Try&rev=ea6c46fbc897**

Comment 32

[Brian Grinstead [:bgrins]]

OK, latest binary (with current background colors on dark theme) is now available for download at https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-ea6c46fbc897/try-macosx64-debug/.

Comment 33

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

Comment on attachment 8367592 [details] [diff] [review]
light-theme4.patch

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

I'm seeing the exact same dark colors as the light-theme3 patch.

Comment 34

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

Attachment: nits

Here is what I'm talking about :) Only a few small changes.

I also fixed a few other very minor things, like the blue selection background still bleeding outside the items in the menu widget, the scopes still not having consistent borders (when there were more than two scopes in the variables view, they would be separated by a 2px thick border), removing some redundant background properties, etc.

This patch is basically the reason why I don't have friends :)

One more thing: the <tabs> in .devtools-sidebar-tabs are the only thing remaining in the UI with text shadows. I think we should remove those shadows.

Comment 35

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

Found another thing that should be themed: .requests-menu-timings-division, on the millisecond, second and minute scales. These are the dotted division lines above the requests waterfall that delimit the time passed. On the light theme, those divisions are not visible at all.

Comment 36

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

Attachment: nits2

And here's what I was talking about in comment 35.

Comment 37

[Brian Grinstead [:bgrins]]

(In reply to Victor Porof [:vp] from comment #34)
> Created attachment 8367810 [details] [diff] [review]
> nits
> 
> Here is what I'm talking about :) Only a few small changes.
> 
> I also fixed a few other very minor things, like the blue selection
> background still bleeding outside the items in the menu widget, the scopes
> still not having consistent borders (when there were more than two scopes in
> the variables view, they would be separated by a 2px thick border), removing
> some redundant background properties, etc.
> 
> This patch is basically the reason why I don't have friends :)
> 
> One more thing: the <tabs> in .devtools-sidebar-tabs are the only thing
> remaining in the UI with text shadows. I think we should remove those
> shadows.

(In reply to Victor Porof [:vp] from comment #35)
> Found another thing that should be themed: .requests-menu-timings-division,
> on the millisecond, second and minute scales. These are the dotted division
> lines above the requests waterfall that delimit the time passed. On the
> light theme, those divisions are not visible at all.

Both of these look good.  I will apply them on top of the current patch, remove the text shadow on the sidebar tabs, and reupload.

Comment 38

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

Attachment: breadcrumbs text

I just run into a couple of small things, I'm just curious about what you guys think about them:

* sometimes the text inside breadcrumbs (for classes, when items are selected) in the inspector is hard to read; i attached a screenshot showing this.

* the background of sidebar tabs when hovered is much darker than the background of toolbox tool tabs when hovered; is this intended?

Comment 39

[Darrin Henein [:darrin]]

(In reply to Victor Porof [:vp] from comment #38)
> * sometimes the text inside breadcrumbs (for classes, when items are
> selected) in the inspector is hard to read; i attached a screenshot showing
> this.

Agreed. Also the 1px border/shadow on the right/left of the active breadcrumb could probably go. I think it would look better with just a clean blue background.

Comment 40

[Brian Grinstead [:bgrins]]

> * the background of sidebar tabs when hovered is much darker than the
> background of toolbox tool tabs when hovered; is this intended?

The color is not intentional, in fact, it is using the same background color for hover in both dark and light theme right now.  I can update to use a more consistent color.

(In reply to Darrin Henein [:darrin] from comment #39)
> (In reply to Victor Porof [:vp] from comment #38)
> > * sometimes the text inside breadcrumbs (for classes, when items are
> > selected) in the inspector is hard to read; i attached a screenshot showing
> > this.
> 
> Agreed. Also the 1px border/shadow on the right/left of the active
> breadcrumb could probably go. I think it would look better with just a clean
> blue background.

OK, I can remove the border on the separators and update the text color for the classes.

Comment 41

[Brian Grinstead [:bgrins]]

Attachment: light-theme5.patch

Changes since last patch:

* Merged in nits.patch and nits2.patch to address some styling concerns
* Updated the active light theme breadcrumb to always have the active colored text
* Based this on the updated breadcrumb patch, which removes the borders on the selected crumb
* Changed the hover color for sidebar tabs to match the hover color for toolbox tabs

(Victor, I copied over the r+, feel free to change if there is anything else you think that needs addressing)

Try push: https://tbpl.mozilla.org/?tree=Try&rev=79f1cc50be9d

Comment 42

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

r+ all the way :)

I noticed two more things! Correct me if these are by design:

* The splitter that resizes the Toolbox is always light. Open a page with a dark background to see this. I think it should be black in that case instead.

* The shader editor looks horrific in the light theme :) I'll file a new bug and take care of this, no worries.

Comment 43

[Brian Grinstead [:bgrins]]

> * The splitter that resizes the Toolbox is always light. Open a page with a
> dark background to see this. I think it should be black in that case instead.

IIRC, this is loaded in the browser which isn't aware of dark and light themes so we need to share a color for this splitter.

Comment 44

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

(In reply to Brian Grinstead [:bgrins] from comment #43)
> > * The splitter that resizes the Toolbox is always light. Open a page with a
> > dark background to see this. I think it should be black in that case instead.
> 
> IIRC, this is loaded in the browser which isn't aware of dark and light
> themes so we need to share a color for this splitter.

I vote for dark.

Comment 45

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

I also filed bug 965819, not necessarily related to this one, but evident now with the distinction between light and dark themes.

Comment 46

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

Attachment: call stack

More feedback!
Selected items in the debugger's call stack tab look bad.

Comment 47

[Brian Grinstead [:bgrins]]

(In reply to Victor Porof [:vp] from comment #46)
> Created attachment 8367983 [details]
> call stack
> 
> More feedback!
> Selected items in the debugger's call stack tab look bad.

I think this is the background-clip: content-box for side menu widget items.  I think padding-box would solve the original problem (bleeding border colors) but prevent shrinking background from happening.

Comment 48

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

(In reply to Brian Grinstead [:bgrins] from comment #47)
> (In reply to Victor Porof [:vp] from comment #46)

Oh, that's interesting. I'll look into this.

Comment 49

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

Yup, background-clip: padding-box works! Nice catch Brian!

Comment 50

[Darrin Henein [:darrin]]

Comment on attachment 8367940 [details] [diff] [review]
light-theme5.patch

After a fairly superficial look at the new theme (i.e. haven't been using it for days) it looks great! I believe there are still some small visual nits to resolve but for the most part looks good, with the understanding that we will resolve some of these issues in future bugs (ie slim toolbar fixing breadcrumbs, borderless buttons, etc.) Thanks Brian!

Comment 51

[Brian Grinstead [:bgrins]]

Attachment: light-theme6.patch

Last batch of changes, ready for checkin

Comment 52

[Brian Grinstead [:bgrins]]

Attachment: light-theme6.patch

Last patch didn't have anything in it.

Comment 53

[Brian Grinstead [:bgrins]]

https://hg.mozilla.org/integration/fx-team/rev/ae4b3d2319b2
https://tbpl.mozilla.org/?tree=Fx-Team&rev=ae4b3d2319b2

Comment 55

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

Attachment: splitter

The toolbox splitter is still light.

Comment 56

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

Attachment: text shadows

The sidebar tabs still have text shadow.

Comment 57

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ae4b3d2319b2

Comment 58

[Brian Grinstead [:bgrins]]

(In reply to Victor Porof [:vp] from comment #55)
> Created attachment 8368457 [details]
> splitter
> 
> The toolbox splitter is still light.

We fixed this in Bug 966275