941799 - DevTools themes - variables view should match current theme

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[Brian Grinstead [:bgrins]]

Attachment: Screenshot 2013-11-21 13.15.24.png

When running `console.dir` with the dark theme, the inline variables view is quite... white (see screenshot)

Comment 1

[Brian Grinstead [:bgrins]]

There are a lot of hardcoded colors in widgets.css: https://mxr.mozilla.org/mozilla-central/source/browser/themes/osx/devtools/widgets.css#495.  We should be applying the theme classes for coloring here.

Comment 2

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

(In reply to Brian Grinstead [:bgrins] from comment #1)
> There are a lot of hardcoded colors in widgets.css:
> https://mxr.mozilla.org/mozilla-central/source/browser/themes/osx/devtools/
> widgets.css#495.  We should be applying the theme classes for coloring here.

Yeah, those are from way before the dark/light theme switching mechanism was in place. Ideally, all the token colorizing classes should be moved to the theme css files.

Comment 3

[Brian Grinstead [:bgrins]]

Attachment: theme-variable-view.patch

I've updated the styles on the variables view to use the respective theme styles.  The places I've checked are:

* console.dir() in webconsole
* cmd+I in scratchpad
* paused in debugger - sidebar with variables
* network panel - headers, cookies, and params

Two things I'd like to know:

1) Have I missed a spot where this widget is being used?
2) Please have a look at the variables view with this patch applied and let me know if there is anything that looks wrong / out of place.

Comment 4

[Brian Grinstead [:bgrins]]

Attachment: variables-view-light.png

Screenshot of light theme for variables view - to get a feel for it you should apply the patch though, since the UI can be so varied.

Comment 5

[Brian Grinstead [:bgrins]]

Attachment: variables-view-dark.png

Screenshot of dark theme for variables view - to get a feel for it you should apply the patch though, since the UI can be so varied.

Comment 6

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Brian Grinstead [:bgrins] from comment #3)
> 1) Have I missed a spot where this widget is being used?

It's also used in the App Manager for the manifest editor, though you currently have to set a pref (devtools.appmanager.manifestEditor.enabled) to enable it.  If you add a hosted project (such as http://bugmotodo.org/manifest.webapp), you can edit the app's manifest via an instance of the VariablesView.

It's not clear to me whether we want this particular instance to change appearance based on DevTools themes, since it has some specific color overrides and other tweaks which might not make sense for the dark theme.

I'll try out the patch soon and check it out too.

Comment 7

[Mihai Sucan [:msucan]]

Comment on attachment 8338049 [details] [diff] [review]
theme-variable-view.patch

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

Looks good to me. Thank you!

My only potential gripe is with the color for non-enumerable properties. Inspect |window| and see parseInt or the SVG* properties. The property name color is too dark, hard to read on a dark background.

I'm a light theme user, so this is really up to people who use dark themes to nit-pick on colors.:)

Comment 8

[Brian Grinstead [:bgrins]]

