Closed Bug 969914 Opened 6 years ago Closed 5 years ago

Make developer toolbar match the light devtools theme when applied

Categories

(DevTools :: Framework, defect, P2)

defect

Tracking

(firefox40 fixed)

RESOLVED FIXED
Firefox 40
Tracking Status
firefox40 --- fixed

People

(Reporter: ntim, Assigned: bgrins)

References

Details

(Keywords: ux-consistency, Whiteboard: [polish-backlog][difficulty=easy])

Attachments

(4 files, 5 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:30.0) Gecko/20100101 Firefox/30.0 (Beta/Release)
Build ID: 20140207030201
Flags: needinfo?(bgrinstead)
Component: Untriaged → Developer Tools: Framework
Depends on: 966661
Blocks: 966661
No longer depends on: 966661
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/LightTheme-Inspector-Locked@2x.png and https://people.mozilla.org/~shorlander/mockups/devTools/ux-refresh-2013/DarkTheme-Inspector-Locked@2x.png.
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?
Flags: needinfo?(bgrinstead) → needinfo?(dhenein)
OS: Windows 8.1 → All
Hardware: x86_64 → All
Version: 30 Branch → Trunk
Keywords: ux-consistency
See Also: → 975675
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Depends on: 1053434
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 {

}
Clearing old needinfo
Flags: needinfo?(dhenein)
Summary: Make responsive mode, Developer toolbar and Inspector infobar match the light theme → Make developer toolbar and node infobar match the light devtools theme when applied
Assignee: nobody → ntim007
Status: NEW → ASSIGNED
Assignee: ntim007 → nobody
Status: ASSIGNED → NEW
Limiting the scope in the title to just developer toolbar
Status: NEW → ASSIGNED
Summary: Make developer toolbar and node infobar match the light devtools theme when applied → Make developer toolbar match the light devtools theme when applied
Status: ASSIGNED → NEW
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.
Flags: needinfo?(ntim007)
Attached patch WIP patch (obsolete) — Splinter Review
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.
Flags: needinfo?(ntim007)
Assignee: nobody → aljullu
Status: NEW → ASSIGNED
Whiteboard: [devedition-40][difficulty=easy]
Attached patch developer-toolbar-themes.patch (obsolete) — Splinter Review
This updates the theming for the toolbar itself, now just need to do the popups
Attachment #8524898 - Attachment is obsolete: true
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.
Assignee: aljullu → bgrinstead
Attachment #8590289 - Flags: review?(jwalker)
Priority: -- → P2
Attachment #8590289 - Flags: review?(jwalker) → review+
https://hg.mozilla.org/integration/fx-team/rev/57171901a6fa
Keywords: leave-open
Whiteboard: [devedition-40][difficulty=easy] → [fixed-in-fx-team][devedition-40][difficulty=easy]
Attachment #8590289 - Flags: checkin+
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/LightTheme-Inspector-Locked@2x.png
Attached patch developer-toolbar-themes.patch (obsolete) — Splinter Review
Also styling the popups now.  Still need to figure out how to test out the menu and gcli-row-out styling.
Attachment #8590284 - Attachment is obsolete: true
(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/
> LightTheme-Inspector-Locked@2x.png

Updated that in the latest patch
Attached patch developer-toolbar-themes.patch (obsolete) — Splinter Review
Tim, what do you think about the styling?
Attachment #8590415 - Attachment is obsolete: true
Attachment #8590466 - Flags: feedback?(ntim.bugs)
https://hg.mozilla.org/mozilla-central/rev/57171901a6fa
Whiteboard: [fixed-in-fx-team][devedition-40][difficulty=easy] → [devedition-40][difficulty=easy]
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.
Attachment #8590466 - Flags: review?(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);
Attachment #8590466 - Flags: review?(jwalker) → review+
Attached patch developer-toolbar-themes.patch (obsolete) — Splinter Review
Patrick, can you take a look at the styling changes here?  Joe's already reviewed the JS changes to developer toolbar.
Attachment #8591769 - Flags: review?(pbrosset)
Attachment #8590466 - Attachment is obsolete: true
Attachment #8590466 - Flags: feedback?(ntim.bugs)
- 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
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.
Attachment #8591769 - Flags: feedback+
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.
(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 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.
Attachment #8591769 - Flags: review?(pbrosset) → review+
(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.
(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
Added comments.  Pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b47a3a177ca3
Attachment #8591769 - Attachment is obsolete: true
Attachment #8592261 - Flags: review+
Whiteboard: [devedition-40][difficulty=easy] → [fixed-in-fx-team][devedition-40][difficulty=easy]
Attachment #8592261 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/848604d33fac
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team][devedition-40][difficulty=easy] → [devedition-40][difficulty=easy]
Target Milestone: --- → Firefox 40
Whiteboard: [devedition-40][difficulty=easy] → [polish-backlog][difficulty=easy]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.