1399498 - [Windows 7] Make Background Tabs Dark for Default Theme

Status: VERIFIED FIXED

(Firefox :: Theme, enhancement, P1)

Description

[Stephen Horlander [:shorlander]]

Background tabs should be dark and be in a solid box not floating on the glass fog background:

http://design.firefox.com/people/shorlander/photon/Mockups/windows-7.html

Comment 1

[Johann Hofmann [:johannh]]

Attachment: Bug 1399498 - Make chrome background show in tabs on Windows 7 and remove glass fog on default theme.

This change is part of the original Photon spec for Windows 7.

Review commit: https://reviewboard.mozilla.org/r/180866/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/180866/

Comment 2

[Johann Hofmann [:johannh]]

Dão, this patch uses elements from the changes I made in bug 1391227 and bug 1398125, but it applies cleanly on central without those patches (it will have merge conflicts with those patches, in fact).

It would probably be more practical to review the other two patches before this one, so that I can rebase and we have a more clear changelog, but I'd leave that up to you.

I'm relatively certain that this should work well on Windows 8, but I'll try it myself later.

Comment 3

[Johann Hofmann [:johannh]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=908cb7a0399099b04fff25b6019934ec2e4c0343

Comment 4

[Dão Gottwald [:dao]]

Comment on attachment 8909276 [details]
Bug 1399498 - Make chrome background show in tabs on Windows 7 and remove glass fog on default theme.

https://reviewboard.mozilla.org/r/180866/#review186388

::: browser/themes/windows/browser.css:112
(Diff revision 1)
> + * Windows 7 draws the chrome background color as the tab background
> + * instead of in the tabs toolbar.
> + */
> +@media (-moz-os-version: windows-win7) {
> +  :root:not(:-moz-lwtheme) {
> +    --chrome-background-color: hsl(235,33%,19%);

This variable is currently used only in compacttheme.css (for good reasons; the name is confusing). Can we keep it there?

::: browser/themes/windows/browser.css:120
(Diff revision 1)
> +
> +  #TabsToolbar:not(:-moz-lwtheme) {
> +    color: hsl(240,9%,98%);
> +  }
> +
> +  #tabbrowser-tabs > .tabbrowser-tab {

Does this need :not(:-moz-lwtheme)?

::: browser/themes/windows/browser.css:131
(Diff revision 1)
> +    border-image: none !important;
> +  }
> +
> +  #tabbrowser-tabs > .tabbrowser-tab > .tab-stack > .tab-background {
> +    border-image: none !important;
> +    border-top: 1px solid var(--tabs-border);

Please document what you're doing here

::: browser/themes/windows/browser.css:135
(Diff revision 1)
> +    border-image: none !important;
> +    border-top: 1px solid var(--tabs-border);
> +  }
> +
> +  #tabbrowser-tabs > .tabbrowser-tab[first-tab="true"] > .tab-stack > .tab-background {
> +    border-inline-start: 1px solid var(--tabs-border) !important;

This doesn't seem right for scrolling tabs. I need to think a bit more about this, also in light of bug 1390221 and bug 1390025. Can you remove it from this patch and file a new bug if it isn't already covered by an existing bug?

