Closed Bug 1094509 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox :: Theme, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 36
Tracking Status
firefox35 --- fixed
firefox36 --- fixed

People

(Reporter: bgrins, Assigned: bgrins)

References

Details

Attachments

(6 files, 4 obsolete files)

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.
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
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
Attachment #8517818 - Flags: feedback?(gijskruitbosch+bugs)
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 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?
Attachment #8517818 - Flags: feedback?(gijskruitbosch+bugs) → feedback+
Blocks: 1094138
Attached patch toggle-light-devedition.patch (obsolete) — Splinter Review
(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.
Attachment #8517818 - Attachment is obsolete: true
Attachment #8518299 - Flags: feedback?(gijskruitbosch+bugs)
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? :-)
Attachment #8518299 - Flags: feedback?(gijskruitbosch+bugs) → feedback+
Attached patch toggle-light-devedition.patch (obsolete) — Splinter Review
(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
Attachment #8518299 - Attachment is obsolete: true
Attachment #8518515 - Flags: review?(gijskruitbosch+bugs)
Attached image screenshot.png
A new screenshot - would love any ux feedback.  Here is a try build: https://tbpl.mozilla.org/?tree=Try&rev=3de54db1c22d
Attachment #8518525 - Flags: ui-review?(shorlander)
Attachment #8518525 - Flags: ui-review?(madhava)
Attached patch toggle-light-devedition.patch (obsolete) — Splinter Review
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
Attachment #8518600 - Flags: review?(gijskruitbosch+bugs)
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.
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.
Attachment #8518515 - Flags: review?(gijskruitbosch+bugs)
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 ?
Attachment #8518600 - Flags: review?(gijskruitbosch+bugs) → review+
Attached image light-theme.png
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
Attachment #8518515 - Attachment is obsolete: true
Attachment #8518600 - Attachment is obsolete: true
Attachment #8519179 - Flags: review+
Attached patch typo-fix.patchSplinter Review
Oops, had a typo in devedition.css causing css parsing errors.  Here's a fix
Attachment #8519314 - Flags: review?(jwalker)
Comment on attachment 8519314 [details] [diff] [review]
typo-fix.patch

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

r=jwalker,a=KWierso
Attachment #8519314 - Flags: review?(jwalker) → review+
https://hg.mozilla.org/mozilla-central/rev/1b2470364d58
https://hg.mozilla.org/mozilla-central/rev/3f54cbf8e85b
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
Group: mozilla-employee-confidential
Attachment #8518525 - Flags: ui-review?(shorlander)
Attachment #8518525 - Flags: ui-review?(madhava)
You need to log in before you can comment on or make changes to this bug.