Closed Bug 966275 Opened 6 years ago Closed 6 years ago

DevTools Themes - Change color of main devtools splitter to better match both themes

Categories

(DevTools :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 29

People

(Reporter: bgrins, Assigned: bgrins)

References

Details

(Whiteboard: [good first verify])

Attachments

(3 files, 3 obsolete files)

It used to be black, now it is #aaa after Bug 957117.  Anything that has too much of a contrast looks weird on either theme, but we can't use different colors based on the theme, so we need to come up with a compromise.

#aaa looks great on all backgrounds for the light theme, but it looks quite bright on a dark background with the dark theme (see screenshot).
The light theme tab toolbar is #ebeced, and the dark theme is #252c33, so we want something that works against both of these colors.
#42484f is ideal on dark theme (it matches the border colors of the tabs), while #aaa is ideal on light theme (matches border colors of tabs).  The midpoint of these two colors is #76797D - it seems to look alright (screenshot and patch incoming)
Attached image tabbar-splitter-color.png (obsolete) —
Comparison of light/dark contrast with the splitter
Attached patch theme-splitter-color.patch (obsolete) — Splinter Review
What do you think?
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Attachment #8368573 - Flags: review?(vporof)
I think this is still much too light on a dark background. Why not just make the splitter invisible and add a colored border on the top of the tabbar?
Attachment #8368573 - Flags: review?(vporof)
(In reply to Victor Porof [:vp] from comment #5)
> I think this is still much too light on a dark background. Why not just make
> the splitter invisible and add a colored border on the top of the tabbar?

We could do this for the BottomHost, but it wouldn't work for the SidebarHost, and right now there is no way to target CSS specifically for the host type.  Maybe there is a way in toolbox-hosts.js to do this?  I can check...
(In reply to Brian Grinstead [:bgrins] from comment #6)

You could use the 700 media query. It'll work 95% of the time, and this is a pretty small UI nit for users to be really nitpicky.
(In reply to Victor Porof [:vp] from comment #7)
> (In reply to Brian Grinstead [:bgrins] from comment #6)
> 
> You could use the 700 media query. It'll work 95% of the time, and this is a
> pretty small UI nit for users to be really nitpicky.

I'm not a fan of this.  If it is wrong, then there will be no splitter visible and the content will be shifted in a weird way (there will be a border moved to the left of a bottom host, for instance).  Plus, I don't think it would be that uncommon to have it set to bottom host and < 700px if the browser was not taking up the full screen.

I'd say we should either pick a neutral color that works for both themes, or spend the time to properly notify the toolbox UI of which host it is in.  Honestly, I don't think the current color in the patch is that bad (especially when you see it next to the command icons, which appear just as light).  I'd be open to try out some different colors though.
What do you say about simply having a transparent splitter? :) (just a thought) Naturally users think they can resize the toolbox. No need for a necessarily colored line for this, if the cursor changes accordingly all is good.
Attached image neutral-with-alpha.png (obsolete) —
(In reply to Victor Porof [:vp] from comment #9)
> What do you say about simply having a transparent splitter? :) (just a
> thought) Naturally users think they can resize the toolbox. No need for a
> necessarily colored line for this, if the cursor changes accordingly all is
> good.

I tried transparent.. it looks a little weird then for light them / light bg and dark theme / dark bg.  What about if we do the midpoint color with alpha?  This screenshot shows rgba(118, 121, 125, .5)
Or we could
Attached image splitter-comparison.png
Here is an updated screenshot with the rgba(118, 121, 125, .5) splitter
Attachment #8368569 - Attachment is obsolete: true
Attachment #8368758 - Attachment is obsolete: true
I like this.
Updated the color for the devtools splitter
Attachment #8368573 - Attachment is obsolete: true
Attachment #8368808 - Flags: review?(vporof)
Comment on attachment 8368808 [details] [diff] [review]
theme-splitter-color.patch

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

::: browser/themes/shared/devtools/light-theme.css
@@ +307,5 @@
>    border-bottom: 0;
>  }
>  
> +.devtools-horizontal-splitter {
> +  border-bottom: 1px solid #aaa;

Shouldn't there be corresponding rules in the dark theme?
Attachment #8368808 - Flags: review?(vporof) → review+
(In reply to Victor Porof [:vp] from comment #14)
> Comment on attachment 8368808 [details] [diff] [review]
> theme-splitter-color.patch
> 
> Review of attachment 8368808 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/themes/shared/devtools/light-theme.css
> @@ +307,5 @@
> >    border-bottom: 0;
> >  }
> >  
> > +.devtools-horizontal-splitter {
> > +  border-bottom: 1px solid #aaa;
> 
> Shouldn't there be corresponding rules in the dark theme?

There already is for dark theme: https://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/devtools/dark-theme.css#311.  The light theme didn't yet have them since it was the same color for both.
https://hg.mozilla.org/mozilla-central/rev/7e577607c404
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 29
Whiteboard: [good first verify]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.