1094509 - DevEdition theme: switch to light devedition theme when light devtools theme is applied

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[Brian Grinstead [:bgrins]]

Following these steps:
* Have devedition theme applied (browser.devedition.theme.enabled = true)
* Open devtools
* Switch devtools theme to light

Currently in this case the Australis theme is applied.  To be more consistent, we should use the devedition styling but switch to the light devtools colors.

Comment 1

[Brian Grinstead [:bgrins]]

Attachment: toggle-light-devedition-WIP.patch

I'm sure this will need some UX love, but here is what I am thinking as far as implementation.  Try push: https://tbpl.mozilla.org/?tree=Try&rev=a7f196da4dee

Comment 2

[Joe Walker [:jwalker]]

In both the dark and light theme the bottom border of the current tab extends 2 pixels down into the URB bar. This stands out a lot more in the light theme due to the greater contrast between the bottom border of the current tab and the url bar background. I wonder if we should desaturate the bottom border of the current tab?

Comment 3

[:Gijs (he/him)]

Comment on attachment 8517818 [details] [diff] [review]
toggle-light-devedition-WIP.patch

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

r+ on the JS bits.

f+ on the code for the CSS bits. I've not done a visual review - you showed me a screenshot yesterday, and it looked fine at a glance; it'd be good to get this checked on our 3 primary OSes by some folks, as well as getting UX to look at styling.

Comments follow:

::: browser/base/content/test/general/browser_devedition.js
@@ +73,5 @@
>  
>    Services.prefs.setCharPref(PREF_DEVTOOLS_THEME, "dark");
>    is (document.documentElement.getAttribute("devtoolstheme"), "dark",
>      "The documentElement has an attribute based on devtools theme.");
> +  ok (DevEdition.styleSheet, "The devedition stylesheet is still there with the dark devtools theme.");

Not sure the last two pref switches and testing here add testing coverage? Seems we do the same thing twice now, might as well leave the second half out?

::: browser/themes/shared/devedition.inc.css
@@ +71,5 @@
>    --searchbar-dropmarker-2x-url: url("chrome://browser/skin/devtools/dropmarker.svg");
>  }
>  
> +:root[devtoolstheme="light"] {
> +  --space-above-tabbar: 1px;

Some of this can just remain the same, right, so it can be in a :root {} selector at the top?

@@ +127,5 @@
>    padding-left: 0;
>    padding-right: 0;
>  }
>  
>  #navigator-toolbox ::-moz-selection {

Eh, I missed where this was added.

I wonder if this will have annoying consequences for selection colors in panels and the like that get added as descendants of the toolbox?

@@ +258,5 @@
>    color: var(--tab-hover-color);
>  }
>  
>  .tabbrowser-tab[selected] {
> +  color: var(--tab-selection-color) !important;

Curious why you needed this !important? :-)

::: browser/themes/windows/devedition.css
@@ +9,5 @@
>  }
>  
>  #back-button > .toolbarbutton-icon,
>  #forward-button > .toolbarbutton-icon {
> +  background: var(--chrome-nav-buttons-background) !important;

Doesn't this need hover things as well? Does that just work?

Comment 4

[Brian Grinstead [:bgrins]]

Attachment: toggle-light-devedition.patch

(In reply to :Gijs Kruitbosch from comment #3)
> Comment on attachment 8517818 [details] [diff] [review]
> toggle-light-devedition-WIP.patch
> 
> Review of attachment 8517818 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ on the JS bits.
> 
> f+ on the code for the CSS bits. I've not done a visual review - you showed
> me a screenshot yesterday, and it looked fine at a glance; it'd be good to
> get this checked on our 3 primary OSes by some folks, as well as getting UX
> to look at styling.
> 

Thanks, I'm going to ask for feedback with the knowledge that we won't land anything until it goes through further UX testing, and we will go through another review at that point.  I haven't looked closer at comment 2 yet, for instance.  But I'm going to next test this with the changes from 1094138.

> Comments follow:
> 
> ::: browser/base/content/test/general/browser_devedition.js
> @@ +73,5 @@
> >  
> >    Services.prefs.setCharPref(PREF_DEVTOOLS_THEME, "dark");
> >    is (document.documentElement.getAttribute("devtoolstheme"), "dark",
> >      "The documentElement has an attribute based on devtools theme.");
> > +  ok (DevEdition.styleSheet, "The devedition stylesheet is still there with the dark devtools theme.");
> 
> Not sure the last two pref switches and testing here add testing coverage?
> Seems we do the same thing twice now, might as well leave the second half
> out?
> 

Removed

> ::: browser/themes/shared/devedition.inc.css
> @@ +71,5 @@
> >    --searchbar-dropmarker-2x-url: url("chrome://browser/skin/devtools/dropmarker.svg");
> >  }
> >  
> > +:root[devtoolstheme="light"] {
> > +  --space-above-tabbar: 1px;
> 
> Some of this can just remain the same, right, so it can be in a :root {}
> selector at the top?
> 

Yeah, I'll move that into a shared block.

> @@ +127,5 @@
> >    padding-left: 0;
> >    padding-right: 0;
> >  }
> >  
> >  #navigator-toolbox ::-moz-selection {
> 
> Eh, I missed where this was added.
> 
> I wonder if this will have annoying consequences for selection colors in
> panels and the like that get added as descendants of the toolbox?
> 