::: browser/themes/windows/compacttheme.css:38
(Diff revision 1)
>  @media (-moz-windows-glass) {
> -  .tabbrowser-tab {
> -    background-color: var(--chrome-background-color);
> +  /* Always show light toolbar buttons on aero glass surface. */
> +  #main-window #TabsToolbar .toolbarbutton-1,
> +  #main-window #TabsToolbar .scrollbutton-up,
> +  #main-window #TabsToolbar .scrollbutton-down {
> +    fill: hsl(240,9%,98%) !important;

If you remove :not(:-moz-lwtheme) from the #TabsToolbar:not(:-moz-lwtheme) rule you added in browser.css, this shouldn't be needed.

::: browser/themes/windows/compacttheme.css:42
(Diff revision 1)
> +  #main-window #TabsToolbar .scrollbutton-down {
> +    fill: hsl(240,9%,98%) !important;
>    }
> +
> +  #toolbar-menubar {
> +    text-shadow: 0 0 .5em white, 0 0 .5em white, 0 1px 0 rgba(255,255,255,.4);

Why is this in compacttheme.css?

Comment 5

[Johann Hofmann [:johannh]]

Comment on attachment 8909276 [details]
Bug 1399498 - Make chrome background show in tabs on Windows 7 and remove glass fog on default theme.

https://reviewboard.mozilla.org/r/180866/#review186388

> This variable is currently used only in compacttheme.css (for good reasons; the name is confusing). Can we keep it there?

I don't think so, it does exactly what we need here (have a way for default and light/dark themes to set the tab background). I can rename it to something more clear if you prefer. Alternatively we could also not use a variable at all and just add the rule in line 120 to compacttheme.css. Glancing at your second comment it seems like you'd prefer that, so I'll go this route.

> Does this need :not(:-moz-lwtheme)?

Since the only lwthemes that declare this variable are light/dark theme this works fine, but as mentioned above it might be clearer if we split this up.

> This doesn't seem right for scrolling tabs. I need to think a bit more about this, also in light of bug 1390221 and bug 1390025. Can you remove it from this patch and file a new bug if it isn't already covered by an existing bug?

This is what I've been using in bug 1398125 to draw a border around light/dark theme tabs, but now all three themes need a border around them. We can just delay that bug until this one lands and solve it there if you like (the bug is annoying but should be easy to uplift).

> If you remove :not(:-moz-lwtheme) from the #TabsToolbar:not(:-moz-lwtheme) rule you added in browser.css, this shouldn't be needed.

That doesn't work for light theme, which needs dark font inside the tabs but light toolbar buttons on aero.

> Why is this in compacttheme.css?

I should have added a comment. It's the counterpart of this rule: https://searchfox.org/mozilla-central/rev/1c13d5cf85f904afb8976c02a80daa252b893fca/browser/themes/windows/browser-aero.css#334

That rule has :not(:-moz-lwtheme), but we want it to apply to light/dark theme as well, since they also use aero glass.

Comment 6

[Johann Hofmann [:johannh]]

Comment on attachment 8909276 [details]
Bug 1399498 - Make chrome background show in tabs on Windows 7 and remove glass fog on default theme.

https://reviewboard.mozilla.org/r/180866/#review186388

> This is what I've been using in bug 1398125 to draw a border around light/dark theme tabs, but now all three themes need a border around them. We can just delay that bug until this one lands and solve it there if you like (the bug is annoying but should be easy to uplift).

Thinking about it again, I might also remove this part from bug 1398125 and file an entirely new bug.

Comment 7

[Johann Hofmann [:johannh]]

Comment on attachment 8909276 [details]
Bug 1399498 - Make chrome background show in tabs on Windows 7 and remove glass fog on default theme.

https://reviewboard.mozilla.org/r/180866/#review186388

> Please document what you're doing here

Regarding the border-image override, I'll remove that from the patch and make a separate bug, it's not that important and maybe not correct anyway.

Comment 8

[Johann Hofmann [:johannh]]

Comment on attachment 8909276 [details]
Bug 1399498 - Make chrome background show in tabs on Windows 7 and remove glass fog on default theme.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/180866/diff/1-2/

Comment 9

[Dão Gottwald [:dao]]

Comment on attachment 8909276 [details]
Bug 1399498 - Make chrome background show in tabs on Windows 7 and remove glass fog on default theme.

https://reviewboard.mozilla.org/r/180866/#review186562

::: browser/themes/windows/browser.css:110
(Diff revision 2)
>  
> +/*
> + * Windows 7 draws the chrome background color as the tab background
> + * instead of in the tabs toolbar.
> + */
> +@media (-moz-os-version: windows-win7) {

I expect we don't want this for classic nor for high-contrast themes.

::: browser/themes/windows/browser.css:120
(Diff revision 2)
> +  #TabsToolbar:not(:-moz-lwtheme) {
> +    color: hsl(240,9%,98%);
> +  }
> +
> +  #tabbrowser-tabs > .tabbrowser-tab:not(:-moz-lwtheme) {
> +    background-color: hsl(235,33%,19%);

Why is this not set on .tab-background?

::: browser/themes/windows/browser.css:124
(Diff revision 2)
> +  #tabbrowser-tabs > .tabbrowser-tab:not(:-moz-lwtheme) {
> +    background-color: hsl(235,33%,19%);
> +  }
> +
> +  /* Always show full-height tab separators on tabs with borders. */
> +  #tabbrowser-tabs > .tabbrowser-tab::before {

Why #tabbrowser-tabs > ?

::: browser/themes/windows/browser.css:129
(Diff revision 2)
> +  #tabbrowser-tabs > .tabbrowser-tab::before {
> +    border-image: none !important;
> +  }
> +
> +  /* Show border on tabs with background colors in Windows 7. */
> +  #tabbrowser-tabs > .tabbrowser-tab > .tab-stack > .tab-background {