(In reply to Mihai Sucan [:msucan] from comment #7)
> Comment on attachment 8338049 [details] [diff] [review]
> theme-variable-view.patch
> 
> Review of attachment 8338049 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good to me. Thank you!
> 
> My only potential gripe is with the color for non-enumerable properties.
> Inspect |window| and see parseInt or the SVG* properties. The property name
> color is too dark, hard to read on a dark background.

This is actually set by a .5 opacity, so it would be very easy to bump this up a point or two.

Comment 9

[Brian Grinstead [:bgrins]]

Attachment: variables-theme-opacity.png

Comparison of different opacities for non-enumerable properties on dark theme

Comment 10

[Brian Grinstead [:bgrins]]

(In reply to J. Ryan Stinnett [:jryans] from comment #6)
> (In reply to Brian Grinstead [:bgrins] from comment #3)
> > 1) Have I missed a spot where this widget is being used?
> 
> It's also used in the App Manager for the manifest editor, though you
> currently have to set a pref (devtools.appmanager.manifestEditor.enabled) to
> enable it.  If you add a hosted project (such as
> http://bugmotodo.org/manifest.webapp), you can edit the app's manifest via
> an instance of the VariablesView.
> 
> It's not clear to me whether we want this particular instance to change
> appearance based on DevTools themes, since it has some specific color
> overrides and other tweaks which might not make sense for the dark theme.
> 
> I'll try out the patch soon and check it out too.

Looking at the manifest editor, I think having the specific overrides here is fine.  In addition to this, having theme-body doesn't do anything since the theme switching isn't loaded at all on that page.  Screenshot of manifest editor with patch applied incoming.

Comment 11

[Brian Grinstead [:bgrins]]

Attachment: manifest-editor-variables-view.png

Screenshot of manifest editor on hosted app with patch applied (seems to look the same as without patch)

Comment 12

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

Comment on attachment 8338049 [details] [diff] [review]
theme-variable-view.patch

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

Me likey!

Style comments:

1. Variables used to have different coloring vs. properties (blue vs. purple). This is regressed now. The previous distinction helped a quite a bit with nesting and distinguishing between the two in the debugger.

2. Non-enumerable variables and properties are very hard to see when using the dark theme. May I suggest a lighter color?

3. Function closures look really bad on dark backgrounds (dark blue with blue shadow on black). Same goes with all "special" properties (like return's, exceptions, etc.) These should be themed as well.

4. The coloring for data types doesn't match what's in the source editor. For example, strings are orange in the editor but green in the variables view. My vote is to match the source editor.

5. Focused items in the variables view now look strikingly weird with the dark theme. That should be themed as well.

6. Imho, the inline variables view border in the webconsole could be removed; It looks a bit hideous when using the dark theme.

Code comments below.

::: browser/themes/linux/devtools/debugger.css
@@ -87,5 @@
>    padding: 2px;
>  }
>  
> -.list-widget-item:not(.selected):not(.empty):hover {
> -  background: linear-gradient(rgba(255,255,255,0.9), rgba(255,255,255,0.85)), Highlight;

This was removed but not updated to match the theming. Watch expressions in the debugger aren't highlighted on hover anymore.

@@ -154,5 @@
>  
> -#instruments-pane > tabpanels > tabpanel {
> -  background: #fff;
> -}
> -

This removal makes the watch expressions in the debugger have a subtle dark shadow on top and rounded borders on the edges. Better make it match whatever the theme background is.

::: browser/themes/linux/devtools/widgets.css
@@ -463,4 @@
>  }
>  
>  .variable-or-property[changed] {
>    background: rgba(255,255,0,0.65);

This background should be based on the theme, should it not? Same comment applies to all colors in the variables view (and in the future, to all colors in this file).

::: browser/themes/osx/devtools/widgets.css
@@ +642,5 @@
>  }
>  
>  .element-value-input,
>  .element-name-input {
>    border: 1px solid #999 !important;

I suggest a transparent gray here, or having this themed as well. The border is much too light when using the dark theme.

Comment 13

[Brian Grinstead [:bgrins]]

(In reply to Victor Porof [:vp] from comment #12)
> Comment on attachment 8338049 [details] [diff] [review]
> theme-variable-view.patch
> 
> Review of attachment 8338049 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Me likey!
> 
> Style comments:
> 
> 1. Variables used to have different coloring vs. properties (blue vs.
> purple). This is regressed now. The previous distinction helped a quite a
> bit with nesting and distinguishing between the two in the debugger.
> 

Updated as part of (4).

> 2. Non-enumerable variables and properties are very hard to see when using
> the dark theme. May I suggest a lighter color?

Bumped up opacity a bit and switched color to the purple used elsewhere - seems more readable to me now, but may be worth another look.
 
> 3. Function closures look really bad on dark backgrounds (dark blue with
> blue shadow on black). Same goes with all "special" properties (like
> return's, exceptions, etc.) These should be themed as well.

Moved this color to the theme.  Also moved all of these special types to corresponding colors - may be worth another look:
.variable-or-property[return] .name
.variable-or-property[safe-getter] .name 
.variable-or-property[scope] .name
.variable-or-property[exception] .name 

> 4. The coloring for data types doesn't match what's in the source editor.
> For example, strings are orange in the editor but green in the variables
> view. My vote is to match the source editor.

Updated - the colors look a lot better now.

> 5. Focused items in the variables view now look strikingly weird with the
> dark theme. That should be themed as well.

Updated to match selection color in source editor
 
> 6. Imho, the inline variables view border in the webconsole could be
> removed; It looks a bit hideous when using the dark theme.

Tried removing it - but it is really weird and just kind of floating around without the border there.  I went ahead and used a transparent grey color and shrunk the radius a bit to make it stand out less though.

> Code comments below.
> 
> ::: browser/themes/linux/devtools/debugger.css
> @@ -87,5 @@
> >    padding: 2px;
> >  }
> >  
> > -.list-widget-item:not(.selected):not(.empty):hover {
> > -  background: linear-gradient(rgba(255,255,255,0.9), rgba(255,255,255,0.85)), Highlight;
> 
> This was removed but not updated to match the theming. Watch expressions in
> the debugger aren't highlighted on hover anymore.
> 

Added overrides for both light and dark themes here.

> @@ -154,5 @@
> >  
> > -#instruments-pane > tabpanels > tabpanel {
> > -  background: #fff;
> > -}
> > -
> 
> This removal makes the watch expressions in the debugger have a subtle dark
> shadow on top and rounded borders on the edges. Better make it match
> whatever the theme background is.

Applied theme-body to these elements.

> 
> ::: browser/themes/linux/devtools/widgets.css
> @@ -463,4 @@
> >  }
> >  
> >  .variable-or-property[changed] {
> >    background: rgba(255,255,0,0.65);
> 
> This background should be based on the theme, should it not? Same comment
> applies to all colors in the variables view (and in the future, to all
> colors in this file).
> 

I've used the 'flattened' yellow color for now (what it was actually showing up as on a white background): rgb(255, 248, 101).  It looks OK in both themes, I think.

> ::: browser/themes/osx/devtools/widgets.css
> @@ +642,5 @@
> >  }
> >  
> >  .element-value-input,
> >  .element-name-input {
> >    border: 1px solid #999 !important;
> 
> I suggest a transparent gray here, or having this themed as well. The border
> is much too light when using the dark theme.

Updated to transparent grey (matches the inline iframe in webconsole border color).

Comment 14

[Brian Grinstead [:bgrins]]

Attachment: theme-variable-view.patch

Updated from original patch based on feedback.  Take another look and let me know if you see anything else.

Comment 15

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

Comment on attachment 8339377 [details] [diff] [review]
theme-variable-view.patch

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

Much better! I still see a few tiny problems. Sorry about all of these comments, I'm just really careful about smallish details :)

1. The yellow highlight animation acts differently now: it no longer fades out, but immediately ends.

2. There's still no distinction between variables and properties (as far as I can see). They're both purple in the dark theme, blue in the light theme. They used to be different, let's keep them different.

3. Focused scopes/variables/properties are light blue in the dark theme. It looks very nice now with the light theme, but out of place with the dark theme.

4. (forgot about this previously) The color of the dotted underlines beneath variables/properties (suggesting about their configurable/enumerable/writeable state) should also be themed.

The following are just nits, you decide if they should block landing or not:

5. Nit: the yellow highlight is very, very contrast-y on the dark theme, and will likely burn my eyes when working late at night :) I'd suggest using a theme color instead of a prebaked yellow.

6. Nit: 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.

::: browser/themes/linux/devtools/debugger.css
@@ +91,1 @@
>    background: linear-gradient(rgba(255,255,255,0.9), rgba(255,255,255,0.85)), Highlight;

I think we shouldn't be using "Highlight" anymore, because that's the system's theme color. But I don't care that much.

::: browser/themes/linux/devtools/widgets.css
@@ +459,5 @@
>  }
>  
> +.variables-view-scope[changed] > .title,
> +.variable-or-property[changed] > .title {
> +  background: rgb(255, 248, 101);

As said before, I still think this (and the one above) should be from https://developer.mozilla.org/en-US/docs/Tools/DevToolsColors

@@ +489,5 @@
>    font-weight: 600;
>  }
>  
> +.variable-or-property:focus > .title > *,
> +.variable-or-property[changed] > .title > * {

Do we really have to use a * here? Since selectors are parsed from right to left, every single element in the DOM is considered to "potentially match this", until it hits the ".title" class, which is still pretty vague.

I'd prefer keeping all the rules below, even if it's pretty verbose.

Comment 16

[Brian Grinstead [:bgrins]]

I'll work on making the suggested changes.  A few questions though:

> 3. Focused scopes/variables/properties are light blue in the dark theme. It
> looks very nice now with the light theme, but out of place with the dark
> theme.

I'm already using the highlighted text color in the source editor.  Is there something else you think we should be using?

> 4. (forgot about this previously) The color of the dotted underlines beneath
> variables/properties (suggesting about their
> configurable/enumerable/writeable state) should also be themed.

When you say also be themed - are you meaning that it should match the text below, or just that it should use similar colors as currently but should match better on the dark theme?


> ::: browser/themes/linux/devtools/widgets.css
> @@ +459,5 @@
> >  }
> >  
> > +.variables-view-scope[changed] > .title,
> > +.variable-or-property[changed] > .title {
> > +  background: rgb(255, 248, 101);
> 
> As said before, I still think this (and the one above) should be from
> https://developer.mozilla.org/en-US/docs/Tools/DevToolsColors

Not sure that any of these would look particularly good for a flashy highlight effect which typically uses yellow (maybe light orange, I guess).  I'll take a look at this issue with the fading out and see if anything looks OK.

Comment 17

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #16)
> I'll work on making the suggested changes.  A few questions though:
> 
> > 3. Focused scopes/variables/properties are light blue in the dark theme. It
> > looks very nice now with the light theme, but out of place with the dark
> > theme.
> 
> I'm already using the highlighted text color in the source editor.  Is there
> something else you think we should be using?

Actually, I've applied this same selected background color but with some alpha, and I think it fits into the dark theme a little better.  I'll go with this for now.

Comment 18

[Brian Grinstead [:bgrins]]

Attachment: theme-variable-view.patch

FWIW, here is an update from the last patch with:

* Different colors for properties and variables
* Yellow highlight animation fades out again
* Yellow highlight color and blue selected color have both been softened with a little bit of transparency
* Using "label" instead of "*" to select multiple token types

Doesn't change the color of the dotted border below variables and properties to indicate different states - have a needinfo? on that.

Comment 19

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

(In reply to Brian Grinstead [:bgrins] from comment #16)
> I'll work on making the suggested changes.  A few questions though:
> 
> > 3. Focused scopes/variables/properties are light blue in the dark theme. It
> > looks very nice now with the light theme, but out of place with the dark
> > theme.
> 
> I'm already using the highlighted text color in the source editor.  Is there
> something else you think we should be using?
> 

I think the editor line highlight would work nicely for this.

Comment 20

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

(In reply to Brian Grinstead [:bgrins] from comment #16)
> 
> When you say also be themed - are you meaning that it should match the text
> below, or just that it should use similar colors as currently but should
> match better on the dark theme?

The underlines should only use colors from the devtools palette, both in light and dark themes. The rules should also be moved to the theme files.

Comment 21

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

Comment on attachment 8339541 [details] [diff] [review]
theme-variable-view.patch

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

The coloring for variables and properties is still broken. See attached screenshot and the explanation below.

::: browser/themes/osx/devtools/widgets.css
@@ +437,5 @@
>  
> +.variables-view-scope:focus > .title,
> +.variable-or-property:focus > .title {
> +  background: rgba(185, 215, 253, .8);
> +  color: black;

Please move these into the theme files. This is a theme-dependent color. If it changes, we shouldn't try to hunt it down in every css file.

@@ +456,3 @@
>  .variable-or-property[changed] {
> +  background: rgba(255, 248, 101, .8);
> +  color: black;

Same argument as above for this. If you move them to the theme file, in the dark theme use a lower alpha to make it look less contrasty.

@@ -484,5 @@
>  
>  .variables-view-variable {
>    -moz-margin-start: 1px;
>    -moz-margin-end: 1px;
> -  border-bottom: 1px solid #eee;

I've just noticed this removal. It used to help delimit variables in the debugger, I'd prefer if it was kept around, but you decide.

@@ +483,5 @@
>    font-weight: 600;
>  }
>  
> +.variable-or-property:focus > .title > label,
> +.variable-or-property[changed] > .title > label {

I don't think having .variable-or-property[changed] > .title > label here is necessary. It makes the color flash weirdly when changed a value is changed and breaks the animation.

@@ +503,5 @@
>    border-bottom: 1px dashed #f99;
>  }
>  
> +.variable-or-property[safe-getter] > .title > .name {
> +  border-bottom: 1px dashed #8b0;

All of these borders should be moved in the theme files and use colors from the devtools palette.

::: browser/themes/shared/devtools/dark-theme.css
@@ +92,5 @@
>  
>  .theme-fg-color1,
> +.cm-s-mozilla .cm-number,
> +.variable-or-property .token-number,
> +.variable-or-property[return] .name { /* green */

This should be .variable-or-property[return] > .title > .name

The variables view structure is a tree. If you don't use direct descendant selectors, the colors will be applied to all children instead of *only* the specified variable or property.

.variable-or-property
  .title
    .name
    .value
  .enum
    .variable-or-property
      .title
        .name
        .value
      .enum
        ...
      .nonenum
        ...
    ...
  .nonenum
    .variable-or-property
      .title
        .name
        .value
      .enum
        ...
      .nonenum
        ...
    ...

You don't want the .name selector to leak inside the children.

Otherwise, you'd end up with broken coloring for properties. Go to http://htmlpad.org/debugger/, click me and inspect "this" to see.

All of this should be done for the light theme as well.

@@ +102,5 @@
>  .cm-s-mozilla .cm-variable,
>  .cm-s-mozilla .cm-def,
>  .cm-s-mozilla .cm-property,
> +.cm-s-mozilla .cm-qualifier,
> +.variables-view-property .title .name,

This should be .variables-view-variable > .title > .name to have blue color on the variables and purple on properties.

@@ +103,5 @@
>  .cm-s-mozilla .cm-def,
>  .cm-s-mozilla .cm-property,
> +.cm-s-mozilla .cm-qualifier,
> +.variables-view-property .title .name,
> +.variable-or-property[scope] .name { /* blue */

.variable-or-property[scope] > .title > .name

@@ +111,5 @@
>  .theme-fg-color3,
>  .cm-s-mozilla .cm-builtin,
>  .cm-s-mozilla .cm-tag,
> +.cm-s-mozilla .cm-header,
> +.variable-or-property .name,

.variables-view-property > .title > .name

@@ +112,5 @@
>  .cm-s-mozilla .cm-builtin,
>  .cm-s-mozilla .cm-tag,
> +.cm-s-mozilla .cm-header,
> +.variable-or-property .name,
> +.variable-or-property[safe-getter] .name { /* pink/lavender */

.variable-or-property[safe-getter] > .title > .name

@@ +138,5 @@
>  .cm-s-mozilla .cm-atom,
>  .cm-s-mozilla .cm-quote,
> +.cm-s-mozilla .cm-error,
> +.variable-or-property .token-boolean,
> +.variable-or-property[exception] .name { /* Red */

.variable-or-property[exception] > .title > .name

Comment 22

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

Attachment: Screen Shot 2013-11-28 at 10.58.23.png

Comment 23

[Brian Grinstead [:bgrins]]

 > ::: browser/themes/osx/devtools/widgets.css
> @@ +437,5 @@
> >  
> > +.variables-view-scope:focus > .title,
> > +.variable-or-property:focus > .title {
> > +  background: rgba(185, 215, 253, .8);
> > +  color: black;
> 
> Please move these into the theme files. This is a theme-dependent color. If
> it changes, we shouldn't try to hunt it down in every css file.
> 
> @@ +456,3 @@
> >  .variable-or-property[changed] {
> > +  background: rgba(255, 248, 101, .8);
> > +  color: black;
> 
> Same argument as above for this. If you move them to the theme file, in the
> dark theme use a lower alpha to make it look less contrasty.
> 

In this case, I can move them to the theme files since it is quite similar to what I am doing with CodeMirror-activeline, and since we probably want slightly different colors for the different things.  See below for an argument for *not* doing this with the border colors, though.

> @@ -484,5 @@
> >  
> >  .variables-view-variable {
> >    -moz-margin-start: 1px;
> >    -moz-margin-end: 1px;
> > -  border-bottom: 1px solid #eee;
> 
> I've just noticed this removal. It used to help delimit variables in the
> debugger, I'd prefer if it was kept around, but you decide.
> 
> @@ +483,5 @@
> >    font-weight: 600;
> >  }
> >  
> > +.variable-or-property:focus > .title > label,
> > +.variable-or-property[changed] > .title > label {
> 
> I don't think having .variable-or-property[changed] > .title > label here is
> necessary. It makes the color flash weirdly when changed a value is changed
> and breaks the animation.
> 

I've removed this.

> @@ +503,5 @@
> >    border-bottom: 1px dashed #f99;
> >  }
> >  
> > +.variable-or-property[safe-getter] > .title > .name {
> > +  border-bottom: 1px dashed #8b0;
> 
> All of these borders should be moved in the theme files and use colors from
> the devtools palette.
> 

At this point, moving border colors into the theme files causes more trouble than it is worth IMO.  We don't want the theme file to have to know this much about the different widgets and how they are styled - once it starts 'fully' styling elements I believe those theme files are getting too  specific (these files are included in every panel in devtools, after all).  Once we have CSS variables we should be able to do something from this file like: `border-bottom: 1px dashed var(fg-color1)`.  I believe that this will be a better solution for this case and we should temporarily live with having hardcoded border colors in this file.

> ::: browser/themes/shared/devtools/dark-theme.css
> @@ +92,5 @@
> >  
> >  .theme-fg-color1,
> > +.cm-s-mozilla .cm-number,
> > +.variable-or-property .token-number,
> > +.variable-or-property[return] .name { /* green */
> 
> This should be .variable-or-property[return] > .title > .name

I've updated all of the selectors to use direct descendant.

Comment 24

[Brian Grinstead [:bgrins]]

(In reply to Victor Porof [:vp] from comment #21)
> Comment on attachment 8339541 [details] [diff] [review]
> theme-variable-view.patch
> 
> Review of attachment 8339541 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The coloring for variables and properties is still broken. See attached
> screenshot and the explanation below.
> 
> ::: browser/themes/osx/devtools/widgets.css
> @@ +437,5 @@
> >  
> > +.variables-view-scope:focus > .title,
> > +.variable-or-property:focus > .title {
> > +  background: rgba(185, 215, 253, .8);
> > +  color: black;
> 
> Please move these into the theme files. This is a theme-dependent color. If
> it changes, we shouldn't try to hunt it down in every css file.

I've moved this to the theme files, and am using fg-color2 for both themes, which is a blue color with white text.  I think it looks good, but check it out and let me know.

> 
> @@ +456,3 @@
> >  .variable-or-property[changed] {
> > +  background: rgba(255, 248, 101, .8);
> > +  color: black;
> 
> Same argument as above for this. If you move them to the theme file, in the
> dark theme use a lower alpha to make it look less contrasty.

I've switched the background to use theme-bg-darker colors for the background on changed elements in both themes - just like the change events in the inspector.

Comment 25

[Brian Grinstead [:bgrins]]

Attachment: theme-variable-view.patch

I believe this addresses all comments, but let me know if it is missing something or if you see something else that should be updated.

One more note I missed in the earlier comments:

> >  .variables-view-variable {
> >    -moz-margin-start: 1px;
> >    -moz-margin-end: 1px;
> > -  border-bottom: 1px solid #eee;
>
> I've just noticed this removal. It used to help delimit variables in the
> debugger, I'd prefer if it was kept around, but you decide.

This border is back, and I'm using a grey color with alpha so it looks OK / subtle on both themes.

Comment 26

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

Comment on attachment 8343499 [details] [diff] [review]
theme-variable-view.patch

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

Thank you. A few more comments below, but I'm very happy with the patch! I owe you a beverage of choice :)

::: browser/themes/linux/devtools/widgets.css
@@ +453,3 @@
>  }
>  
> +.variables-view-scope[changed],

.variables-view-scope didn't use to do anything special when getting the "changed" attribute, because scope "values" aren't a thing that can change (they're not variables or properties). This should be removed.

@@ +479,5 @@
>    -moz-margin-end: 1px;
>  }
>  
>  .variables-view-variable:not(:last-child) {
> +  border-bottom: 1px solid rgba(128, 128, 128, .3);

Nit: this is a little too opaque, making the border look much darker than before. A .15 transparency makes it look like before.

@@ +486,5 @@
>  .variables-view-variable > .title > .name {
>    font-weight: 600;
>  }
>  
> +.variables-view-container .variable-or-property:focus > .title > label {

Why the .variables-view-container class before? (.variable-or-property nodes are never child nodes of anything else).

To avoid the !important maybe? I'd prefer having the !important in that case.

::: browser/themes/shared/devtools/dark-theme.css
@@ +51,5 @@
>    background-color: rgba(0,0,0,0.5);
>  }
>  
> +.theme-bg-contrast,
> +.variables-view-scope[changed],

No need for .variables-view-scope[changed] (see other comment about scopes and [changed]).

@@ +103,5 @@
>  .cm-s-mozilla .cm-variable,
>  .cm-s-mozilla .cm-def,
>  .cm-s-mozilla .cm-property,
> +.cm-s-mozilla .cm-qualifier,
> +.variables-view-property > .title > .name,

I said in comment 21 that this needs to be
.variables-view-variable > .title > .name,

and for theme-fg-color3
.variables-view-property > .title > .name,

otherwise the variables and properties will have the same color.
(same thing in dark-theme.css)

@@ +112,5 @@
>  .theme-fg-color3,
>  .cm-s-mozilla .cm-builtin,
>  .cm-s-mozilla .cm-tag,
> +.cm-s-mozilla .cm-header,
> +.variable-or-property > .title > .name,

.variables-view-property > .title > .name,

@@ +138,5 @@
>  .theme-fg-color7,
>  .cm-s-mozilla .cm-atom,
>  .cm-s-mozilla .cm-quote,
> +.cm-s-mozilla .cm-error,
> +.variable-or-property .token-boolean,

Did you file a bug about making atoms and booleans more distinguishable from strings?

Comment 27

[Brian Grinstead [:bgrins]]

Attachment: theme-variable-view.patch

Addresses issues from Comment 26

Comment 28

[Brian Grinstead [:bgrins]]

(In reply to Victor Porof [:vp] from comment #26)
> Comment on attachment 8343499 [details] [diff] [review]
> theme-variable-view.patch
> 
> Review of attachment 8343499 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thank you. A few more comments below, but I'm very happy with the patch! I
> owe you a beverage of choice :)
> 
> ::: browser/themes/linux/devtools/widgets.css
> @@ +453,3 @@
> >  }
> >  
> > +.variables-view-scope[changed],
> 
> .variables-view-scope didn't use to do anything special when getting the
> "changed" attribute, because scope "values" aren't a thing that can change
> (they're not variables or properties). This should be removed.
> 

Removed.

> @@ +479,5 @@
> >    -moz-margin-end: 1px;
> >  }
> >  
> >  .variables-view-variable:not(:last-child) {
> > +  border-bottom: 1px solid rgba(128, 128, 128, .3);
> 
> Nit: this is a little too opaque, making the border look much darker than
> before. A .15 transparency makes it look like before.

Changed.

> 
> @@ +486,5 @@
> >  .variables-view-variable > .title > .name {
> >    font-weight: 600;
> >  }
> >  
> > +.variables-view-container .variable-or-property:focus > .title > label {
> 
> Why the .variables-view-container class before? (.variable-or-property nodes
> are never child nodes of anything else).
> 
> To avoid the !important maybe? I'd prefer having the !important in that case.
> 

Yes, this is why.  Switched to !important.

> 
> @@ +103,5 @@
> >  .cm-s-mozilla .cm-variable,
> >  .cm-s-mozilla .cm-def,
> >  .cm-s-mozilla .cm-property,
> > +.cm-s-mozilla .cm-qualifier,
> > +.variables-view-property > .title > .name,
> 
> I said in comment 21 that this needs to be
> .variables-view-variable > .title > .name,
> 
> and for theme-fg-color3
> .variables-view-property > .title > .name,
> 
> otherwise the variables and properties will have the same color.
> (same thing in dark-theme.css)
> 
> @@ +112,5 @@
> >  .theme-fg-color3,
> >  .cm-s-mozilla .cm-builtin,
> >  .cm-s-mozilla .cm-tag,
> > +.cm-s-mozilla .cm-header,
> > +.variable-or-property > .title > .name,
> 
> .variables-view-property > .title > .name,
> 
> @@ +138,5 @@
> >  .theme-fg-color7,
> >  .cm-s-mozilla .cm-atom,
> >  .cm-s-mozilla .cm-quote,
> > +.cm-s-mozilla .cm-error,
> > +.variable-or-property .token-boolean,
> 

Updated.

> Did you file a bug about making atoms and booleans more distinguishable from
> strings?

Opened Bug 947242 to handle switching over to the new colors.  If you look at https://developer.mozilla.org/en-US/docs/Tools/DevToolsColors, the corresponding highlight colors may be different enough.  I'll note that we should check this in that bug.

Comment 29

[Brian Grinstead [:bgrins]]

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

Comment 30

[Ryan VanderMeulen [:RyanVM]]

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