969914 - Make developer toolbar match the light devtools theme when applied

Status: RESOLVED FIXED

(DevTools :: Framework, defect, P2)

Description

[Tim Nguyen :ntim]

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:30.0) Gecko/20100101 Firefox/30.0 (Beta/Release)
Build ID: 20140207030201

Comment 1

[Brian Grinstead [:bgrins]]

All of these are being loaded in the browser.xul frame, which isn't aware of the currently applied DevTools theme.

1) Responsive mode is kind of its own thing, and I don't think it really looks bad next to the light theme.
2) I don't think the highlighted node infobar needs to match the current theme completely, since it is overlaid on web content.  We could consider using some single color across both themes, especially since the infobar is now significantly more simple than it was when the mockups were made.  Comparing the dark and light theme mockups, though, they are using matching colors: https://people.mozilla.org/~shorlander/mockups/devTools/ux-refresh-2013/[e-mail] and https://people.mozilla.org/~shorlander/mockups/devTools/ux-refresh-2013/[e-mail].
3) The developer toolbar is the most visually jarring of the bunch, though it is on track to be more integrated with the toolbox and the web console (Bug 911456 is set up for tracking this progress).  Depending on how quickly that progresses it is probably something that could be left (since it will automatically be resolved at that point).  If we have to come up with some workaround for (2) it probably wouldn't going to be too big of a deal to restyle this though.

If needed, we could notify the browser.xul page of the currently applied devtools theme, and change the styling based on that.  I've avoided this as it gets a bit complicated for situations when the toolbox has not yet been loaded (like when the developer toolbar is opened before the rest of devtools).

Pinging darrin for some feedback on points 1 and 2.  Should we be making responsive mode / node infobar match currently used theme?

Comment 2

[Brian Grinstead [:bgrins]]

Talked with Joe about possibly just duplicating relevant themeable rules for developer toolbar in commandline.inc.css, then listening for a perf change and applying 'theme-light'/'theme-dark' class to the #developer-toolbar element in browser.xul.  This could also include the tooltips for Bug 975675.

Comment 3

[Brian Grinstead [:bgrins]]

Now we have a selector that can be used that has landed in Bug 1053434.  I would suggest the first target(s) for styling should be the Developer Toolbar and the node infobar.  Responsive mode is in-content so it could be done later (if at all).

/* Style an element differently if the light devtools theme is applied */
:root[devtoolstheme="light"] .foo {

}

Comment 4

[Brian Grinstead [:bgrins]]

Clearing old needinfo

Comment 5

[Brian Grinstead [:bgrins]]

Limiting the scope in the title to just developer toolbar

Comment 6

[Brian Grinstead [:bgrins]]

Tim, did you have a WIP patch from when you were working on this?  I know we had discussed a few options on the colors used, so just want to make sure we don't duplicate effort.

Comment 7

[Tim Nguyen :ntim]

Attachment: WIP patch

There you go. This only changes the colors for now. But for some reason, I couldn't seem to get -moz-border-end to work properly. I also couldn't get the CSS filter to apply.

Comment 8

[Brian Grinstead [:bgrins]]

Attachment: developer-toolbar-themes.patch

This updates the theming for the toolbar itself, now just need to do the popups

Comment 9

[Brian Grinstead [:bgrins]]

Attachment: gcli-theme-move-to-shared.patch

These files were identical, so I did an hg cp over to a shared file and then deleted the os-specific ones.  This will make theming it easier.

Comment 10

[Brian Grinstead [:bgrins]]

https://hg.mozilla.org/integration/fx-team/rev/57171901a6fa

Comment 11

[Tim Nguyen :ntim]

Comment on attachment 8590284 [details] [diff] [review]
developer-toolbar-themes.patch

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

::: browser/themes/shared/devtools/commandline.inc.css
@@ +191,5 @@
>    background-position: 0 center;
>    background-size: 32px 16px;
>  }
>  
> +/* Light theme always has the blue icon.  Dark theme only has it when focused. */

Stephen's mockup seems to show a grey icon by default : https://people.mozilla.org/~shorlander/mockups/devTools/ux-refresh-2013/[e-mail]

Comment 12

[Brian Grinstead [:bgrins]]

Attachment: developer-toolbar-themes.patch

Also styling the popups now.  Still need to figure out how to test out the menu and gcli-row-out styling.

Comment 13

[Brian Grinstead [:bgrins]]