Should I make this more specific - targetting only the URL bar and search bar?  It's definitely needed for those, and I'm not sure if there are any other selectable bits up there.

> @@ +258,5 @@
> >    color: var(--tab-hover-color);
> >  }
> >  
> >  .tabbrowser-tab[selected] {
> > +  color: var(--tab-selection-color) !important;
> 
> Curious why you needed this !important? :-)
> 

It wasn't actually applying without it, but I should have done some more research about it.  Basically, there is a specific rule that tells this to inherit on OSX (http://dxr.mozilla.org/mozilla-central/source/browser/themes/osx/browser.css#3059), and in the dark lw theme, --chrome-color was the same as --tab-selection-color so I never noticed that it wasn't actually working.  I could alternatively update the selector to match: .tabbrowser-tab[selected=true]:not(:-moz-lwtheme).

> ::: browser/themes/windows/devedition.css
> @@ +9,5 @@
> >  }
> >  
> >  #back-button > .toolbarbutton-icon,
> >  #forward-button > .toolbarbutton-icon {
> > +  background: var(--chrome-nav-buttons-background) !important;
> 
> Doesn't this need hover things as well? Does that just work?

It just works based on http://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/devedition.inc.css#202.  I guess that is a little confusing though since the other OSes have special rules.

Comment 5

[:Gijs (he/him)]

Comment on attachment 8518299 [details] [diff] [review]
toggle-light-devedition.patch

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

(In reply to Brian Grinstead [:bgrins] from comment #4)
> Created attachment 8518299 [details] [diff] [review]
> toggle-light-devedition.patch
> > ::: browser/themes/shared/devedition.inc.css
> > @@ +71,5 @@
> > >    --searchbar-dropmarker-2x-url: url("chrome://browser/skin/devtools/dropmarker.svg");
> > >  }
> > >  
> > > +:root[devtoolstheme="light"] {
> > > +  --space-above-tabbar: 1px;
> > 
> > Some of this can just remain the same, right, so it can be in a :root {}
> > selector at the top?
> > 
> 
> Yeah, I'll move that into a shared block.

Was there nothing else?

> 
> > @@ +127,5 @@
> > >    padding-left: 0;
> > >    padding-right: 0;
> > >  }
> > >  
> > >  #navigator-toolbox ::-moz-selection {
> > 
> > Eh, I missed where this was added.
> > 
> > I wonder if this will have annoying consequences for selection colors in
> > panels and the like that get added as descendants of the toolbox?
> > 
> 
> Should I make this more specific - targetting only the URL bar and search
> bar?  It's definitely needed for those, and I'm not sure if there are any
> other selectable bits up there.

There aren't by default. There would be add-ons to consider, but then, I think you're only styling the url bar and search bar in terms of background color anyway, aren't you? In which case, this should use the same selector. Keep in mind those were updated by https://hg.mozilla.org/integration/fx-team/rev/4d284c7760bf .

> > @@ +258,5 @@
> > >    color: var(--tab-hover-color);
> > >  }
> > >  
> > >  .tabbrowser-tab[selected] {
> > > +  color: var(--tab-selection-color) !important;
> > 
> > Curious why you needed this !important? :-)
> > 
> 
> It wasn't actually applying without it, but I should have done some more
> research about it.  Basically, there is a specific rule that tells this to
> inherit on OSX
> (http://dxr.mozilla.org/mozilla-central/source/browser/themes/osx/browser.
> css#3059), and in the dark lw theme, --chrome-color was the same as
> --tab-selection-color so I never noticed that it wasn't actually working.  I
> could alternatively update the selector to match:
> .tabbrowser-tab[selected=true]:not(:-moz-lwtheme).

No, in that case this is probably clearer, but a comment would help.

> 
> > ::: browser/themes/windows/devedition.css
> > @@ +9,5 @@
> > >  }
> > >  
> > >  #back-button > .toolbarbutton-icon,
> > >  #forward-button > .toolbarbutton-icon {
> > > +  background: var(--chrome-nav-buttons-background) !important;
> > 
> > Doesn't this need hover things as well? Does that just work?
> 
> It just works based on
> http://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/
> devedition.inc.css#202.  I guess that is a little confusing though since the
> other OSes have special rules.

