Closed Bug 1205330 Opened 6 years ago Closed 6 years ago

review / revise the dark theme palette

Categories

(DevTools :: General, defect)

defect
Not set
normal

Tracking

(firefox43 affected, firefox49 fixed)

RESOLVED FIXED
Firefox 49
Tracking Status
firefox43 --- affected
firefox49 --- fixed

People

(Reporter: canuckistani, Assigned: hholmes)

References

(Depends on 1 open bug, )

Details

(Whiteboard: [devtools-ux])

Attachments

(2 files, 10 obsolete files)

"When you put pure black next to a set of meticulously picked colors, the black overpowers everything else. It stands out because it’s not natural. All of the “black” everyday objects around you have some amount of light bouncing off of them, which means they aren’t black, they’re dark gray. And that light probably has a tint to it, so they’re not even dark gray, they’re colored-dark gray."
Whiteboard: [devtools-ux]
Depends on: 1242709
See Also: → 1242709
Attached patch dark-refresh.patch (obsolete) — Splinter Review
This won't seem like an improvement without the changes implemented in bug 1246313, unfortunately.
Attached patch dark-refresh-markup-2.patch (obsolete) — Splinter Review
Patch with markup colors changed around too.
Attached patch starter-dark-refresh.patch (obsolete) — Splinter Review
rebased.
Attachment #8743514 - Attachment is obsolete: true
Attached patch dark-refresh-markup-2.patch (obsolete) — Splinter Review
Attachment #8743516 - Attachment is obsolete: true
(In reply to Helen V. Holmes (:helenvholmes) (:✨)(pls ni?) from comment #4)
> Created attachment 8743547 [details] [diff] [review]
> dark-refresh-markup-2.patch

skreenshotz plz kthxbai
Attached image such-dark-such-wow.png
I got chu Jeff
Attached patch dark-refresh.patch (obsolete) — Splinter Review
These colors probably don't look fabulous with our current syntax highlighting; bug 1246313 is filed for that.

Updated dev edition stuff too, although I'm sure I'm not covering all of my bases here...
Attachment #8743546 - Attachment is obsolete: true
Attachment #8743547 - Attachment is obsolete: true
Attachment #8745017 - Flags: review?(bgrinstead)
Comment on attachment 8745017 [details] [diff] [review]
dark-refresh.patch

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

We discussed, I think this patch is including some unrelated changes

::: alskdjfkdjsal.patch
@@ +1,1 @@
> +From 8b44d3d4f632f03415680c421accf53dc3cff490 Mon Sep 17 00:00:00 2001

Somehow this patch file got added, it should be removed
Attachment #8745017 - Flags: review?(bgrinstead)
Attached patch dark-refresh.patch (obsolete) — Splinter Review
Attachment #8745017 - Attachment is obsolete: true
Attachment #8745022 - Flags: review?(bgrinstead)
Comment on attachment 8745022 [details] [diff] [review]
dark-refresh.patch

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

Also see devtools/client/themes/commandline.css and devtools/client/themes/commandline.inc.css, which will need to be synced up with any changes here

::: devtools/client/themes/variables.css
@@ +35,4 @@
>    --theme-content-color3: #667380;
>  
>    --theme-highlight-green: #2cbb0f;
> +  --theme-highlight-blue: #3455db;

Looks like some light theme color changes here, should be removed, right?

@@ +61,4 @@
>  }
>  
>  :root.theme-dark {
> +  --theme-body-background: #393F4C;

Maybe it's just because I'm used to it, but I prefer how it looks when the main sections are (a) really close in color to the tabs and (b) are darker than the tabs (even just a touch darker like #21232b).

Up to you, I think the code changes look fine

@@ +70,3 @@
>    --theme-selection-background: #1d4f73;
>    --theme-selection-background-semitransparent: rgba(29, 79, 115, .5);
>    --theme-selection-color: #f5f7fa;

Can we also use this as a chance to make the splitter color not 'black'?  It would look better if it was softened up just a bit.  Fine if you want to do that in another bug
Attachment #8745022 - Flags: review?(bgrinstead)
Attached patch dark-refresh.patch (obsolete) — Splinter Review
(In reply to Brian Grinstead [:bgrins] from comment #10)
> Comment on attachment 8745022 [details] [diff] [review]
> dark-refresh.patch
> 
> Review of attachment 8745022 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Also see devtools/client/themes/commandline.css and
> devtools/client/themes/commandline.inc.css, which will need to be synced up
> with any changes here
Done!

> ::: devtools/client/themes/variables.css
> @@ +35,4 @@
> >    --theme-content-color3: #667380;
> >  
> >    --theme-highlight-green: #2cbb0f;
> > +  --theme-highlight-blue: #3455db;
> 
> Looks like some light theme color changes here, should be removed, right?
Welp. I'm terrible with version control.

> @@ +70,3 @@
> >    --theme-selection-background: #1d4f73;
> >    --theme-selection-background-semitransparent: rgba(29, 79, 115, .5);
> >    --theme-selection-color: #f5f7fa;
> 
> Can we also use this as a chance to make the splitter color not 'black'?  It
> would look better if it was softened up just a bit.  Fine if you want to do
> that in another bug
Agreed!
Attachment #8745022 - Attachment is obsolete: true
Attachment #8745142 - Flags: review?(bgrinstead)
Comment on attachment 8745142 [details] [diff] [review]
dark-refresh.patch

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

::: devtools/client/themes/variables.css
@@ +37,4 @@
>    --theme-highlight-green: #2cbb0f;
>    --theme-highlight-blue: #0088cc;
>    --theme-highlight-bluegrey: #0072ab;
> +  --theme-highlight-purple: #887ce6;

two more light theme changes: here

@@ +41,3 @@
>    --theme-highlight-lightorange: #d97e00;
>    --theme-highlight-orange: #f13c00;
> +  --theme-highlight-red: #e22f6f;

and here

@@ +71,2 @@
>    --theme-selection-background-semitransparent: rgba(29, 79, 115, .5);
> +  --theme-selection-color: white;

Honestly, this selection-color is one thing I'd avoid changing in this patch, since it's currently the same between both themes, and is hardcoded in various places: https://dxr.mozilla.org/mozilla-central/search?q=f5f7fa&redirect=true&case=false.  I think that should be cleaned up but if it were me working on it I'd rather do that in another bug.  If that's good with you, then can you roll back all changes of #f5f7fa?
Attachment #8745142 - Flags: review?(bgrinstead)
Attached patch dark-refresh.patch (obsolete) — Splinter Review
(In reply to Brian Grinstead [:bgrins] from comment #12)
> @@ +71,2 @@
> >    --theme-selection-background-semitransparent: rgba(29, 79, 115, .5);
> > +  --theme-selection-color: white;
> 
> Honestly, this selection-color is one thing I'd avoid changing in this
> patch, since it's currently the same between both themes, and is hardcoded
> in various places:
> https://dxr.mozilla.org/mozilla-central/
> search?q=f5f7fa&redirect=true&case=false.  I think that should be cleaned up
> but if it were me working on it I'd rather do that in another bug.  If
> that's good with you, then can you roll back all changes of #f5f7fa?
Agreed—I'll open a new bug, then!
Attachment #8745142 - Attachment is obsolete: true
Attachment #8746665 - Flags: review?(bgrinstead)
See Also: → 1268620
Comment on attachment 8746665 [details] [diff] [review]
dark-refresh.patch

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

Close!  Please update the commit message to encapsulate the changes in one line if possible

::: devtools/client/themes/commandline.inc.css
@@ +26,4 @@
>    --gcli-input-color: #b6babf; /* --theme-body-color-alt */
> +  --gcli-border-color: #454d5d; /* --theme-splitter-color */
> +  --selection-background: #4a90e2; /* --theme-selection-background */
> +  --selection-color: white; /* --theme-selection-color */

This change should be reverted

::: devtools/client/themes/variables.css
@@ +67,4 @@
>  
> +  --theme-tab-toolbar-background: #272b35;
> +  --theme-toolbar-background: #272b35;
> +  --theme-selection-background: #4a90e2;

This should also be udpated here: https://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/devedition.inc.css#48 (if it looks good with the DE theme)

@@ +87,5 @@
>    --theme-highlight-lightorange: #d99b28;
>    --theme-highlight-orange: #d96629;
>    --theme-highlight-red: #eb5368;
>    --theme-highlight-pink: #df80ff;
> +  --theme-highlight-gray: #e9f4fe;

New unused variable introduced here should be removed
Attachment #8746665 - Flags: review?(bgrinstead)
Attached patch dark-refresh.patch (obsolete) — Splinter Review
(In reply to Brian Grinstead [:bgrins] from comment #14)
> Comment on attachment 8746665 [details] [diff] [review]
> @@ +87,5 @@
> >    --theme-highlight-lightorange: #d99b28;
> >    --theme-highlight-orange: #d96629;
> >    --theme-highlight-red: #eb5368;
> >    --theme-highlight-pink: #df80ff;
> > +  --theme-highlight-gray: #e9f4fe;
> 
> New unused variable introduced here should be removed
I removed it per your request, but I could have sworn I read a comment about adding this variable across themes so it could be used safely. Was that maybe in Bug 1246313?
Attachment #8746665 - Attachment is obsolete: true
Attachment #8747243 - Flags: review?(bgrinstead)
(In reply to Helen V. Holmes (:helenvholmes) (:✨)(pls ni?) from comment #15)
> Created attachment 8747243 [details] [diff] [review]
> dark-refresh.patch
> 
> (In reply to Brian Grinstead [:bgrins] from comment #14)
> > Comment on attachment 8746665 [details] [diff] [review]
> > @@ +87,5 @@
> > >    --theme-highlight-lightorange: #d99b28;
> > >    --theme-highlight-orange: #d96629;
> > >    --theme-highlight-red: #eb5368;
> > >    --theme-highlight-pink: #df80ff;
> > > +  --theme-highlight-gray: #e9f4fe;
> > 
> > New unused variable introduced here should be removed
> I removed it per your request, but I could have sworn I read a comment about
> adding this variable across themes so it could be used safely. Was that
> maybe in Bug 1246313?

I think in one of the bugs it was added, but only in one theme, so I mentioned that we should have it in all of them.  But in the latest patches it looks like it's not used anywhere so I guess we can safely remove it and then add it in all themes when it's needed
Comment on attachment 8747243 [details] [diff] [review]
dark-refresh.patch

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

::: browser/themes/shared/devedition.inc.css
@@ +24,5 @@
>    --chrome-nav-buttons-background: #252C33;
>    --chrome-nav-buttons-hover-background: #1B2127;
>    --chrome-nav-bar-controls-border-color: #1D2328;
>    --chrome-selection-color: #fff;
> +  --chrome-selection-background-color: #4c9ed9;

This is the background for highlighted text (i.e. if you select the contents of the URL bar).  I don't know if this change was intentional but I'd consider reverting if not

@@ +31,5 @@
>    --tabs-toolbar-color: #F5F7FA;
>    --tab-background-color: #1C2126;
>    --tab-hover-background-color: #07090a;
>    --tab-selection-color: #f5f7fa;
> +  --tab-selection-background-color: #3B72B3;

This doesn't match with --theme-selection-background (i.e. the selected devtools tab).  I'm not sure if that's intentional, but I'd consider reverting as I mention in variables.css - up to you though.

::: devtools/client/themes/toolbars.css
@@ +393,4 @@
>    border: 1px solid;
>    border-radius: 2px;
>    padding: 4px 6px;
> +  border-color: var(--theme-tab-toolbar-background);

I don't think this change is meant to be - I tested this out locally and it makes the borders disappear on the text boxes.  This should be reverted, yes?

::: devtools/client/themes/variables.css
@@ +67,4 @@
>  
> +  --theme-tab-toolbar-background: #272b35;
> +  --theme-toolbar-background: #272b35;
> +  --theme-selection-background: #4a90e2;

I don't know about this change - it brings the selected tab out of sync with Dev Edition tabs and also things like the console filter buttons.  I'd consider reverting, or bringing it a bit closer to the current color so it doesn't require changes to individual tools like the console.  If you change it, please make sure to change all other instances of it in the patch (and also the note I made in devedition.inc.css)
Attachment #8747243 - Flags: review?(bgrinstead)
Attached patch dark-refresh.patch (obsolete) — Splinter Review
(In reply to Brian Grinstead [:bgrins] from comment #17)
> Comment on attachment 8747243 [details] [diff] [review]
> dark-refresh.patch
> 
> Review of attachment 8747243 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/themes/shared/devedition.inc.css
> @@ +24,5 @@
> >    --chrome-nav-buttons-background: #252C33;
> >    --chrome-nav-buttons-hover-background: #1B2127;
> >    --chrome-nav-bar-controls-border-color: #1D2328;
> >    --chrome-selection-color: #fff;
> > +  --chrome-selection-background-color: #4c9ed9;
> 
> This is the background for highlighted text (i.e. if you select the contents
> of the URL bar).  I don't know if this change was intentional but I'd
> consider reverting if not
I don't think it was intentional the first time, so I've changed it knowing what it is this time :|

> ::: devtools/client/themes/toolbars.css
> @@ +393,4 @@
> >    border: 1px solid;
> >    border-radius: 2px;
> >    padding: 4px 6px;
> > +  border-color: var(--theme-tab-toolbar-background);
> 
> I don't think this change is meant to be - I tested this out locally and it
> makes the borders disappear on the text boxes.  This should be reverted, yes?
I've swapped this out with a slightly lighter color—whoops! I don't think the change was reverted, exactly, but it's been changed.

> ::: devtools/client/themes/variables.css
> @@ +67,4 @@
> >  
> > +  --theme-tab-toolbar-background: #272b35;
> > +  --theme-toolbar-background: #272b35;
> > +  --theme-selection-background: #4a90e2;
> 
> I don't know about this change - it brings the selected tab out of sync with
> Dev Edition tabs and also things like the console filter buttons.  I'd
> consider reverting, or bringing it a bit closer to the current color so it
> doesn't require changes to individual tools like the console.  If you change
> it, please make sure to change all other instances of it in the patch (and
> also the note I made in devedition.inc.css)
So, I didn't revert the change, but I /did/ change the buttons in the console—with the new, bright background for selected tabs, it looked strange to carry that blue color over into the console. (Felt like everything was screaming at me.)

Take it for a whirl; I wonder if maybe the sub-navs (e.g., "Rules") might need to be decked down a notch too. I wasn't a huge fan of the dark blue we were using before so I want to change it /somehow/—anyway, let me know how you feel about it.
Attachment #8747243 - Attachment is obsolete: true
Attachment #8748331 - Flags: review?(bgrinstead)
Comment on attachment 8748331 [details] [diff] [review]
dark-refresh.patch

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

So, looks good to me.  But, pulling in the selection colors has opened up a new can of worms (doesn't look like too much work though).  All of these instances should be replaced with the new rgb value: https://dxr.mozilla.org/mozilla-central/search?q=rgba(29%2C+79%2C+115&redirect=true&case=false

::: devtools/client/themes/toolbars.css
@@ +346,4 @@
>  .theme-dark .devtools-toolbarbutton:not([disabled])[label][open],
>  .theme-dark .devtools-button:not(:empty)[checked=true],
>  .theme-dark #toolbox-buttons .devtools-toolbarbutton[text-as-image][checked=true] {
> +  background: #5675B9; /* --theme-selection-background */

In this case you could actually use `var(--theme-selection-background)` since there's no alpha channel.  Although I think it looks a little nicer with something like:

rgba(86, 117, 185, 0.7) or even better for reuse: var(--theme-selection-background-semitransparent)

::: devtools/client/themes/variables.css
@@ +71,1 @@
>    --theme-selection-background-semitransparent: rgba(29, 79, 115, .5);

This color should change to the new RGB values rgba(86, 117, 185, 0.5)
Attachment #8748331 - Flags: review?(bgrinstead)
(In reply to Brian Grinstead [:bgrins] from comment #19)
> Comment on attachment 8748331 [details] [diff] [review]
> So, looks good to me.  But, pulling in the selection colors has opened up a
> new can of worms (doesn't look like too much work though).  All of these
> instances should be replaced with the new rgb value:
> https://dxr.mozilla.org/mozilla-central/
> search?q=rgba(29%2C+79%2C+115&redirect=true&case=false
Ah, good catch, thank you!

> In this case you could actually use `var(--theme-selection-background)`
> since there's no alpha channel.  Although I think it looks a little nicer
> with something like:
> 
> rgba(86, 117, 185, 0.7) or even better for reuse:
> var(--theme-selection-background-semitransparent)
I have made this variable, but because the alpha channels across memory/performance/debugger for this color are all different values, I'm not sure how useful it is without being able to pass the alpha as a parameter. We need mixins, Brian! Sass FTW! :P
Attachment #8748331 - Attachment is obsolete: true
Attachment #8748690 - Flags: review?(bgrinstead)
Comment on attachment 8748690 [details] [diff] [review]
dark-refresh.patch

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

LGTM!
Attachment #8748690 - Flags: review?(bgrinstead) → review+
Helen, I went ahead and updated it before landing but note I got this error when pushing:

remote: ************************** ERROR ****************************
remote: Rev 114778de4376 contains git-format-patch "[PATCH]" cruft. Use git-format-patch -k to avoid this.
remote: "Helen V. Holmes" <helen.v.holmes@gmail.com>
remote: [PATCH] Bug 1205330: Revisions to dark theme, r=bgrins
remote: From db590eae33c87336343dfe73627394ec2e72665d Mon Sep 17 00:00:00 2001
remote: ---
remote:  browser/themes/shared/devedition.inc.css   | 15 ++++++---------
remote:  devtools/client/themes/commandline.css     |  6 +++---
remote:  devtools/client/themes/commandline.inc.css | 10 +++++-----
remote:  devtools/client/themes/debugger.css        |  6 +++---
remote:  devtools/client/themes/memory.css          |  4 ++--
remote:  devtools/client/themes/performance.css     |  4 ++--
remote:  devtools/client/themes/toolbars.css        | 12 +++---------
remote:  devtools/client/themes/variables.css       | 22 +++++++++++-----------
remote:  8 files changed, 35 insertions(+), 44 deletions(-)
remote: 
remote: MozReview-Commit-ID: 7gOuqTPBFcf
remote: *************************************************************
remote: 
remote: 
remote: transaction abort!
remote: rollback completed
https://hg.mozilla.org/mozilla-central/rev/028990b642eb
https://hg.mozilla.org/mozilla-central/rev/680574fb36ff
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Depends on: 1270990
--chrome-selection-background-color: #5675B9;
--tab-selection-background-color: #5675B9;
please do not change this tab color, i like #1A4666
Flags: needinfo?(hholmes)
--chrome-selection-background-color: #5675B9;
--tab-selection-background-color: #5675B9;
please do not change this tab color, i like #1A4666
Flags: needinfo?(hholmes)
Depends on: 1273344
Successfully reproduce this bug on Nightly 43.0a1 (2015-09-16) (Build ID: 20150916030203) on Linux,

This Bug's Fix is now verified on Latest Firefox Beta 49.0b3

Build ID: 20160811031722
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:49.0) Gecko/20100101 Firefox/49.0
OS: Linux 4.4.0-2-deepin-amd64
QA Whiteboard: [testday-20160812]
Depends on: 1327976
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.