(In reply to Tim Nguyen [:ntim] from comment #11)
> Stephen's mockup seems to show a grey icon by default :
> https://people.mozilla.org/~shorlander/mockups/devTools/ux-refresh-2013/
> [e-mail]

Updated that in the latest patch

Comment 14

[Brian Grinstead [:bgrins]]

Attachment: developer-toolbar-themes.patch

Tim, what do you think about the styling?

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/57171901a6fa

Comment 16

[Brian Grinstead [:bgrins]]

Comment on attachment 8590466 [details] [diff] [review]
developer-toolbar-themes.patch

Joe, you are the best person to review the code/test changes.  Feel free to take a look at the styling changes also but I can get another reviewer for that if not.

Comment 17

[Brian Grinstead [:bgrins]]

Try push for second patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=12ced6673708

Comment 18

[Joe Walker [:jwalker]]

Comment on attachment 8590466 [details] [diff] [review]
developer-toolbar-themes.patch

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

r+ for the developer toolbar changes.

::: browser/devtools/shared/DeveloperToolbar.jsm
@@ +858,5 @@
> +  if (this.document) {
> +    this.document.documentElement.setAttribute(
> +      "devtoolstheme",
> +      this._devtoolbar._doc.documentElement.getAttribute("devtoolstheme")
> +    );

Wouldn't it be clearer to extract the value into a variable

let theme = ...getAttribute(...);
...setAttribute(..., theme);

Comment 19

[Brian Grinstead [:bgrins]]

Attachment: developer-toolbar-themes.patch

Patrick, can you take a look at the styling changes here?  Joe's already reviewed the JS changes to developer toolbar.

Comment 20

[Tim Nguyen :ntim]

Attachment: light-theme-dev-toolbar.png

- Tooltip shadow
I would probably just go with the shadow we use for the devtools autocomplete popup.

- Lighter background for the input
I'd probably go with a lighter background for the input, since gray usually means disabled.
Here are the colors I suggest :
 - Default #F7F7F7 (sidebar background)
 - Focus #FCFCFC (body background)

- Strange round corner
This was there before, but it's much more visible with the theme changes, can we remove those rounded corners ?

Dark theme feedback coming next

Comment 21

[Tim Nguyen :ntim]

Attachment: dark-theme-devtoolbar.png

See the light theme comments for the shadow and the round corner.

The dark theme mostly look goods to me. This is probably my taste, but I would probably tweak the default input state to have a slightly bit darker background.

Comment 22

[Tim Nguyen :ntim]

Comment on attachment 8591769 [details] [diff] [review]
developer-toolbar-themes.patch

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

The commandline.css and commandline.inc.css files should probably be renamed later so people clearly understand what they are for.

Comment 23

[Brian Grinstead [:bgrins]]

(In reply to Tim Nguyen [:ntim] from comment #21)
> Created attachment 8591789 [details]
> dark-theme-devtoolbar.png
> 
> See the light theme comments for the shadow and the round corner.

Thanks for the feedback!  I've now set border-radius: 0 on .gclitoolbar-input-node.  The shadow is trickier, I believe it's part of the native panel popup styling (as it can also be seen on things like the awesome bar, colorpicker popup, etc).  I can't find anything we are doing to style that popup differently, so unless if I'm missing something I'm inclined to leave it alone.

Comment 24

[Patrick Brosset <:pbro>]

Comment on attachment 8591769 [details] [diff] [review]
developer-toolbar-themes.patch

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

::: browser/themes/shared/devtools/commandline.css
@@ +3,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +/* Note: We are copy/pasting variables from light-theme and dark-theme,
> +   since they aren't loaded in this context (within commandlineoutput.xhtml
> +   and commandlinetooltip.xhtml). */

I think it's worth adding to this comment that it's important for anyone changing theme colors that they keep light/dark-theme and this file in sync.
In fact, adding a comment in light/dark-theme.css about it would help too.

::: browser/themes/shared/devtools/commandline.inc.css
@@ +6,5 @@
>  
>  /* Developer toolbar */
>  
> +/* Note: We are copy/pasting variables from light-theme and dark-theme,
> +   since they aren't loaded in this context (within browser.css). */

Same remark here.

@@ +14,5 @@
> +  --gcli-input-focused-background: #f7f7f7; /* --theme-sidebar-background */
> +  --gcli-input-color: #18191a; /* --theme-body-color */
> +  --gcli-border-color: #aaaaaa; /* --theme-splitter-color */
> +  --selection-background: #4c9ed9; /* --theme-selection-background */
> +  --selection-color: #f5f7fa; /* --theme-selection-color */

Also, random idea: wouldn't it be possible to extract all of our variables into a new file, that would contain only variables, and load it everywhere we need it to be loaded? in browser.css, in commandline.css, and in light/dark-theme.css?

I don't think this duplication hurts too much if we have enough comments to let developers and reviewers know they should keep an eye on this, and because theme color changes aren't too frequent, but still, it would be cleaner.

Comment 25

[Brian Grinstead [:bgrins]]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #24)
> Comment on attachment 8591769 [details] [diff] [review]
> developer-toolbar-themes.patch
> 
> Review of attachment 8591769 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/themes/shared/devtools/commandline.css
> @@ +3,5 @@
> >   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> >  
> > +/* Note: We are copy/pasting variables from light-theme and dark-theme,
> > +   since they aren't loaded in this context (within commandlineoutput.xhtml
> > +   and commandlinetooltip.xhtml). */
> 
> I think it's worth adding to this comment that it's important for anyone
> changing theme colors that they keep light/dark-theme and this file in sync.
> In fact, adding a comment in light/dark-theme.css about it would help too.
> 
> ::: browser/themes/shared/devtools/commandline.inc.css
> @@ +6,5 @@
> >  
> >  /* Developer toolbar */
> >  
> > +/* Note: We are copy/pasting variables from light-theme and dark-theme,
> > +   since they aren't loaded in this context (within browser.css). */
> 
> Same remark here.
> 

OK, I'll update those comments.

> @@ +14,5 @@
> > +  --gcli-input-focused-background: #f7f7f7; /* --theme-sidebar-background */
> > +  --gcli-input-color: #18191a; /* --theme-body-color */
> > +  --gcli-border-color: #aaaaaa; /* --theme-splitter-color */
> > +  --selection-background: #4c9ed9; /* --theme-selection-background */
> > +  --selection-color: #f5f7fa; /* --theme-selection-color */
> 
> Also, random idea: wouldn't it be possible to extract all of our variables
> into a new file, that would contain only variables, and load it everywhere
> we need it to be loaded? in browser.css, in commandline.css, and in
> light/dark-theme.css?
> 
> I don't think this duplication hurts too much if we have enough comments to
> let developers and reviewers know they should keep an eye on this, and
> because theme color changes aren't too frequent, but still, it would be
> cleaner.

It'd be nice to have as little duplication as possible, and it may really make sense to do something like this at some point.  But for now splitting it up and including all of our variables into browser.css (which is loaded in the main browser.xul file) is more than I want to take on here.

What would help leave this as an option in the future is if we renamed the --gcli-background-color, --gcli-input-background, etc to match the devtools theme variable names that they are corresponding to.  Then if we do swap it out with an included file we won't need any changes to this file.

Comment 26

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #25)
> (In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #24)
> 
> > @@ +14,5 @@
> > > +  --gcli-input-focused-background: #f7f7f7; /* --theme-sidebar-background */
> > > +  --gcli-input-color: #18191a; /* --theme-body-color */
> > > +  --gcli-border-color: #aaaaaa; /* --theme-splitter-color */
> > > +  --selection-background: #4c9ed9; /* --theme-selection-background */
> > > +  --selection-color: #f5f7fa; /* --theme-selection-color */
> > 
> > Also, random idea: wouldn't it be possible to extract all of our variables
> > into a new file, that would contain only variables, and load it everywhere
> > we need it to be loaded? in browser.css, in commandline.css, and in
> > light/dark-theme.css?
> > 
> > I don't think this duplication hurts too much if we have enough comments to
> > let developers and reviewers know they should keep an eye on this, and
> > because theme color changes aren't too frequent, but still, it would be
> > cleaner.
> 
> It'd be nice to have as little duplication as possible, and it may really
> make sense to do something like this at some point.  But for now splitting
> it up and including all of our variables into browser.css (which is loaded
> in the main browser.xul file) is more than I want to take on here.
> 
> What would help leave this as an option in the future is if we renamed the
> --gcli-background-color, --gcli-input-background, etc to match the devtools
> theme variable names that they are corresponding to.  Then if we do swap it
> out with an included file we won't need any changes to this file.

.. Which I just realized won't work for everything, because we are mapping different variables from the light and dark theme to a single variable here

Comment 27

[Brian Grinstead [:bgrins]]

Attachment: developer-toolbar-themes.patch

Added comments.  Pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b47a3a177ca3

Comment 28

[Brian Grinstead [:bgrins]]

remote:   https://hg.mozilla.org/integration/fx-team/rev/848604d33fac

Comment 29

[Ekanan Ketunuti]

https://hg.mozilla.org/mozilla-central/rev/848604d33fac