Er, wrong link? The quoted patch code here is about back/fwd buttons; the link is about the tab scroll buttons. Or am I missing something?

::: browser/themes/shared/devedition.inc.css
@@ +115,5 @@
> +
> +  /* Menu button separator */
> +  --panel-ui-button-background-image: linear-gradient(to bottom, rgba(0,0,0,0.1), rgba(0,0,0,0.1));
> +  --panel-ui-button-background-size: 1px calc(100% - 1px);
> +  --panel-ui-button-background-position: 1px 0px;

I wonder if this shouldn't be using the same styling as australis... but I guess this works, too.

FWIW, neither this or the dark theme do what australis does in having it not be a straight opaque line, meaning it looks odd if you enable the bookmarks toolbar (this is why we have the fade-out at the top and bottom). Might adjust that while we're here, if you feel like it? :-)

Comment 6

[Brian Grinstead [:bgrins]]

Attachment: toggle-light-devedition.patch

(In reply to :Gijs Kruitbosch from comment #5)
> Comment on attachment 8518299 [details] [diff] [review]
> toggle-light-devedition.patch
> 
> Review of attachment 8518299 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> (In reply to Brian Grinstead [:bgrins] from comment #4)
> > Created attachment 8518299 [details] [diff] [review]
> > toggle-light-devedition.patch
> > > ::: browser/themes/shared/devedition.inc.css
> > > @@ +71,5 @@
> > > >    --searchbar-dropmarker-2x-url: url("chrome://browser/skin/devtools/dropmarker.svg");
> > > >  }
> > > >  
> > > > +:root[devtoolstheme="light"] {
> > > > +  --space-above-tabbar: 1px;
> > > 
> > > Some of this can just remain the same, right, so it can be in a :root {}
> > > selector at the top?
> > > 
> > 
> > Yeah, I'll move that into a shared block.
> 
> Was there nothing else?
> 

Found a couple more things to add there.  Still not a ton shared though.

> > 
> > > @@ +127,5 @@
> > > >    padding-left: 0;
> > > >    padding-right: 0;
> > > >  }
> > > >  
> > > >  #navigator-toolbox ::-moz-selection {
> > > 
> > > Eh, I missed where this was added.
> > > 
> > > I wonder if this will have annoying consequences for selection colors in
> > > panels and the like that get added as descendants of the toolbox?
> > > 
> > 
> > Should I make this more specific - targetting only the URL bar and search
> > bar?  It's definitely needed for those, and I'm not sure if there are any
> > other selectable bits up there.
> 
> There aren't by default. There would be add-ons to consider, but then, I
> think you're only styling the url bar and search bar in terms of background
> color anyway, aren't you? In which case, this should use the same selector.
> Keep in mind those were updated by
> https://hg.mozilla.org/integration/fx-team/rev/4d284c7760bf .

Updated this to:

    #urlbar ::-moz-selection,
    #navigator-toolbox .searchbar-textbox ::-moz-selection
> 
> > > @@ +258,5 @@
> > > >    color: var(--tab-hover-color);
> > > >  }
> > > >  
> > > >  .tabbrowser-tab[selected] {
> > > > +  color: var(--tab-selection-color) !important;
> > > 
> > > Curious why you needed this !important? :-)
> > > 
> > 
> > It wasn't actually applying without it, but I should have done some more
> > research about it.  Basically, there is a specific rule that tells this to
> > inherit on OSX
> > (http://dxr.mozilla.org/mozilla-central/source/browser/themes/osx/browser.
> > css#3059), and in the dark lw theme, --chrome-color was the same as
> > --tab-selection-color so I never noticed that it wasn't actually working.  I
> > could alternatively update the selector to match:
> > .tabbrowser-tab[selected=true]:not(:-moz-lwtheme).
> 
> No, in that case this is probably clearer, but a comment would help.
> 

Added a comment

> > 
> > > ::: browser/themes/windows/devedition.css
> > > @@ +9,5 @@
> > > >  }
> > > >  
> > > >  #back-button > .toolbarbutton-icon,
> > > >  #forward-button > .toolbarbutton-icon {
> > > > +  background: var(--chrome-nav-buttons-background) !important;
> > > 
> > > Doesn't this need hover things as well? Does that just work?
> > 
> > It just works based on
> > http://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/
> > devedition.inc.css#202.  I guess that is a little confusing though since the
> > other OSes have special rules.
> 
> Er, wrong link? The quoted patch code here is about back/fwd buttons; the
> link is about the tab scroll buttons. Or am I missing something?
> 