Why #tabbrowser-tabs > .tabbrowser-tab > .tab-stack > ?

::: browser/themes/windows/compacttheme.css:38
(Diff revision 2)
>  @media (-moz-windows-glass) {
> -  .tabbrowser-tab {
> +  /* Always show light toolbar buttons on aero glass surface. */
> +  #main-window #TabsToolbar .toolbarbutton-1,
> +  #main-window #TabsToolbar .scrollbutton-up,
> +  #main-window #TabsToolbar .scrollbutton-down {
> +    fill: hsl(240,9%,98%) !important;

This shouldn't be needed as long as we set the right text color on #TabsToolbar.

::: browser/themes/windows/compacttheme.css:55
(Diff revision 2)
> +  }
> +}
> +
> +@media (-moz-os-version: windows-win7) {
> +  /* Show the tabs toolbar background color inside tabs on Win 7. */
> +  #tabbrowser-tabs > .tabbrowser-tab {

Why did you change this selector?

Comment 10

[Johann Hofmann [:johannh]]

Comment on attachment 8909276 [details]
Bug 1399498 - Make chrome background show in tabs on Windows 7 and remove glass fog on default theme.

https://reviewboard.mozilla.org/r/180866/#review186562

> I expect we don't want this for classic nor for high-contrast themes.

Why not?

> Why #tabbrowser-tabs > .tabbrowser-tab > .tab-stack > ?

As with the background, to override using a more specific selector, I'll just use !important and let you decide what you prefer. :)

> This shouldn't be needed as long as we set the right text color on #TabsToolbar.

Again, light theme has black as #TabsToolbar color but needs white as fill for toolbarbutton icons because of aero. Maybe I'm missing something, how would it work without this override?

> Why did you change this selector?

While moving code around. I didn't notice the change in my diff view.

Comment 11

[Johann Hofmann [:johannh]]

Comment on attachment 8909276 [details]
Bug 1399498 - Make chrome background show in tabs on Windows 7 and remove glass fog on default theme.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/180866/diff/2-3/

Comment 12

[Dão Gottwald [:dao]]

(In reply to Johann Hofmann [:johannh] from comment #10)
> Comment on attachment 8909276 [details]
> Bug 1399498 - Make chrome background show in tabs on Windows 7 and remove
> glass fog on default theme.
> 
> https://reviewboard.mozilla.org/r/180866/#review186562
> 
> > I expect we don't want this for classic nor for high-contrast themes.
> 
> Why not?

Classic already has a darker title bar, and adding this custom background on top of that probably looks extra outlandish. For High Contrast this is just counterproductive in terms of accessibility, aesthetics aside.

> > This shouldn't be needed as long as we set the right text color on #TabsToolbar.
> 
> Again, light theme has black as #TabsToolbar color but needs white as fill
> for toolbarbutton icons because of aero. Maybe I'm missing something, how
> would it work without this override?

The tabs need black text color but not the toolbar itself.

Comment 13

[Johann Hofmann [:johannh]]

(In reply to Dão Gottwald [::dao] from comment #12)
> > > This shouldn't be needed as long as we set the right text color on #TabsToolbar.
> > 
> > Again, light theme has black as #TabsToolbar color but needs white as fill
> > for toolbarbutton icons because of aero. Maybe I'm missing something, how
> > would it work without this override?
> 
> The tabs need black text color but not the toolbar itself.

So that would require us to override the TabsToolbar color to always be white in compacttheme.css AND override the tab color to be the original TabsToolbar color in compacttheme.css as well, I don't really understand how that is superior to only overriding the fill value of the TabsToolbar. Am I missing something?

Comment 14

[Johann Hofmann [:johannh]]

(In reply to Dão Gottwald [::dao] from comment #12)
> (In reply to Johann Hofmann [:johannh] from comment #10)
> > Comment on attachment 8909276 [details]
> > Bug 1399498 - Make chrome background show in tabs on Windows 7 and remove
> > glass fog on default theme.
> > 
> > https://reviewboard.mozilla.org/r/180866/#review186562
> > 
> > > I expect we don't want this for classic nor for high-contrast themes.
> > 
> > Why not?
> 
> Classic already has a darker title bar, and adding this custom background on
> top of that probably looks extra outlandish. For High Contrast this is just
> counterproductive in terms of accessibility, aesthetics aside.

So I tried this and it just looks strange for dark and light theme. For two different reasons. For dark theme, it's a bit strange to retain the blue background while the rest of the UI goes dark. For light theme, we would have to override the TabsToolbar color to white since the black color doesn't work on the blue background. Do you really think we have to special-case classic theme? Would you agree to only not show the background color on the default theme for win classic?

Comment 15

[Dão Gottwald [:dao]]

(In reply to Johann Hofmann [:johannh] from comment #13)
> > The tabs need black text color but not the toolbar itself.
> 
> So that would require us to override the TabsToolbar color to always be
> white in compacttheme.css AND override the tab color to be the original
> TabsToolbar color in compacttheme.css as well, I don't really understand how
> that is superior to only overriding the fill value of the TabsToolbar. Am I
> missing something?

It gets us consistent colors for items with text (e.g. zoom controls) on the toolbar.

(In reply to Johann Hofmann [:johannh] from comment #14)
> So I tried this and it just looks strange for dark and light theme. For two
> different reasons. For dark theme, it's a bit strange to retain the blue
> background while the rest of the UI goes dark. For light theme, we would
> have to override the TabsToolbar color to white since the black color
> doesn't work on the blue background. Do you really think we have to
> special-case classic theme? Would you agree to only not show the background
> color on the default theme for win classic?

I'd have to see it to judge it. I don't think we should be concerned with Windows Classic in this bug.

Comment 16

[Johann Hofmann [:johannh]]

(In reply to Dão Gottwald [::dao] from comment #15)
> (In reply to Johann Hofmann [:johannh] from comment #13)
> > > The tabs need black text color but not the toolbar itself.
> > 
> > So that would require us to override the TabsToolbar color to always be
> > white in compacttheme.css AND override the tab color to be the original
> > TabsToolbar color in compacttheme.css as well, I don't really understand how
> > that is superior to only overriding the fill value of the TabsToolbar. Am I
> > missing something?
> 
> It gets us consistent colors for items with text (e.g. zoom controls) on the
> toolbar.

Ah, indeed. That's a good reason! I'll change it.

> (In reply to Johann Hofmann [:johannh] from comment #14)
> > So I tried this and it just looks strange for dark and light theme. For two
> > different reasons. For dark theme, it's a bit strange to retain the blue
> > background while the rest of the UI goes dark. For light theme, we would
> > have to override the TabsToolbar color to white since the black color
> > doesn't work on the blue background. Do you really think we have to
> > special-case classic theme? Would you agree to only not show the background
> > color on the default theme for win classic?
> 
> I'd have to see it to judge it. I don't think we should be concerned with
> Windows Classic in this bug.

Ok, let's open a follow-up bug for properly looking at Windows Classic & High Contrast and take anything that concerns it out of this patch.

Comment 17

[Johann Hofmann [:johannh]]

Comment on attachment 8909276 [details]
Bug 1399498 - Make chrome background show in tabs on Windows 7 and remove glass fog on default theme.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/180866/diff/3-4/

Comment 18

[Dão Gottwald [:dao]]

Comment on attachment 8909276 [details]
Bug 1399498 - Make chrome background show in tabs on Windows 7 and remove glass fog on default theme.

https://reviewboard.mozilla.org/r/180866/#review187050

::: browser/themes/windows/browser.css:110
(Diff revision 4)
>  
> +/*
> + * Windows 7 draws the chrome background color as the tab background
> + * instead of in the tabs toolbar.
> + */
> +@media (-moz-os-version: windows-win7) {

Need to query -moz-windows-default-theme here

::: browser/themes/windows/compacttheme.css:36
(Diff revision 4)
>  }
>  
>  @media (-moz-windows-glass) {
> +  /* Always show light toolbar elements on aero glass surface. */
> +  #TabsToolbar,
> +  #TabsToolbar .toolbarbutton-1 {

.toolbarbutton-1 already inherits color

Comment 19

[Johann Hofmann [:johannh]]

(In reply to Dão Gottwald [::dao] from comment #18)
> ::: browser/themes/windows/compacttheme.css:36
> (Diff revision 4)
> >  }
> >  
> >  @media (-moz-windows-glass) {
> > +  /* Always show light toolbar elements on aero glass surface. */
> > +  #TabsToolbar,
> > +  #TabsToolbar .toolbarbutton-1 {
> 
> .toolbarbutton-1 already inherits color

This is to override https://searchfox.org/mozilla-central/rev/d08b24e613cac8c9c5a4131452459241010701e0/browser/themes/shared/compacttheme.inc.css#93.

Would you like me to add a comment?

Comment 20

[Dão Gottwald [:dao]]

(In reply to Johann Hofmann [:johannh] from comment #19)
> (In reply to Dão Gottwald [::dao] from comment #18)
> > ::: browser/themes/windows/compacttheme.css:36
> > (Diff revision 4)
> > >  }
> > >  
> > >  @media (-moz-windows-glass) {
> > > +  /* Always show light toolbar elements on aero glass surface. */
> > > +  #TabsToolbar,
> > > +  #TabsToolbar .toolbarbutton-1 {
> > 
> > .toolbarbutton-1 already inherits color
> 
> This is to override
> https://searchfox.org/mozilla-central/rev/
> d08b24e613cac8c9c5a4131452459241010701e0/browser/themes/shared/compacttheme.
> inc.css#93.
> 
> Would you like me to add a comment?

Ideally that rule shouldn't target the buttons directly either.

Comment 21

[Johann Hofmann [:johannh]]

(In reply to Dão Gottwald [::dao] from comment #20)
> (In reply to Johann Hofmann [:johannh] from comment #19)
> > (In reply to Dão Gottwald [::dao] from comment #18)
> > > ::: browser/themes/windows/compacttheme.css:36
> > > (Diff revision 4)
> > > >  }
> > > >  
> > > >  @media (-moz-windows-glass) {
> > > > +  /* Always show light toolbar elements on aero glass surface. */
> > > > +  #TabsToolbar,
> > > > +  #TabsToolbar .toolbarbutton-1 {
> > > 
> > > .toolbarbutton-1 already inherits color
> > 
> > This is to override
> > https://searchfox.org/mozilla-central/rev/
> > d08b24e613cac8c9c5a4131452459241010701e0/browser/themes/shared/compacttheme.
> > inc.css#93.
> > 
> > Would you like me to add a comment?
> 
> Ideally that rule shouldn't target the buttons directly either.

So, I don't think that rule even applies anymore, at least it's not relevant to the bug it's trying to fix (on my machine). I'll try to remove it and fix whatever we break.

Comment 22

[Johann Hofmann [:johannh]]

Comment on attachment 8909276 [details]
Bug 1399498 - Make chrome background show in tabs on Windows 7 and remove glass fog on default theme.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/180866/diff/4-5/

Comment 23

[Dão Gottwald [:dao]]

Comment on attachment 8909276 [details]
Bug 1399498 - Make chrome background show in tabs on Windows 7 and remove glass fog on default theme.

https://reviewboard.mozilla.org/r/180866/#review187118

::: browser/themes/shared/compacttheme.inc.css:93
(Diff revision 5)
>  
> -#navigator-toolbox .toolbarbutton-1,
>  .browserContainer > findbar .findbar-button,
>  #PlacesToolbar toolbarbutton.bookmark-item {
>    color: var(--chrome-color);
>    text-shadow: none;

Is the rest of this rule still needed? Would be a good opportunity to remove this.

Comment 24

[Johann Hofmann [:johannh]]

(In reply to Dão Gottwald [::dao] from comment #23)
> Comment on attachment 8909276 [details]
> Bug 1399498 - Make chrome background show in tabs on Windows 7 and remove
> glass fog on default theme.
> 
> https://reviewboard.mozilla.org/r/180866/#review187118
> 
> ::: browser/themes/shared/compacttheme.inc.css:93
> (Diff revision 5)
> >  
> > -#navigator-toolbox .toolbarbutton-1,
> >  .browserContainer > findbar .findbar-button,
> >  #PlacesToolbar toolbarbutton.bookmark-item {
> >    color: var(--chrome-color);
> >    text-shadow: none;
> 
> Is the rest of this rule still needed? Would be a good opportunity to remove
> this.

It seems like the text-shadow rule is still needed, I'll remove the color part.

Comment 25

[Johann Hofmann [:johannh]]

Comment on attachment 8909276 [details]
Bug 1399498 - Make chrome background show in tabs on Windows 7 and remove glass fog on default theme.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/180866/diff/5-6/

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ab1e0e064452
Make chrome background show in tabs on Windows 7 and remove glass fog on default theme. r=dao

Comment 27

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ab1e0e064452

Comment 28

[Geobert Quach]

Very bad move: dark favicons are invisible now (example: Atlassian Crucible favicon)

Comment 29

[Johann Hofmann [:johannh]]

(In reply to Geobert Quach from comment #28)
> Very bad move: dark favicons are invisible now (example: Atlassian Crucible
> favicon)

You can follow bug 1094357.

Comment 30

[Valentina Claudia Ona]

I verified this issue using Windows 7 x32, ona Nightly 58.0a1 and Firefox Beta 57.0b4 with Build ID 20170928180207.
I will mark this as verified fixed.