Ah you are right, nice catch.  I've added that back.

It's the .tabbrowser-tab:hover in taht rule.

> ::: browser/themes/shared/devedition.inc.css
> @@ +115,5 @@
> > +
> > +  /* Menu button separator */
> > +  --panel-ui-button-background-image: linear-gradient(to bottom, rgba(0,0,0,0.1), rgba(0,0,0,0.1));
> > +  --panel-ui-button-background-size: 1px calc(100% - 1px);
> > +  --panel-ui-button-background-position: 1px 0px;
> 
> I wonder if this shouldn't be using the same styling as australis... but I
> guess this works, too.
> 
> FWIW, neither this or the dark theme do what australis does in having it not
> be a straight opaque line, meaning it looks odd if you enable the bookmarks
> toolbar (this is why we have the fade-out at the top and bottom). Might
> adjust that while we're here, if you feel like it? :-)

Done

Comment 7

[Brian Grinstead [:bgrins]]

Attachment: screenshot.png

A new screenshot - would love any ux feedback.  Here is a try build: https://tbpl.mozilla.org/?tree=Try&rev=3de54db1c22d

Comment 8

[Brian Grinstead [:bgrins]]

Attachment: toggle-light-devedition.patch

Rebased and also changed the box shadow on navbar from `0 -1px var(--chrome-navigator-toolbox-separator-color)` to `0 1px var(--chrome-nav-bar-separator-color)`.  After a discussion with MattN, I think that is what was causing the 'overlap' Joe is talking about in Comment 2

Comment 9

[Brian Grinstead [:bgrins]]

Latest try builds will be here: https://tbpl.mozilla.org/?tree=Try&rev=07d8344ac0ec.  These include the changes here and Gijs' changes to fix the customize button in 1094138.

Comment 10

[Jeff Griffiths (:canuckistani) (:⚡︎)]

Latest try looks pretty sweet, screenshot from my MBAir, OS X 10.10:

http://note.io/13Psu5Y

If you're reading this and you have a windows 8 machine, please post a screenshot.

Comment 11

[:Gijs (he/him)]

Comment on attachment 8518600 [details] [diff] [review]
toggle-light-devedition.patch

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

r+ for code, but some questions that should be dealt with before landing below:

::: browser/themes/shared/devedition.inc.css
@@ -85,5 @@
>  }
>  
> -.tabbrowser-arrowscrollbox > .arrowscrollbox-scrollbox {
> -  -moz-padding-end: 0;
> -  -moz-padding-start: 0;

Why was this removed?

::: browser/themes/windows/devedition.css
@@ +27,5 @@
> +
> +/* Override !important properties for hovered back button */
> +#main-window #back-button:hover:not([disabled="true"]) > .toolbarbutton-icon,
> +#main-window #forward-button:hover:not([disabled="true"]) > .toolbarbutton-icon {
> +  background: #1B2127 !important;

This should be a variable, shouldn't it? Right now it's the same for dark and light themes?

Also, presumably this doesn't need #main-window, because it's !important ?

Comment 12

[Joe Walker [:jwalker]]

Attachment: light-theme.png

Comment 13

[Brian Grinstead [:bgrins]]

Attachment: screenshot-ubuntu-restored.png

Comment 14

[Brian Grinstead [:bgrins]]

Attachment: screenshot-ubuntu-maximized.png

Comment 15

[Brian Grinstead [:bgrins]]

Attachment: toggle-light-devedition.patch

Updates test on top of 1094138, updates some colors after UX discussion, and includes a couple of linux fixes I went over in person with MattN

Comment 16

[Brian Grinstead [:bgrins]]

https://tbpl.mozilla.org/?tree=Try&rev=a9be2757827d

Comment 17

[Brian Grinstead [:bgrins]]

https://hg.mozilla.org/integration/fx-team/rev/1b2470364d58

Comment 18

[Brian Grinstead [:bgrins]]

Attachment: typo-fix.patch

Oops, had a typo in devedition.css causing css parsing errors.  Here's a fix

Comment 19

[Brian Grinstead [:bgrins]]

https://hg.mozilla.org/integration/fx-team/rev/3f54cbf8e85b

Comment 20

[Brian Grinstead [:bgrins]]

Comment on attachment 8519314 [details] [diff] [review]
typo-fix.patch

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

r=jwalker,a=KWierso

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2e401372dcf5
https://hg.mozilla.org/releases/mozilla-aurora/rev/c60e833d51cf

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1b2470364d58
https://hg.mozilla.org/mozilla-central/rev/3f54cbf8e85b